Re: [PATCH bpf] veth: convert frag_list skbs before running XDP

[Matt Fleming <matt@readmodwrite.com>]

On Fri, Jul 17, 2026 at 11:56:41AM +0200, Toke Høiland-Jørgensen wrote:
> Matt Fleming <matt@readmodwrite.com> writes:
> 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index 1c5142149175..efb24aae1f26 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -756,7 +756,7 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
> >  	u32 frame_sz;
> >  
> >  	if (skb_shared(skb) || skb_head_is_locked(skb) ||
> > -	    skb_shinfo(skb)->nr_frags ||
> > +	    skb_shinfo(skb)->nr_frags || skb_has_frag_list(skb) ||
> 
> Isn't 'skb_shinfo(skb)->nr_frags || skb_has_frag_list(skb)' basically
> the same as 'skb_is_nonlinear(skb)'? Which, incidentally, is what
> generic XDP uses in the check that guards calling into the
> skb_pp_cow_data() path.
 
Yeah, you're right. I tested that expression and it still fixes the
bug. I'll update v2 to use skb_is_nonlinear().

> Looking at those two places, generic XDP checks for 'skb_cloned(skb)',
> while veth checks 'skb_shared(skb) || skb_head_is_locked(skb)'. AFAICT,
> the latter is stricter; should we update the generic XDP check?
 
Possibly, but the surrounding code isn't set up to deal with
skb_shared() SKBs so that'd be a larger change. I can take a look at
that too but I'm going to need more time to get my head around making
that change correctly given that there's different fallback rules than
veth.

> >  	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> >  		if (skb_pp_cow_data(rq->page_pool, pskb, XDP_PACKET_HEADROOM))
> >  			goto drop;
> > @@ -771,7 +771,7 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
> >  	xdp_prepare_buff(xdp, skb->head, skb_headroom(skb),
> >  			 skb_headlen(skb), true);
> >  
> > -	if (skb_is_nonlinear(skb)) {
> > +	if (skb_shinfo(skb)->nr_frags) {
> >  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
> >  		xdp_buff_set_frags_flag(xdp);
> >  	} else {
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 18dabb4e9cfa..1e837d01a908 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -936,12 +936,11 @@ int skb_pp_cow_data(struct page_pool *pool, struct sk_buff **pskb,
> >  	int err, i, head_off;
> >  	void *data;
> >  
> > -	/* XDP does not support fraglist so we need to linearize
> > -	 * the skb.
> > +	/*
> > +	 * skb_copy_bits() handles both frags[] and frag_list input. If the
> > +	 * copied skb remains non-linear, it uses frags[], which is the
> > +	 * representation used by XDP multi-buffer.
> >  	 */
> 
> This comment sorta reads like a function documentation comment, but it
> ends up sitting weirdly in the middle of the function body. The comment
> you're replacing was tied to the statement below, but this one isn't,
> really. Should we turn it into an actual function doc comment instead?

Good point. I'll make this a function doc comment.

Thanks,
Matt