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

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

On Thu, Oct 02, 2025 at 11:27:52AM -0700, Martin KaFai Lau wrote:
> On 10/2/25 3:07 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.
> > 
> > Tested-by: syzbot@syzkaller.appspotmail.com
> > Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> > Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> > ---
> >   net/bpf/test_run.c | 67 +++++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 61 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index 3425100b1e8c..e4f4b423646a 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -910,6 +910,12 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
> >   	/* cb is allowed */
> >   	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, cb),
> > +			   offsetof(struct __sk_buff, data_end)))
> > +		return -EINVAL;
> > +
> > +	/* data_end is allowed, but not copied to skb */
> > +
> > +	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, data_end),
> >   			   offsetof(struct __sk_buff, tstamp)))
> >   		return -EINVAL;
> > @@ -984,9 +990,12 @@ static struct proto bpf_dummy_proto = {
> >   int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >   			  union bpf_attr __user *uattr)
> >   {
> > +	u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >   	bool is_l2 = false, is_direct_pkt_access = false;
> >   	struct net *net = current->nsproxy->net_ns;
> >   	struct net_device *dev = net->loopback_dev;
> > +	u32 headroom = NET_SKB_PAD + NET_IP_ALIGN;
> > +	u32 linear_sz = kattr->test.data_size_in;
> >   	u32 size = kattr->test.data_size_in;
> >   	u32 repeat = kattr->test.repeat;
> >   	struct __sk_buff *ctx = NULL;
> > @@ -1023,9 +1032,16 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >   	if (IS_ERR(ctx))
> >   		return PTR_ERR(ctx);
> > -	data = bpf_test_init(kattr, kattr->test.data_size_in,
> > -			     size, NET_SKB_PAD + NET_IP_ALIGN,
> > -			     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> > +	if (ctx) {
> > +		if (!is_l2 || ctx->data_end > kattr->test.data_size_in) {
> 
> What is the need for the "!is_l2" test?

There's nothing limiting us to only tc program types, but I was
wondering if it makes sense to open this (non-linear skbs) to all
program types. For example, cgroup_skb programs cannot call
bpf_skb_pull_data to deal with non-linear skbs.

Even the LWT program types would require special care because ex. the
bpf_clone_redirect helper can end up calling eth_type_trans which
assumes we have at least ETH_HLEN in the linear area. I wasn't sure it
was worth opening this capability to these program types without a clear
use case.

> 
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		if (ctx->data_end)
> > +			linear_sz = max(ETH_HLEN, ctx->data_end);
> > +	}
> > +
> > +	data = bpf_test_init(kattr, linear_sz, size, headroom, tailroom);
> 
> Instead of passing "size", should linear_sz be passed instead? Unlike xdp,
> allocating exactly linear_sz should be enough considering bpf_skb_pull_data
> can allocate new data if needed.

Indeed. Thanks!

> 
> Should linear_sz be limited to "PAGE_SIZE - headroom..." like how
> test_run_xdp() does it ?

That changes a bit the current behavior. Currently, we will return
EINVAL if a user try to pass more than "PAGE_SIZE - headroom..." as
data_size_in. With the test_run_xdp approach, we'll end up silently
switching to non-linear mode if they do that.

I'm not against it given it brings consistency with the XDP counterpart,
but it could also be a bit surprising.

[...]