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

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

[Matt Fleming]

From: Matt Fleming <mfleming@cloudflare.com>

A frag_list skb can reach veth with data_len set but nr_frags zero.
veth_convert_skb_to_xdp_buff() only converts skbs that are shared,
locked, have frags[], or do not have enough headroom. It later uses
skb_is_nonlinear() to decide whether to set XDP_FLAGS_HAS_FRAGS and
xdp_frags_size.

That exposes frag_list data to XDP as if it were stored in frags[], but
frags[] is empty. AF_XDP copy mode can then trust the bogus XDP fragment
metadata, walk an empty fragment entry, and crash in memcpy() from
__xsk_rcv().

Route frag_list skbs through skb_pp_cow_data() before exposing them to
XDP, and only advertise XDP frags when the resulting skb has frags[].
skb_copy_bits() already handles frag_list input, and skb_pp_cow_data()
builds frags[] output with skb_add_rx_frag(), which is the representation
XDP multi-buffer expects.

Fixes: 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")
Cc: stable@vger.kernel.org
Signed-off-by: Matt Fleming <mfleming@cloudflare.com>
---
 drivers/net/veth.c | 4 ++--
 net/core/skbuff.c  | 9 ++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

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) ||
 	    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.
 	 */
-	if (skb_has_frag_list(skb))
-		return -EOPNOTSUPP;
-
 	max_head_size = SKB_WITH_OVERHEAD(PAGE_SIZE - headroom);
 	if (skb->len > max_head_size + MAX_SKB_FRAGS * PAGE_SIZE)
 		return -ENOMEM;
-- 
2.43.0

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

[Toke Høiland-Jørgensen]

Matt Fleming <matt@readmodwrite.com> writes:

> From: Matt Fleming <mfleming@cloudflare.com>
>
> A frag_list skb can reach veth with data_len set but nr_frags zero.
> veth_convert_skb_to_xdp_buff() only converts skbs that are shared,
> locked, have frags[], or do not have enough headroom. It later uses
> skb_is_nonlinear() to decide whether to set XDP_FLAGS_HAS_FRAGS and
> xdp_frags_size.
>
> That exposes frag_list data to XDP as if it were stored in frags[], but
> frags[] is empty. AF_XDP copy mode can then trust the bogus XDP fragment
> metadata, walk an empty fragment entry, and crash in memcpy() from
> __xsk_rcv().
>
> Route frag_list skbs through skb_pp_cow_data() before exposing them to
> XDP, and only advertise XDP frags when the resulting skb has frags[].
> skb_copy_bits() already handles frag_list input, and skb_pp_cow_data()
> builds frags[] output with skb_add_rx_frag(), which is the representation
> XDP multi-buffer expects.
>
> Fixes: 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")
> Cc: stable@vger.kernel.org
> Signed-off-by: Matt Fleming <mfleming@cloudflare.com>

Seems reasonable, but a few nits below:

> ---
>  drivers/net/veth.c | 4 ++--
>  net/core/skbuff.c  | 9 ++++-----
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> 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.

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?

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

-Toke

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

[Matt Fleming]

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

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

[Maciej Fijalkowski]

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
> 

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

[Matt Fleming]

On Tue, Jul 21, 2026 at 12:58:59PM +0200, Maciej Fijalkowski wrote:
> 
> 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/

That sounds great. I'd be happy to test out the next version.

Thanks,
Matt