Re: [PATCH bpf-next 1/1] selftests: drv-net: add XDP RX checksum metadata tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The BPF program overwrites checksum metadata on every packet, which may cause TCP tests to spuriously fail if trailing control packets lack checksum offload.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem comment style (opening `/*` must be on its own line).
--

--- Patch [1]: [PATCH bpf-next 1/1] selftests: drv-net: add XDP RX checksum metadata tests ---
commit d0f16a1bf595df9c0de880826075e44c316407e0
Author: Vladimir Vdovin <deliran@verdict.gg>

selftests: drv-net: add XDP RX checksum metadata tests

Extend the xdp_metadata driver tests with coverage for
bpf_xdp_metadata_rx_checksum(). Adds a BPF program to read and store
the verdict, and Python tests to validate both valid and invalid RX
checksums.

> diff --git a/tools/testing/selftests/drivers/net/hw/xdp_metadata.py b/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
> index 33a1985356d90..687c43a1797a4 100644
> --- a/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
> +++ b/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
[ ... ]
> @@ -130,6 +146,91 @@ def test_xdp_rss_hash(cfg, proto):
>              f"RSS hash type should include L4 for {proto.upper()} traffic")
>  
>  
> +def _require_rx_csum_meta(cfg):
[ ... ]
> +@ksft_variants([
> +    KsftNamedVariant("tcp", "tcp"),
> +    KsftNamedVariant("udp", "udp"),
> +])
> +def test_xdp_rx_csum_valid(cfg, proto):
[ ... ]
> +    csum = bpf_map_dump(csum_map_id)
> +
> +    pkt_cnt = csum.get(_CSUM_KEY_PKT_CNT, 0)
> +    err_cnt = csum.get(_CSUM_KEY_ERR_CNT, 0)
> +    ip_summed = csum.get(_CSUM_KEY_IP_SUMMED, 0)
> +
> +    ksft_ge(pkt_cnt, 1, comment="should have received at least one packet")
> +    ksft_eq(err_cnt, 0, comment=f"RX checksum error count: {err_cnt}")
> +
> +    ksft_pr(f"  ip_summed: {ip_summed:#x} cksum: "
> +            f"{csum.get(_CSUM_KEY_CKSUM, 0):#010x} "
> +            f"level: {csum.get(_CSUM_KEY_LEVEL, 0)}")
> +    ksft_ne(ip_summed & (XDP_CHECKSUM_UNNECESSARY | XDP_CHECKSUM_COMPLETE), 0,
> +            "device should report a checksum verdict for a valid packet")

[Severity: Medium]
Will this test sporadically fail for TCP traffic? Since multiple packets might
be exchanged, the final state of ip_summed could be from a trailing control
packet (like FIN or RST) that lacks checksum offload, leading to a failure
here.

> +
> +
> +def test_xdp_rx_csum_invalid(cfg):
[ ... ]
> diff --git a/tools/testing/selftests/net/lib/xdp_metadata.bpf.c b/tools/testing/selftests/net/lib/xdp_metadata.bpf.c
> index f71f59215239f..70decae0a6637 100644
> --- a/tools/testing/selftests/net/lib/xdp_metadata.bpf.c
> +++ b/tools/testing/selftests/net/lib/xdp_metadata.bpf.c
[ ... ]
> @@ -40,6 +41,24 @@ struct {
>  	__uint(max_entries, 4);
>  } map_rss SEC(".maps");
>  
> +/* RX checksum results: key 0 = ip_summed bitmask, key 1 = hw cksum value,
> + * key 2 = cksum level, key 3 = packet count, key 4 = error count.
> + */

[Severity: Low]
This isn't a bug, but the BPF subsystem prefers multi-line comments to have
the opening /* on its own line. Could this be adjusted?

> +enum {
> +	CSUM_KEY_IP_SUMMED = 0,
[ ... ]
> @@ -55,8 +74,20 @@ enum xdp_rss_hash_type {
>  	XDP_RSS_L4_ICMP = 1U << 8,
>  };
>  
> +/* Mirror of enum xdp_checksum from include/net/xdp.h.
> + * Needed because the enum is not part of UAPI headers.
> + */

[Severity: Low]
This isn't a bug, but could the opening /* be placed on its own line
to match BPF comment style?

> +enum xdp_checksum {
> +	XDP_CHECKSUM_NONE = 1U << 0,
[ ... ]
> @@ -78,41 +109,39 @@ static __always_inline __u16 get_dest_port(void *l4, void *data_end,
>  	return 0;
>  }
>  
> -SEC("xdp")
> -int xdp_rss_hash(struct xdp_md *ctx)
> +/* Return true when the packet matches the L4 protocol and destination
> + * port configured in map_xdp_setup (zero/unset filters match anything).
> + */

[Severity: Low]
This isn't a bug, but could the opening /* be placed on its own line
to match BPF comment style?

> +static __always_inline bool xdp_match_setup(struct xdp_md *ctx)
>  {
[ ... ]
> @@ -160,4 +204,46 @@ int xdp_rss_hash(struct xdp_md *ctx)
>  	return XDP_PASS;
>  }
>  
> +SEC("xdp")
> +int xdp_rx_csum(struct xdp_md *ctx)
> +{
[ ... ]
> +	ret = bpf_xdp_metadata_rx_checksum(ctx, &ip_summed, &cksum,
> +					   &cksum_level);
> +	if (ret < 0) {
> +		key = CSUM_KEY_ERR_CNT;
> +		cnt = bpf_map_lookup_elem(&map_csum, &key);
> +		if (cnt)
> +			__sync_fetch_and_add(cnt, 1);
> +		return XDP_PASS;
> +	}
> +
> +	key = CSUM_KEY_IP_SUMMED;
> +	val = (__u32)ip_summed;
> +	bpf_map_update_elem(&map_csum, &key, &val, BPF_ANY);

[Severity: Medium]
Does this update overwrite the checksum metadata on every packet? If so, the
map will only contain the verdict of the last processed packet, which could
be an empty control packet without checksum offload, leading to incorrect test
failures. Can we accumulate the results or only update if the packet carries a
payload?

> +
> +	key = CSUM_KEY_CKSUM;
> +	bpf_map_update_elem(&map_csum, &key, &cksum, BPF_ANY);
> +
> +	key = CSUM_KEY_LEVEL;
> +	val = cksum_level;
> +	bpf_map_update_elem(&map_csum, &key, &val, BPF_ANY);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6c7a059057d309e999b706b5b547be9c620ce544.1783514455.git.deliran@verdict.gg?part=1