Re: [PATCH net] selftests: net: Fix checksums in xdp_native

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incomplete checksum fold in csum_fold_helper causes persistent off-by-one errors
- [Medium] Unconditional update of IPv4 UDP checksum corrupts packets with no checksum
- [Medium] Offset parameter is used to modify packet headers before it is bounds-checked and clamped
--

commit 2e32120c7c113684b13c8a13f5c0a114c19b097e
Author: Nimrod Oren <noren@nvidia.com>

selftests: net: Fix checksums in xdp_native

This commit fixes data adjustment cases for IPv4 by adding updates to the
IP and UDP checksums in the IPv4 branch.

> diff --git a/tools/testing/selftests/net/lib/xdp_native.bpf.c b/tools/testing/selftests/net/lib/xdp_native.bpf.c
> --- a/tools/testing/selftests/net/lib/xdp_native.bpf.c
> +++ b/tools/testing/selftests/net/lib/xdp_native.bpf.c
> @@ -268,6 +268,16 @@ static int xdp_mode_tx_handler(struct xdp_md *ctx, __u16 port)
>  	return XDP_PASS;
>  }
>  
> +static __always_inline __u16 csum_fold_helper(__u32 csum)
> +{
> +	return ~((csum & 0xffff) + (csum >> 16));
> +}

Does this folding function handle the carry bit completely? Standard 1's
complement checksum folding usually requires a second addition to handle
the carry from the first addition.

Since the 32-bit seed passed to bpf_csum_diff for iph->check is constructed
via ~((__u32)iph->check), the upper 16 bits are 0xFFFF. This guarantees the
intermediate 32-bit sum will exceed 0xFFFF, producing a carry that might
be discarded here, resulting in an off-by-one checksum.

[ ... ]

> @@ -281,21 +291,22 @@ static void *update_pkt(struct xdp_md *ctx, __s16 offset, __u32 *udp_csum)
>  
>  	if (eth->h_proto == bpf_htons(ETH_P_IP)) {
>  		struct iphdr *iph = data + sizeof(*eth);
[ ... ]
> +		len = iph->tot_len;
> +		len_new = bpf_htons(bpf_ntohs(len) + offset);
> +		iph->tot_len = len_new;
> +		iph->check = csum_fold_helper(
> +			bpf_csum_diff(&len, sizeof(len), &len_new,
> +				      sizeof(len_new), ~((__u32)iph->check)));
>  	} else if (eth->h_proto  == bpf_htons(ETH_P_IPV6)) {
[ ... ]
> +	len = udph->len;
> +	len_new = bpf_htons(bpf_ntohs(len) + offset);
> +
> +	*udp_csum = ~((__u32)udph->check);
> +	*udp_csum = bpf_csum_diff(&len, sizeof(len), &len_new,
> +				  sizeof(len_new), *udp_csum);
> +	*udp_csum = bpf_csum_diff(&len, sizeof(len), &len_new,
> +				  sizeof(len_new), *udp_csum);
> +
>  	udph->len = len_new;

If this is an IPv4 packet and the original UDP checksum is 0 (meaning no
checksum was generated by the transmitter), will this differential update
produce a non-zero, invalid checksum?

The receiver might then attempt to validate this new checksum against the
unchecksummed payload, causing the packet to be dropped. Should we skip the
UDP checksum update entirely for IPv4 when udph->check == 0?

[ ... ]

> @@ -359,7 +364,7 @@ static int xdp_adjst_tail_shrnk_data(struct xdp_md *ctx, __u16 offset,
>  		return -1;
>  
>  	udp_csum = bpf_csum_diff((__be32 *)tmp_buff, offset, 0, 0, udp_csum);
> -	udph->check = (__u16)csum_fold_helper(udp_csum);
> +	udph->check = (__u16)csum_fold_udp_helper(udp_csum);

Is it safe that the raw offset parameter is directly passed to update_pkt
(which modifies iph->tot_len and udph->len) before the offset is clamped
to MAX_ADJST_OFFSET (256) in this caller and others?

If the raw offset is larger than 256, the packet headers will be reduced by
the full offset amount, but the actual packet length adjustment via
bpf_xdp_adjust_tail will only use the clamped amount.

Could this result in packets with mismatched logical header lengths and
physical payload sizes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513072355.1083381-1-noren@nvidia.com?part=1