Re: [PATCH bpf-next v2 2/4] bpf: Craft non-linear skbs in BPF_PROG_TEST_RUN

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

Thanks for the review Amery!

On Mon, Sep 15, 2025 at 05:27:05PM -0700, Amery Hung wrote:
> On Sun, Sep 14, 2025 at 8:10 AM Paul Chaignon <paul.chaignon@gmail.com> wrote:

[...]

> >  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > -                          u32 size, u32 headroom, u32 tailroom)
> > +                          u32 size, u32 headroom, u32 tailroom, bool nonlinear)
> >  {
> >         void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> > -       void *data;
> > +       void *data, *dst;
> >
> >         if (user_size < ETH_HLEN || user_size > PAGE_SIZE - headroom - tailroom)
> >                 return ERR_PTR(-EINVAL);
> >
> > -       size = SKB_DATA_ALIGN(size);
> > -       data = kzalloc(size + headroom + tailroom, GFP_USER);
> > +       /* In non-linear case, data_in is copied to the paged data */
> > +       if (nonlinear) {
> > +               data = alloc_page(GFP_USER);
> 
> Do we need more pages here for non-linear data larger than a page?

We're limiting user_size above to be at most
PAGE_SIZE-headroom-tailroom, so I don't think we support more than a
page of data. Am I missing something?

> 
> > +       } else {
> > +               size = SKB_DATA_ALIGN(size);
> > +               data = kzalloc(size + headroom + tailroom, GFP_USER);
> > +       }
> >         if (!data)
> >                 return ERR_PTR(-ENOMEM);
> >
> > -       if (copy_from_user(data + headroom, data_in, user_size)) {
> > +       if (nonlinear)
> > +               dst = page_address(data);
> > +       else
> > +               dst = data + headroom;
> > +       if (copy_from_user(dst, data_in, user_size)) {
> >                 kfree(data);
> 
> syzbot reported a bug. It seems like data allocated through
> alloc_page() got freed by kfree() here.

Yep, I've fixed it and it will be in the v3.

[...]

> > @@ -1029,6 +1033,27 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >                 break;
> >         }
> >
> > +       if (is_nonlinear && !is_l2)
> > +               return -EINVAL;
> > +
> > +       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)),
> > +                            is_nonlinear);
> > +       if (IS_ERR(data))
> > +               return PTR_ERR(data);
> > +
> > +       ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> > +       if (IS_ERR(ctx)) {
> > +               ret = PTR_ERR(ctx);
> > +               ctx = NULL;
> > +               goto out;
> > +       }
> > +
> > +       linear_size = hh_len;
> > +       if (is_nonlinear && ctx && ctx->data_end > linear_size)
> > +               linear_size = ctx->data_end;
> 
> I think BPF_F_TEST_SKB_NON_LINEAR may not be necessary.
> 
> To not break backward compatibility (assuming existing users most
> likely zero initialized ctx), when ctx->data_end == 0 || ctx->data_end
> == data_size_in, allocate a linear skb as it used to be. Then, if
> ctx->data_end < data_size_in, allocate a non-linear skb.
> 
> WDYT?

That makes sense, if only to be consistent with your patchset. It should
be doable by just calling bpf_ctx_init before bpf_test_init. I'll try
that.

> 
> > +
> >         sk = sk_alloc(net, AF_UNSPEC, GFP_USER, &bpf_dummy_proto, 1);
> >         if (!sk) {
> >                 ret = -ENOMEM;
> > @@ -1036,15 +1061,32 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >         }
> >         sock_init_data(NULL, sk);
> >
> > -       skb = slab_build_skb(data);
> > +       if (is_nonlinear)
> > +               skb = alloc_skb(NET_SKB_PAD + NET_IP_ALIGN + size +
> > +                               SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > +                               GFP_USER);
> > +       else
> > +               skb = slab_build_skb(data);
> >         if (!skb) {
> >                 ret = -ENOMEM;
> >                 goto out;
> >         }
> > +
> >         skb->sk = sk;
> >
> >         skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> > -       __skb_put(skb, size);
> > +
> > +       if (is_nonlinear) {
> > +               skb_fill_page_desc(skb, 0, data, 0, size);
> > +               skb->truesize += PAGE_SIZE;
> > +               skb->data_len = size;
> > +               skb->len = size;
> 
> Do we need to update skb_shared_info?

skb_fill_page_desc() already does, at least the skb_shinfo(skb)->frags[0].
Do you have something else in mind?

[...]