Re: [PATCH net-next 01/15] bpf: Add gated tracepoints for skb life-cycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf: data race in bpf_trace_skb() corrupts SKB bitfields on shared SKBs
- [Medium] bpf: premature tracepoint emission in skb_release_head_state reports NOT_SPECIFIED instead of CONSUMED
- [Medium] bpf: skb_copy tracepoint exposes uninitialized slab memory to BPF programs
- [Low] build: build failure and tracepoint suppression due to improper CONFIG_SKB_EXTENSIONS nesting
--

commit 35624cd8e7b29d9f94cba0d236454331934ce45d
Author: Jakub Sitnicki <jakub@cloudflare.com>

    bpf: Add gated tracepoints for skb life-cycle

    BPF programs attached at different points in the network stack have no way
    to pass data between each other on a per-packet basis. xdp/skb->data_meta
    works for XDP-to-TC handoff, but is not available to programs running at
    later hooks like cgroup/skb, sock_ops, socket filters, tracing or LSM.
    
    Enable programs to keep per-packet metadata in a shared BPF map keyed by
    the skb address. What rides on the packet is only a marker: a new
    skb->traced flag, occupying the spare bit after csum_valid, which makes the
    packet's lifetime events observable.

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 84308498a3a8c..0353df4a46379 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1002,6 +1003,9 @@ struct sk_buff {
>  	__u8			encapsulation:1;
>  	__u8			encap_hdr_csum:1;
>  	__u8			csum_valid:1;
> +#ifdef CONFIG_SKB_GATED_TRACEPOINTS
> +	__u8			traced:1;
> +#endif

[Severity: High]
Does this new bitfield introduce a data race?

Since it is packed alongside other widely modified flags within the same byte,
unprotected modifications can lead to read-modify-write data races on cloned
or shared SKBs, potentially silently corrupting adjacent metadata flags like
csum_valid or encapsulation.

[ ... ]

> @@ -5147,6 +5183,8 @@ static inline void skb_ext_del(struct sk_buff *skb, int unused) {}
>  static inline void __skb_ext_copy(struct sk_buff *d, const struct sk_buff *s) {}
>  static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *s) {}
>  static inline bool skb_has_extensions(struct sk_buff *skb) { return false; }
> +static inline void skb_maybe_trace_free(struct sk_buff *skb,
> +					enum skb_drop_reason reason) {}
>  #endif /* CONFIG_SKB_EXTENSIONS */

[Severity: Low]
Are we missing the fallback declaration for skb_maybe_trace_copy() here?

The #else block for CONFIG_SKB_EXTENSIONS provides a stub for
skb_maybe_trace_free() but completely omits skb_maybe_trace_copy(). When
compiling with CONFIG_SKB_GATED_TRACEPOINTS=y and CONFIG_SKB_EXTENSIONS=n,
this will cause a build failure in __copy_skb_header() which calls it
unconditionally.

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..33c1ee3b966b0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12426,6 +12426,51 @@ __bpf_kfunc int bpf_dynptr_from_skb(struct __sk_buff *s, u64 flags,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SKB_GATED_TRACEPOINTS
> +/**
> + * bpf_trace_skb() - Opt an skb into gated lifetime tracepoints

[ ... ]

> +__bpf_kfunc int bpf_trace_skb(struct sk_buff *skb, u64 flags__k)
> +{
> +	if (flags__k)
> +		return -EINVAL;
> +
> +	if (!skb->traced)
> +		skb->traced = 1;
> +
> +	return 0;
> +}

[Severity: High]
Does this code corrupt SKB bitfields on shared SKBs?

The kfunc bpf_trace_skb() performs a non-atomic write on the shared bitfield
byte without verifying if the SKB is shared. Any BPF program that calls this
kfunc on a shared packet can concurrently modify the bitfield and corrupt
adjacent kernel memory flags.

[ ... ]

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8912a66cd9097..038854c83701a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1187,6 +1187,7 @@ void skb_release_head_state(struct sk_buff *skb)
>  	}
>  	nf_reset_ct(skb);
>  	skb_ext_reset(skb);
> +	skb_maybe_trace_free(skb, SKB_DROP_REASON_NOT_SPECIFIED);
>  }

[Severity: Medium]
Will this premature tracepoint emission cause us to miss the actual
consumption event?

When an SKB with head state is consumed via __consume_stateless_skb() (for
example, in udp_rcv()) or deferred via napi_consume_skb(),
skb_release_head_state() is called first.

This prematurely fires skb_maybe_trace_free() with NOT_SPECIFIED and clears
the skb->traced bit before the actual consumption happens. As a result, BPF
programs monitoring the skb_free tracepoint receive an incorrect drop reason
and the subsequent actual consume tracepoint fails to fire.

[ ... ]

> @@ -1601,6 +1607,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>  	CHECK_SKB_FIELD(tc_index);
>  #endif
>  
> +	skb_maybe_trace_copy(new, old);
>  }

[Severity: Medium]
Could this tracepoint expose uninitialized slab memory to BPF programs?

In __skb_clone(), __copy_skb_header() is called before critical structural
fields (like len, data_len, mac_len) are initialized:

__skb_clone()
   n->sk = NULL;
   __copy_skb_header(n, skb);
   C(len);
   C(data_len);

By unconditionally invoking skb_maybe_trace_copy() inside
__copy_skb_header(), the tracepoint passes the partially constructed clone
immediately to the BPF program, allowing it to observe uninitialized slab
memory from the new SKB.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-bpf-meta-gated-tracepoints-v1-0-844dbf3e1edf@cloudflare.com?part=1