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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Jul 20, 2026 at 11:24:47AM +0100, Matt Fleming wrote:
> 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.

Hi all,

I had an RFC that made veth to reuse generic XDP code path [0]. Jakub
commented we could have a common pp cow check on both sides. My plan is to
revive this work as this still causes page_pool issues when AF_XDP is used
on veth.

[0]: https://lore.kernel.org/bpf/20260509084858.773921-1-maciej.fijalkowski@intel.com/

Thanks,
Maciej

> 
> > >  	    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
>