Re: [PATCH net v2 2/2] selftests/bpf: cover AF_XDP multi-buffer TX software checksum

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] generate_packet() in
  tools/testing/selftests/bpf/prog_tests/xdp_metadata.c fills the…

Pre-existing issues:
- [Medium] The freplace check in test_xdp_metadata() never retries.

--

selftests/bpf: cover AF_XDP multi-buffer TX software checksum

This commit extends test_xdp_metadata to send a UDP packet once as a
single TX descriptor and once split across two descriptors with
XDP_PKT_CONTD. The existing metadata and checksum checks run for both.
It adds a bind_flags argument to open_xsk(), a split_len argument to
generate_packet() and a num argument to complete_tx().

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> index 5c31054ad4a4c..1cf70fb800fba 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c

[ ... ]

> @@ -133,36 +135,36 @@ static void close_xsk(struct xsk *xsk)

[ ... ]

> -	eth = data;
> +	memset(pkt, 0, sizeof(pkt));
> +	eth = (void *)pkt;
>  	iph = (void *)(eth + 1);
>  	udph = (void *)(iph + 1);
>  
> -	memcpy(eth->h_dest, "\x00\x00\x00\x00\x00\x02", ETH_ALEN);
> -	memcpy(eth->h_source, "\x00\x00\x00\x00\x00\x01", ETH_ALEN);
> +	memcpy(eth->h_dest, RX_MAC, ETH_ALEN);
> +	memcpy(eth->h_source, TX_MAC, ETH_ALEN);

[Severity: Medium]
Should RX_MAC and TX_MAC be copied here? In this file they are text
strings meant for the ip link commands:

#define TX_MAC "00:00:00:00:00:01"
#define RX_MAC "00:00:00:00:00:02"

With ETH_ALEN as the length, memcpy() copies the first six ASCII
characters, "00:00:". Both h_dest and h_source then become
30:30:3a:30:30:3a.

The old binary literals matched the addresses set on veTX and veRX. Now
every AF_XDP frame that generate_packet() builds has a destination MAC
that doesn't match veRX. That covers the single-descriptor packet, the
new multi-buffer packet and the freplace packet. Won't eth_type_trans()
on the receive side mark these frames PACKET_OTHERHOST?

The test still passes because rx() in progs/xdp_metadata.c only filters
on the UDP destination port:

	if (udp->dest != bpf_htons(8080))
		return XDP_PASS;

Was this change meant to be part of a patch that only adds the
multi-buffer case? Keeping the "\x00\x00\x00\x00\x00\x02" and
"\x00\x00\x00\x00\x00\x01" literals would avoid the problem.

>  	eth->h_proto = htons(ETH_P_IP);

[ ... ]

> @@ -518,7 +564,7 @@ void test_xdp_metadata(void)
>  		goto out;
>  
>  	/* Send packet to trigger . */
> -	if (!ASSERT_GE(generate_packet(&tx_xsk, AF_XDP_CONSUMER_PORT), 0,
> +	if (!ASSERT_GE(generate_packet(&tx_xsk, AF_XDP_CONSUMER_PORT, 0), 0,
>  		       "generate freplace packet"))
>  		goto out;

[Severity: Medium]
This isn't a bug introduced by this patch, but the polling loop after
this send in test_xdp_metadata() never seems to run:

	int retries = 10;
	...
	while (!retries--) {
		if (bpf_obj2->bss->called)
			break;
		usleep(10);
	}
	ASSERT_GT(bpf_obj2->bss->called, 0, "not called");

On the first check retries is 10, so !retries-- is false and the loop
body is skipped. The assertion then runs right after sendto() and the
netns switch.

freplace_rx() increments called from the veth NAPI receive path, which
runs in NET_RX softirq. If that softirq is deferred to ksoftirqd, can
this assertion fail without a single retry?

Should the condition be retries-- > 0 instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024141157.59301-1-testwrzwrz%40gmail.com