Re: [RFC PATCH bpf-next v1 2/7] selftests/bpf: add test for bpf_xdp_assert_rx_csum over cpumap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The XDP program redirects all IPv6 packets unconditionally without filtering for the specific test UDP packet, causing the test to be flaky due to background IPv6 noise (e.g., MLD/ND).
- [Low] Multi-line comments do not follow the required BPF subsystem style.
--

commit 846ee2231e17e81d931043f95937a20e26145ff4
Author: Vladimir Vdovin <deliran@verdict.gg>

selftests/bpf: add test for bpf_xdp_assert_rx_csum over cpumap

This commit adds a selftest to verify that the bpf_xdp_assert_rx_csum helper
properly propagates checksum status through a cpumap redirect. It uses an XDP
program and an fexit hook on __xdp_build_skb_from_frame to observe the
resulting ip_summed value.

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_rx_csum.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_rx_csum.c
> new file mode 100644
> index 0000000000000..2def92fe11118
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_rx_csum.c
[ ... ]
> +static struct udp_pkt pkt = {
> +	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
[ ... ]
> +	.udp.len = __bpf_constant_htons(sizeof(struct udphdr) + 16),
> +};
> +
> +/* Inject one frame on veth0; it is received on veth1 where native XDP
> + * redirects it into the cpumap. Report the ip_summed the rebuilt skb carried.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF
subsystem style? The guidelines suggest putting the opening /* on its own line.

> +static int inject_and_observe(struct test_xdp_cpumap_rx_csum *skel, int sfd,
[ ... ]
> +void test_xdp_cpumap_rx_csum(void)
> +{
[ ... ]
> +	skel = test_xdp_cpumap_rx_csum__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel open_and_load"))
> +		goto out;
> +
> +	/* cpumap entry without a program: a plain redirect that forces the
> +	 * frame->skb conversion in __xdp_build_skb_from_frame().
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF
subsystem style?

> +	map_fd = bpf_map__fd(skel->maps.cpu_map);
[ ... ]
> +	ifindex_dst = if_nametoindex("veth1");
> +	ifindex_src = if_nametoindex("veth0");
> +	if (!ASSERT_GT(ifindex_dst, 0, "veth1 ifindex") ||
> +	    !ASSERT_GT(ifindex_src, 0, "veth0 ifindex"))
> +		goto out;
> +
> +	/* Native XDP so the redirect goes through xdp_convert_buff_to_frame(),
> +	 * which propagates the rx-csum flag into the frame. Generic mode would
> +	 * redirect a ready-made skb and never hit our code path.
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF
subsystem style?

> +	err = bpf_xdp_attach(ifindex_dst, bpf_program__fd(skel->progs.xdp_redir),
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_cpumap_rx_csum.c b/tools/testing/selftests/bpf/progs/test_xdp_cpumap_rx_csum.c
> new file mode 100644
> index 0000000000000..86c691887d254
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_cpumap_rx_csum.c
[ ... ]
> +SEC("xdp")
> +int xdp_redir(struct xdp_md *ctx)
> +{
> +	/* Assert the L4 checksum so the skb built on the cpumap redirect
> +	 * path is marked CHECKSUM_UNNECESSARY instead of validated in software.
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF
subsystem style?

> +	if (assert_csum)
> +		bpf_xdp_assert_rx_csum(ctx);
> +
> +	return bpf_redirect_map(&cpu_map, 0, 0);
> +}

[Severity: Medium]
Will this unconditionally redirect all packets (including background noise
like IPv6 MLD/ND) to the cpu_map? If the kernel generates asynchronous
background packets when the veth interfaces are brought up, could this
cause the test to be flaky?

> +
> +/* Observe ip_summed exactly as __xdp_build_skb_from_frame() leaves it, before
> + * GRO in the cpumap kthread can normalize it. tc-ingress would be too late:
> + * GRO software-validates a CHECKSUM_NONE skb and marks it UNNECESSARY anyway.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF
subsystem style?

> +SEC("fexit/__xdp_build_skb_from_frame")
> +int BPF_PROG(on_build, struct xdp_frame *xdpf, struct sk_buff *skb,
> +	     struct net_device *dev, struct sk_buff *ret)
> +{
> +	if (ret && ret->protocol == bpf_htons(ETH_P_IPV6)) {

[Severity: Medium]
Since the check only looks for ETH_P_IPV6, could this inadvertently capture
background IPv6 noise like MLD/ND packets and short-circuit the test loop in
inject_and_observe() with an unexpected ip_summed value? Might it be safer
to filter for the specific test UDP packet?

> +		observed_ip_summed = ret->ip_summed;
> +		seen = true;
> +	}
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630191510.81402-1-deliran@verdict.gg?part=2