Re: [PATCH bpf-next v5 8/8] selftests: net: add test for XDP_PASS skb checksum invalidation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `tc_check_csum` BPF program strictly requires IPv6 traffic, but the python test probe can send IPv4 traffic depending on the environment configuration, causing the test to falsely fail.
- [Medium] The test leaks a `tc filter` and its associated BPF program if the interface already had a `clsact` qdisc.
--

--- Patch [8]: [PATCH bpf-next v5 8/8] selftests: net: add test for XDP_PASS skb checksum invalidation ---
commit 2b96bd0af0eb846efe81e2f0246ecebcc855acd6
Author: Lorenzo Bianconi <lorenzo@kernel.org>

selftests: net: add test for XDP_PASS skb checksum invalidation

This commit adds a selftest to verify that a device running in XDP mode
correctly invalidates the hardware RX checksum when an attached XDP program
returns XDP_PASS. It checks this by attaching an XDP program and a TC
ingress program to inspect the resulting skb via the bpf_skb_rx_checksum()
kfunc.

> diff --git a/tools/testing/selftests/drivers/net/hw/xdp_metadata.py b/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
> index 33a1985356d90..8ccd34b776b4d 100755
> --- a/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
> +++ b/tools/testing/selftests/drivers/net/hw/xdp_metadata.py
[ ... ]
> @@ -130,6 +131,57 @@ def test_xdp_rss_hash(cfg, proto):
>              f"RSS hash type should include L4 for {proto.upper()} traffic")
>  
>  
> +def test_xdp_pass_rx_csum(cfg):
> +    """Test xdp_pass sets CHECKSUM_NONE on the resulting skb.
> +
> +    Attaches an XDP program that returns XDP_PASS and a TC ingress
> +    program that checks skb->ip_summed via bpf_skb_rx_checksum().
> +    Verifies the value is CHECKSUM_NONE.
> +    """
> +
> +    bpf_obj = cfg.net_lib_dir / "skb_metadata_csum.bpf.o"
> +    xdp_obj = cfg.net_lib_dir / "xdp_dummy.bpf.o"
> +
> +    # GRO may overwrite skb->ip_summed after the driver sets it,
> +    # so disable it to preserve the checksum set by the driver.
> +    ethtool(f"-K {cfg.ifname} gro off")
> +    defer(ethtool, f"-K {cfg.ifname} gro on")
> +    ip(f"link set dev {cfg.ifname} xdp obj {xdp_obj} sec xdp")
> +    defer(ip, f"link set dev {cfg.ifname} xdp off")
> +
> +    qdiscs = json.loads(cmd(f"tc -j qdisc show dev {cfg.ifname}").stdout)
> +    if not any(q['kind'] == 'clsact' for q in qdiscs):
> +        cmd(f"tc qdisc add dev {cfg.ifname} clsact")
> +        defer(cmd, f"tc qdisc del dev {cfg.ifname} clsact")
> +    cmd(f"tc filter add dev {cfg.ifname} ingress bpf da obj {bpf_obj} sec tc")

[Severity: Medium]
Does this leak the tc filter and BPF program if the interface already had a
clsact qdisc?

Since the filter deletion is not explicitly deferred here, it appears the
filter will remain active if the test bypassed the qdisc creation and
deletion. This could cause subsequent test runs to falsely fail if they
discover and query the leaked BPF program instead of the newly attached one.

> +
> +    progs = bpftool("prog list", json=True)
> +    tc_prog_id = None
> +    for p in progs:
> +        if p.get("name") == "tc_check_csum":
> +            tc_prog_id = p["id"]
> +            break
> +
> +    if tc_prog_id is None:
> +        raise KsftSkipEx("Could not find tc_check_csum BPF program")
> +
> +    maps = bpf_prog_map_ids(tc_prog_id)
> +    csum_map_id = maps.get("map_csum_result")
> +    if csum_map_id is None:
> +        raise KsftSkipEx("Could not find map_csum_result map")
> +
> +    for _ in range(10):
> +        _send_probe(cfg, 12345, proto="udp")

[Severity: Medium]
Will this cause spurious test failures if the test environment defaults
to IPv4?

The _send_probe() method will send IPv4 traffic if the environment's
cfg.addr_ipver defaults to 4. However, the BPF program tc_check_csum
explicitly ignores non-IPv6 traffic. If IPv4 packets are sent, the counters
will remain at zero and the test will fail.

> +
> +    result = bpf_map_dump(csum_map_id)
[ ... ]
> diff --git a/tools/testing/selftests/net/lib/skb_metadata_csum.bpf.c b/tools/testing/selftests/net/lib/skb_metadata_csum.bpf.c
> new file mode 100644
> index 0000000000000..6a953c4acfe78
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/skb_metadata_csum.bpf.c
[ ... ]
> @@ -0,0 +1,73 @@
[ ... ]
> +SEC("tc")
> +int tc_check_csum(struct __sk_buff *skb)
> +{
> +	void *data_end = (void *)(long)skb->data_end;
> +	void *data = (void *)(long)skb->data;
> +	__u32 ip_summed, csum_meta;
> +	struct ethhdr *eth = data;
> +	struct ipv6hdr *ip6;
> +	struct udphdr *udp;
> +
> +	if ((void *)(eth + 1) > data_end)
> +		return TC_ACT_OK;
> +
> +	if (eth->h_proto != bpf_htons(ETH_P_IPV6))
> +		return TC_ACT_OK;

[Severity: Medium]
Can this strict IPv6 check cause a mismatch with the python script?

As noted above, the python script calling _send_probe() might send IPv4 UDP
packets depending on the environment configuration, which would be silently
dropped here, bypassing the counter updates entirely.

> +
> +	ip6 = (void *)(eth + 1);
> +	if ((void *)(ip6 + 1) > data_end)
> +		return TC_ACT_OK;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-bpf-xdp-meta-rxcksum-v5-0-623d5c0d0ab7@kernel.org?part=8