Re: [PATCH bpf-next v2] selftests/bpf: add XDP test for per-flow LRU_HASH window updates

[bot+bpf-ci]

> selftests/bpf: add XDP test for per-flow LRU_HASH window updates

TL;DR:
- Test issue: wrap subtest uses identical packet lengths, so it can't detect a
  wrong post-wrap index
- trunc subtest sends a non-IPv4 frame, so it likely never reaches the
  truncation checks

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_lru_window.c b/tools/testing/selftests/bpf/prog_tests/xdp_lru_window.c
> new file mode 100644
> index 000000000000..55175c2165ec
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_lru_window.c
> @@ -0,0 +1,168 @@

[ ... ]

> +static void test_one_and_wrap(void)
> +{
> +	struct xdp_lru_window_state st;
> +	struct xdp_lru_window_key key;
> +	int i, n;
> +
> +	reset_map();
> +	if (inject(1, DST_PORT))
> +		return;
> +	fill_key(&key, DST_PORT);
> +	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &st), "lookup"))
> +		return;
> +	ASSERT_EQ(st.seq, 1, "seq");
> +	ASSERT_EQ(st.pkt_len[0], sizeof(struct ipv4_packet), "len0");
> +
> +	n = AGGREGATION_WINDOW + 5;
> +	if (inject(n - 1, DST_PORT))
> +		return;
> +	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &st), "lookup wrap"))
> +		return;
> +	ASSERT_EQ(st.seq, n, "seq wrap");
> +	for (i = 0; i < AGGREGATION_WINDOW; i++)
> +		ASSERT_EQ(st.pkt_len[i], sizeof(struct ipv4_packet), "slot");
> +}

Can the final "slot" loop detect a wrong post-wrap write?

Every packet sent by inject() is the same pkt_v4 copy, so every write stores
sizeof(struct ipv4_packet) (54).  After the first 50 packets, slots 0..49
already hold 54, and packets 51..55 write 54 into slots 0..4 again.  The
loop therefore checks nothing that was not already true before the index
wrapped.

The "seq wrap" check only shows the program did not take the
"idx >= AGGREGATION_WINDOW" early return.  A miscompiled modulo that
produced a wrong but in-range index would still pass.

The commit message says this subtest checks that "the bounded array wraps".
Would it make sense to send the post-wrap packets with a different length
(run_pkt() accepts any len >= ETH_HLEN, and the program records
data_end - data), then check that slots 0..4 hold the new length while
slots 5..49 keep the old one?

[ ... ]

> +static void test_trunc(void)
> +{
> +	struct xdp_lru_window_state before, after;
> +	struct xdp_lru_window_key key, next;
> +	unsigned char short_pkt[sizeof(struct ethhdr)] = {};
> +	int err, retval;
> +
> +	reset_map();
> +	err = run_pkt(short_pkt, sizeof(short_pkt), &retval);
> +	if (err)
> +		return;
> +	ASSERT_EQ(retval, XDP_PASS, "trunc retval");
> +	err = bpf_map_get_next_key(map_fd, NULL, &next);
> +	ASSERT_EQ(err, -ENOENT, "trunc no insert");

Does this subtest reach any of the program's truncation checks?

short_pkt is 14 zero bytes, so eth->h_proto is 0.  In
progs/xdp_lru_window.c the program passes the Ethernet bounds check and then
returns at:

    if (eth->h_proto != bpf_htons(ETH_P_IP))
            return XDP_PASS;

The IPv4 length check ((void *)(iph + 1) > data_end) and the TCP length
check ((void *)(th + 1) > data_end) never run, here or in any other
subtest.

The "trunc no insert" and "trunc no mutate" assertions would pass the same
way for a full-length non-IPv4 frame, so this exercises the ethertype
filter rather than truncated-packet handling.  The commit message describes
this subtest as checking that "a short (parse-failing) packet neither
inserts a new entry nor mutates an existing one".

Should short_pkt have h_proto set to htons(ETH_P_IP) so the IPv4 length check
is reached?  To also reach the TCP header check, a truncated copy of pkt_v4
of sizeof(struct ethhdr) + sizeof(struct iphdr) bytes could be sent.
bpf_prog_test_run_xdp() only requires data_size_in >= ETH_HLEN, so both
sizes are accepted.

> +
> +	if (inject(1, DST_PORT))
> +		return;
> +	fill_key(&key, DST_PORT);
> +	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &before), "setup"))
> +		return;
> +	if (run_pkt(short_pkt, sizeof(short_pkt), &retval))
> +		return;
> +	ASSERT_EQ(retval, XDP_PASS, "trunc2 retval");
> +	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &after), "after"))
> +		return;
> +	ASSERT_EQ(after.seq, before.seq, "trunc no mutate");
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36965595251