Re: [PATCH bpf v2 2/2] selftests/bpf: add xdp_shrink_frags

[Jakub Kicinski <kuba@kernel.org>]

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?