Re: [PATCH bpf-next v6 3/5] bpf: Craft non-linear skbs in BPF_PROG_TEST_RUN

[Paul Chaignon <paul.chaignon@gmail.com>]

On Tue, Oct 07, 2025 at 10:59:32AM -0700, Martin KaFai Lau wrote:
> On 10/7/25 6:38 AM, Paul Chaignon wrote:
> > This patch adds support for crafting non-linear skbs in BPF test runs
> > for tc programs. The size of the linear area is given by ctx->data_end,
> > with a minimum of ETH_HLEN always pulled in the linear area. If ctx or
> > ctx->data_end are null, a linear skb is used.
> > 
> > This is particularly useful to test support for non-linear skbs in large
> > codebases such as Cilium. We've had multiple bugs in the past few years
> > where we were missing calls to bpf_skb_pull_data(). This support in
> > BPF_PROG_TEST_RUN would allow us to automatically cover this case in our
> > BPF tests.
> > 
> > In addition to the selftests introduced later in the series, this patch
> > was tested by setting enabling non-linear skbs for all tc selftests
> > programs and checking test failures were expected.
> > 

[...]

> >   	skb->sk = sk;
> >   	data = NULL; /* data released via kfree_skb */
> >   	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> > -	__skb_put(skb, size);
> > +	__skb_put(skb, linear_sz);
> > +
> > +	if (unlikely(kattr->test.data_size_in > linear_sz)) {
> > +		void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> > +		struct skb_shared_info *sinfo = skb_shinfo(skb);
> > +
> > +		size = linear_sz;
> 
> nit. I find the "size" variable confusing to follow. The "size" is
> overloaded with different meanings in this function.
> 
> Define a "u32 copied = linear_sz;" for this purpose (number of bytes copied
> so far) here.

Thanks for the review! I implemented this and all above suggestions.

[...]

> > -	/* bpf program can never convert linear skb to non-linear */
> > -	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
> > +	if (skb_is_nonlinear(skb)) {
> > +		/* bpf program can never convert linear skb to non-linear */
> > +		WARN_ON_ONCE(linear_sz == size);
> I don't think I understand this WARN. Do you mean "WARN_ON_ONCE(linear_sz ==
> kattr->test.data_size_in)" instead?

Yes. At that point in the code, size should be equal to
kattr->test.data_size_in, but using kattr->test.data_size_in directly is
clearer.

[...]

> > +	}
> >   	ret = bpf_test_finish(kattr, uattr, skb->data, NULL, size, retval,
> 
> What does it take to have bpf_test_finish support the skb's shinfo instead
> of passing NULL?

Not much actually. I've implemented it in the next iteration.

[...]