This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests/bpf: add xdp_shrink_frags
Adds a new test_progs test with two subtests (tun and veth) that attach an
xdp.frags program shrinking the tail by 3000 bytes so a whole frag is
released. The intent is to reproduce a page_pool frag being freed with the
wrong memory type on skb-backed XDP paths.
Two questions below about what the test actually observes.
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
> b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
> new file mode 100644
> index 0000000000000..f3d8a84a6dc9c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_shrink_frags.c
> @@ -0,0 +1,163 @@
[ ... ]
> +/*
> + * Similar to flow_dissector.c: writev() an IFF_NAPI_FRAGS tap to build a
> + * nonlinear skb (sized for 4K pages, like xdp_adjust_tail.c) that tun runs
> + * through do_xdp_generic().
> + */
> +static void test_tun(struct xdp_shrink_frags *skel)
> +{
> + __u8 head[74], frag1[2048], frag2[2048];
[Severity: Medium]
Is the 74 + 2048 + 2048 byte frame guaranteed to still be nonlinear when the
program runs on kernels with PAGE_SIZE larger than 4096?
skb_pp_cow_data() in net/core/skbuff.c bounds the linear head by the page
size:
max_head_size = SKB_WITH_OVERHEAD(PAGE_SIZE - headroom);
...
size = min_t(u32, skb->len, max_head_size);
...
for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++) {
With PAGE_SIZE 16384 or 65536 the whole 4170-byte frame fits in the head, so
the frag loop body never runs and the xdp_buff has no frags.
tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c handles this by
branching on getpagesize():
int page_size = getpagesize();
...
if (page_size == 65536)
test_xdp_adjust_frags_tail_grow_64k();
Would a getpagesize() branch or skip be appropriate here too, given that
config.aarch64 is shipped in the bpf selftests?
[ ... ]
> + n = writev(tap_fd, iov, ARRAY_SIZE(iov));
> + ASSERT_EQ(n, sizeof(head) + sizeof(frag1) + sizeof(frag2), "writev");
> +
> + usleep(100 * 1000);
> + ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
[Severity: Medium]
Can this subtest distinguish a buggy kernel from a fixed one?
The failure mode the commit message names ("Bad page state ... page_pool
leak") is produced by bad_page() in mm/page_alloc.c, with the reason string
set in page_bad_reason():
if (unlikely(page_pool_page_is_pp(page)))
bad_reason = "page_pool leak";
bad_page() is a static void that only emits pr_alert()/dump_page()/
dump_stack() and then:
add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
No error is propagated back to bpf_xdp_adjust_tail(), writev() or ping, and
the test does not read /proc/sys/kernel/tainted, scan kmsg, or look at
page_pool inflight statistics.
So the assertions here (bpf_xdp_attach, the writev length, and shrink_ran)
all succeed identically on a kernel with and without the fix. Would it make
sense to add an observable oracle (taint bit, kmsg scan, or page_pool
stats), or alternatively to state in the commit message that this is a splat
generator requiring manual dmesg inspection?
[ ... ]
> +static void test_veth(struct xdp_shrink_frags *skel)
> +{
[ ... ]
> + SYS_NOFAIL("ip netns exec " VETH_NETNS
> + " ping -q -s 5000 -c 3 -W 1 " VETH_LOCAL_IP);
> +
> + ASSERT_GT(skel->bss->shrink_ran, 0, "xdp_prog_ran");
[Severity: Medium]
The same two concerns apply here. The ping runs under SYS_NOFAIL and the
only assertion is shrink_ran > 0, so nothing observes whether the released
frag went back to its page_pool.
Additionally, the frag layout of a ~5 KB ping through an 8000-MTU veth is
decided by the sender's SG/GRO behaviour rather than by the test. If the
skb ends up with a single frag larger than 3000 bytes, the shrink happens in
place and no page is released at all.
On large-PAGE_SIZE kernels veth_convert_skb_to_xdp_buff() takes the other
branch after the COW leaves no frags:
if (skb_shinfo(skb)->nr_frags) {
...
xdp_buff_set_frag_pp(xdp);
} else {
xdp_buff_clear_frags_flag(xdp);
}
which bypasses the frag-release code entirely.
> diff --git a/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
> b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
> new file mode 100644
> index 0000000000000..62e3791ac81d9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/xdp_shrink_frags.c
> @@ -0,0 +1,23 @@
[ ... ]
> + /*
> + * The program is loaded with BPF_F_XDP_HAS_FRAGS (xdp.frags), so a
> + * nonlinear skb entering generic XDP is cow'd into page_pool memory
> + * before we run. Shrinking the tail far enough releases at least one
> + * whole frag, which must be returned to its page_pool. Count only a
> + * successful shrink so a too-small frame (e.g. ARP) does not satisfy
> + * the test.
> + */
> + if (bpf_xdp_adjust_tail(ctx, -3000) == 0)
> + __sync_fetch_and_add(&shrink_ran, 1);
[Severity: Medium]
Does a return value of 0 here really imply a frag was released?
bpf_xdp_adjust_tail() in net/core/filter.c only enters the frag path when
the buffer is nonlinear:
if (unlikely(xdp_buff_has_frags(xdp))) { /* non-linear xdp buff */
if (offset < 0)
return bpf_xdp_frags_shrink_tail(xdp, -offset);
...
xdp->data_end = data_end;
return 0;
A purely linear buffer takes the bottom path and also returns 0, so
shrink_ran gets incremented without bpf_xdp_shrink_data() ever running.
That seems to contradict the comment's claim that only a genuine
frag-releasing shrink is counted.
Would asserting somewhere that the buffer actually had frags (for example
comparing ctx->data_end - ctx->data against the total frame length) make the
counter mean what the comment says?
