Re: [PATCH net v2 2/2] veth: fix skb length accounting after XDP frag adjustment

On Mon, Aug 3, 2026 at 4:42 PM Lorenzo Bianconi <[email protected]> wrote:
>
> > On Sat, Aug 1, 2026 at 12:14 AM Mohsin Bashir <[email protected]> wrote:
> > >
> > >
> > >
> > > On 7/30/26 8:23 PM, Sun Jian wrote:
> > > > veth exposes non-linear skb fragments through an xdp_buff. If an XDP
> > > > program adjusts the fragment area, veth_xdp_rcv_skb() copies
> > > > xdp_frags_size back to skb->data_len but leaves skb->len containing the
> > > > old fragment contribution.
> > > >
> > > > After a fragment shrink, this makes skb_headlen() larger than the actual
> > > > linear area. In the reproduced UDP receive path, __skb_datagram_iter()
> > > > copied 1024 bytes past the actual linear tail to userspace, starting at
> > > > struct skb_shared_info. The copied bytes included the affected skb's
> > > > nr_frags, xdp_frags_size and a kernel pointer from
> > > > skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same
> > > > amount and truncated at the end.
> > > >
> > > > Subtract the old data_len before replacing it and add the new data_len
> > > > afterwards, keeping skb->len and skb->data_len synchronized.
> > > >
> > > > The fragment accounting must run before the linear tail adjustment:
> > > > when bpf_xdp_adjust_tail() shrinks the packet into the linear area it
> > > > releases all fragments, and __skb_put() requires skb->data_len == 0
> > > > by that point.
> > > >
> > > > A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by
> > > > 1024 bytes from its fragment area. Before the fix, all 10 runs produced
> > > > corrupted payloads. After the fix, all 10 runs matched the expected
> > > > payload exactly.
> > > >
> > > > Fixes: 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")
> > > > Cc: [email protected]
> > > > Link: https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/
> > > > Signed-off-by: Sun Jian <[email protected]>
> > > > ---
> > > >   drivers/net/veth.c | 23 +++++++++++++++--------
> > > >   1 file changed, 15 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > > > index 00e34afd858e..348391e87e14 100644
> > > > --- a/drivers/net/veth.c
> > > > +++ b/drivers/net/veth.c
> > > > @@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> > > >
> > > >       skb_reset_mac_header(skb);
> > > >
> > > > -     /* check if bpf_xdp_adjust_tail was used */
> > > > -     off = xdp->data_end - orig_data_end;
> > > > -     if (off != 0)
> > > > -             __skb_put(skb, off); /* positive on grow, negative on shrink */
> > > > -
> > > >       /* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
> > > > -      * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
> > > > +      * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
> > > > +      * from skb->len before updating data_len, then add the new one back.
> > > > +      * This must precede the linear tail adjustment below: a changed
> > > > +      * data_end implies that no fragments remain, and __skb_put() requires
> > > > +      * a linear skb.
> > > >        */
> > > > -     if (xdp_buff_has_frags(xdp))
> > > > +     skb->len -= skb->data_len;
> > > > +     if (xdp_buff_has_frags(xdp)) {
> > > >               skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> > > > -     else
> > > > +             skb->len += skb->data_len;
> > > > +     } else {
> > > >               skb->data_len = 0;
> > > > +     }
> > > > +
> > > > +     /* check if bpf_xdp_adjust_tail was used */
> > > > +     off = xdp->data_end - orig_data_end;
> > > > +     if (off != 0)
> > > > +             __skb_put(skb, off); /* positive on grow, negative on shrink */
> > > >
> > > >       skb->protocol = eth_type_trans(skb, rq->dev);
> > > >
> > >
> > > I am most likely missing something here but what happens if we have
> > > frags and we attempt to advance data_end while leaving some frags
> > > present (e.g., bpf_xdp_pull_data())? Looks like, in that case we would
> > > issue __skb_put(skb, off) with off > 0 and we would hit
> > > SKB_LINEAR_ASSERT() because skb is still non-linear?
> >
> > Hi Mohsin,
> >
> > Thanks for catching this. You're right that bpf_xdp_pull_data() can advance
> > data_end while leaving frags present. I'll investigate this path and follow up.
> >
> > Thanks,
> > Sun Jian
>
Hi Lorenzo,

> I guess we should implement the same approach used in
> bpf_prog_run_generic_xdp(), something like:
>
>         if (off) {
>                 skb_set_tail_pointer();
>                 skb->len += off;
>         }
Yes, that makes sense. I confirmed Mohsin's case: bpf_xdp_pull_data()
can advance data_end while keeping frags, so the current v2 can hit
SKB_LINEAR_ASSERT() through __skb_put() on a non-linear skb.

I tested the skb_set_tail_pointer() + skb->len update approach locally
with a forced-tailroom reproducer, and it fixes that path. I will fold
this into v3.

Thanks,
Sun Jian
>
> Regards,
> Lorenzo