Re: [PATCH bpf-next v5 7/8] selftests/bpf: Add test for bpf_skb_rx_checksum kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `csum_cnt` array map is sized incorrectly and explicitly drops valid `SKB_CSUM_PARTIAL` checksum states.
- [Low] Passing an `enum skb_csum *` to a kfunc expecting a `__u32 *` causes an incompatible pointer type warning.
- [Low] The multi-line comment for `bpf_skb_rx_checksum` violates the required BPF subsystem comment style.
--

--- Patch [7]: [PATCH bpf-next v5 7/8] selftests/bpf: Add test for bpf_skb_rx_checksum kfunc ---
commit b45897a4d5ef22742e4af5ab8e7a00ba0c92d2f2
Author: Lorenzo Bianconi <lorenzo@kernel.org>

selftests/bpf: Add test for bpf_skb_rx_checksum kfunc

This commit adds a selftest for the newly added bpf_skb_rx_checksum() kfunc.
The test consists of a TC BPF program that calls the kfunc and records the
checksum type, along with a userspace test runner.

> diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> index ae71e9b690518..d03572e26df2d 100644
> --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> @@ -64,6 +64,14 @@ struct bpf_tcp_req_attrs;
>  extern int bpf_sk_assign_tcp_reqsk(struct __sk_buff *skb, struct sock *sk,
>  				   struct bpf_tcp_req_attrs *attrs, int attrs__sz) __ksym;
>  
> +/* Description
> + *  Read skb RX checksum info (ip_summed and csum_meta).

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

> + * Returns
> + *  0 on success
> + */
> +extern int bpf_skb_rx_checksum(struct __sk_buff *skb, __u32 *ip_summed,
> +			       __u32 *csum_meta) __ksym;
> +

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/skb_rx_checksum.c b/tools/testing/selftests/bpf/progs/skb_rx_checksum.c
> new file mode 100644
> index 0000000000000..dc4e192905d53
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/skb_rx_checksum.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <linux/pkt_cls.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_kfuncs.h"
> +#include "skb_rx_checksum.h"
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, SKB_CSUM_PARTIAL);

[Severity: Medium]
Does this incorrectly restrict the map size? SKB_CSUM_PARTIAL is defined as
3, which means the map can only hold entries for 0, 1, and 2, leaving no
room to track the SKB_CSUM_PARTIAL state itself.

> +	__type(key, __u32);
> +	__type(value, __u64);
> +} csum_cnt SEC(".maps");
> +
> +SEC("tc")
> +int tc_rx_csum(struct __sk_buff *skb)
> +{
> +	enum skb_csum ip_summed;
> +	__u32 csum_meta;
> +	__u64 *cnt;
> +
> +	bpf_skb_rx_checksum(skb, &ip_summed, &csum_meta);

[Severity: Low]
Will this cause an incompatible pointer type compiler warning in
tc_rx_csum()? The kfunc bpf_skb_rx_checksum() expects a __u32 * for its
second argument, but &ip_summed is a pointer to an enum skb_csum.

> +	if (ip_summed < SKB_CSUM_PARTIAL) {

[Severity: Medium]
By using strictly less than SKB_CSUM_PARTIAL, does this silently drop valid
packets that have the SKB_CSUM_PARTIAL state?

> +		cnt = bpf_map_lookup_elem(&csum_cnt, &ip_summed);
> +		if (cnt)
> +			__sync_fetch_and_add(cnt, 1);
> +	}
> +
> +	return TC_ACT_OK;
> +}

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