netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] xdp: use multi-buff only if receive queue supports page pool
@ 2025-09-24  6:08 Octavian Purdila
  2025-09-25  0:09 ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Octavian Purdila @ 2025-09-24  6:08 UTC (permalink / raw)
  To: kuba
  Cc: davem, edumazet, pabeni, horms, ast, daniel, hawk, john.fastabend,
	sdf, uniyu, ahmed.zaki, aleksander.lobakin, toke, lorenzo, netdev,
	bpf, Octavian Purdila, syzbot+ff145014d6b0ce64a173

When a BPF program that supports BPF_F_XDP_HAS_FRAGS is issuing
bpf_xdp_adjust_tail and a large packet is injected via /dev/net/tun a
crash occurs due to detecting a bad page state (page_pool leak).

This is because xdp_buff does not record the type of memory and
instead relies on the netdev receive queue xdp info. Since the TUN/TAP
driver is using a MEM_TYPE_PAGE_SHARED memory model buffer, shrinking
will eventually call page_frag_free. But with current multi-buff
support for BPF_F_XDP_HAS_FRAGS programs buffers are allocated via the
page pool.

To fix this issue check that the receive queue memory mode is of
MEM_TYPE_PAGE_POOL before using multi-buffs.

Reported-by: syzbot+ff145014d6b0ce64a173@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6756c37b.050a0220.a30f1.019a.GAE@google.com/
Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
Signed-off-by: Octavian Purdila <tavip@google.com>
---
 net/core/dev.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8d49b2198d07..b195ee3068c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5335,13 +5335,18 @@ static int
 netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog)
 {
 	struct sk_buff *skb = *pskb;
+	struct netdev_rx_queue *rxq;
 	int err, hroom, troom;
 
-	local_lock_nested_bh(&system_page_pool.bh_lock);
-	err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool), pskb, prog);
-	local_unlock_nested_bh(&system_page_pool.bh_lock);
-	if (!err)
-		return 0;
+	rxq = netif_get_rxqueue(skb);
+	if (rxq->xdp_rxq.mem.type == MEM_TYPE_PAGE_POOL) {
+		local_lock_nested_bh(&system_page_pool.bh_lock);
+		err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool),
+					   pskb, prog);
+		local_unlock_nested_bh(&system_page_pool.bh_lock);
+		if (!err)
+			return 0;
+	}
 
 	/* In case we have to go down the path and also linearize,
 	 * then lets do the pskb_expand_head() work just once here.
-- 
2.51.0.534.gc79095c0ca-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-24  6:08 [PATCH net] xdp: use multi-buff only if receive queue supports page pool Octavian Purdila
@ 2025-09-25  0:09 ` Jakub Kicinski
  2025-09-25  7:53   ` Octavian Purdila
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2025-09-25  0:09 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: davem, edumazet, pabeni, horms, ast, daniel, hawk, john.fastabend,
	sdf, uniyu, ahmed.zaki, aleksander.lobakin, toke, lorenzo, netdev,
	bpf, syzbot+ff145014d6b0ce64a173

On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:
> When a BPF program that supports BPF_F_XDP_HAS_FRAGS is issuing
> bpf_xdp_adjust_tail and a large packet is injected via /dev/net/tun a
> crash occurs due to detecting a bad page state (page_pool leak).
> 
> This is because xdp_buff does not record the type of memory and
> instead relies on the netdev receive queue xdp info. Since the TUN/TAP
> driver is using a MEM_TYPE_PAGE_SHARED memory model buffer, shrinking
> will eventually call page_frag_free. But with current multi-buff
> support for BPF_F_XDP_HAS_FRAGS programs buffers are allocated via the
> page pool.
> 
> To fix this issue check that the receive queue memory mode is of
> MEM_TYPE_PAGE_POOL before using multi-buffs.

This can also happen on veth, right? And veth re-stamps the Rx queues.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-25  0:09 ` Jakub Kicinski
@ 2025-09-25  7:53   ` Octavian Purdila
  2025-09-25  9:42     ` Maciej Fijalkowski
  0 siblings, 1 reply; 10+ messages in thread
From: Octavian Purdila @ 2025-09-25  7:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, edumazet, pabeni, horms, ast, daniel, hawk, john.fastabend,
	sdf, ahmed.zaki, aleksander.lobakin, toke, lorenzo, netdev, bpf,
	syzbot+ff145014d6b0ce64a173, Kuniyuki Iwashima

On Wed, Sep 24, 2025 at 5:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:
> > When a BPF program that supports BPF_F_XDP_HAS_FRAGS is issuing
> > bpf_xdp_adjust_tail and a large packet is injected via /dev/net/tun a
> > crash occurs due to detecting a bad page state (page_pool leak).
> >
> > This is because xdp_buff does not record the type of memory and
> > instead relies on the netdev receive queue xdp info. Since the TUN/TAP
> > driver is using a MEM_TYPE_PAGE_SHARED memory model buffer, shrinking
> > will eventually call page_frag_free. But with current multi-buff
> > support for BPF_F_XDP_HAS_FRAGS programs buffers are allocated via the
> > page pool.
> >
> > To fix this issue check that the receive queue memory mode is of
> > MEM_TYPE_PAGE_POOL before using multi-buffs.
>
> This can also happen on veth, right? And veth re-stamps the Rx queues.

I am not sure if re-stamps will have ill effects.

The allocation and deallocation for this issue happens while
processing the same packet (receive skb -> skb_pp_cow_data ->
page_pool alloc ... __bpf_prog_run ->  bpf_xdp_adjust_tail).

IIUC, if the veth re-stamps the RX queue to MEM_TYPE_PAGE_POOL
skb_pp_cow_data will proceed to allocate from page_pool and
bpf_xdp_adjust_tail will correctly free from page_pool.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-25  7:53   ` Octavian Purdila
@ 2025-09-25  9:42     ` Maciej Fijalkowski
  2025-09-26  2:12       ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej Fijalkowski @ 2025-09-25  9:42 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: Jakub Kicinski, davem, edumazet, pabeni, horms, ast, daniel, hawk,
	john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Thu, Sep 25, 2025 at 12:53:53AM -0700, Octavian Purdila wrote:
> On Wed, Sep 24, 2025 at 5:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:
> > > When a BPF program that supports BPF_F_XDP_HAS_FRAGS is issuing
> > > bpf_xdp_adjust_tail and a large packet is injected via /dev/net/tun a
> > > crash occurs due to detecting a bad page state (page_pool leak).
> > >
> > > This is because xdp_buff does not record the type of memory and
> > > instead relies on the netdev receive queue xdp info. Since the TUN/TAP
> > > driver is using a MEM_TYPE_PAGE_SHARED memory model buffer, shrinking
> > > will eventually call page_frag_free. But with current multi-buff
> > > support for BPF_F_XDP_HAS_FRAGS programs buffers are allocated via the
> > > page pool.
> > >
> > > To fix this issue check that the receive queue memory mode is of
> > > MEM_TYPE_PAGE_POOL before using multi-buffs.
> >
> > This can also happen on veth, right? And veth re-stamps the Rx queues.

What do you mean by 're-stamps' in this case?

> 
> I am not sure if re-stamps will have ill effects.
> 
> The allocation and deallocation for this issue happens while
> processing the same packet (receive skb -> skb_pp_cow_data ->
> page_pool alloc ... __bpf_prog_run ->  bpf_xdp_adjust_tail).
> 
> IIUC, if the veth re-stamps the RX queue to MEM_TYPE_PAGE_POOL
> skb_pp_cow_data will proceed to allocate from page_pool and
> bpf_xdp_adjust_tail will correctly free from page_pool.

netif_get_rxqueue() gives you a pointer the netstack queue, not the driver
one. Then you take the xdp_rxq from there. Do we even register memory
model for these queues? Or am I missing something here.

We're in generic XDP hook where driver specifics should not matter here
IMHO.

> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-25  9:42     ` Maciej Fijalkowski
@ 2025-09-26  2:12       ` Jakub Kicinski
  2025-09-26  7:33         ` Octavian Purdila
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2025-09-26  2:12 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: Octavian Purdila, davem, edumazet, pabeni, horms, ast, daniel,
	hawk, john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Thu, 25 Sep 2025 11:42:04 +0200 Maciej Fijalkowski wrote:
> On Thu, Sep 25, 2025 at 12:53:53AM -0700, Octavian Purdila wrote:
> > On Wed, Sep 24, 2025 at 5:09 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > >
> > > On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:  
>  [...]  
> > >
> > > This can also happen on veth, right? And veth re-stamps the Rx queues.  
> 
> What do you mean by 're-stamps' in this case?
> 
> > 
> > I am not sure if re-stamps will have ill effects.
> > 
> > The allocation and deallocation for this issue happens while
> > processing the same packet (receive skb -> skb_pp_cow_data ->
> > page_pool alloc ... __bpf_prog_run ->  bpf_xdp_adjust_tail).
> > 
> > IIUC, if the veth re-stamps the RX queue to MEM_TYPE_PAGE_POOL
> > skb_pp_cow_data will proceed to allocate from page_pool and
> > bpf_xdp_adjust_tail will correctly free from page_pool.  
> 
> netif_get_rxqueue() gives you a pointer the netstack queue, not the driver
> one. Then you take the xdp_rxq from there. Do we even register memory
> model for these queues? Or am I missing something here.
> 
> We're in generic XDP hook where driver specifics should not matter here
> IMHO.

Well, IDK how helpful the flow below would be but:

veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one() 
                                                               |
                            | xdp_convert_frame_to_buff()   <-'
    ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
  can eat frags but now rxq | bpf_prog_run_xdp()
         is veth's          |

I just glanced at the code so >50% changes I'm wrong, but that's what 
I meant.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-26  2:12       ` Jakub Kicinski
@ 2025-09-26  7:33         ` Octavian Purdila
  2025-09-26 11:24           ` Maciej Fijalkowski
  0 siblings, 1 reply; 10+ messages in thread
From: Octavian Purdila @ 2025-09-26  7:33 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Maciej Fijalkowski, davem, edumazet, pabeni, horms, ast, daniel,
	hawk, john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Thu, Sep 25, 2025 at 7:12 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 25 Sep 2025 11:42:04 +0200 Maciej Fijalkowski wrote:
> > On Thu, Sep 25, 2025 at 12:53:53AM -0700, Octavian Purdila wrote:
> > > On Wed, Sep 24, 2025 at 5:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > >
> > > > On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:
> >  [...]
> > > >
> > > > This can also happen on veth, right? And veth re-stamps the Rx queues.
> >
> > What do you mean by 're-stamps' in this case?
> >
> > >
> > > I am not sure if re-stamps will have ill effects.
> > >
> > > The allocation and deallocation for this issue happens while
> > > processing the same packet (receive skb -> skb_pp_cow_data ->
> > > page_pool alloc ... __bpf_prog_run ->  bpf_xdp_adjust_tail).
> > >
> > > IIUC, if the veth re-stamps the RX queue to MEM_TYPE_PAGE_POOL
> > > skb_pp_cow_data will proceed to allocate from page_pool and
> > > bpf_xdp_adjust_tail will correctly free from page_pool.
> >
> > netif_get_rxqueue() gives you a pointer the netstack queue, not the driver
> > one. Then you take the xdp_rxq from there. Do we even register memory
> > model for these queues? Or am I missing something here.
> >

Ah, yes, you are right. So my comment in the commit message about
TUN/TAP registering a page shared memory model is wrong. But I think
the fix is still correct for the reported syzkaller issue. From
bpf_prog_run_generic_xdp:

        rxqueue = netif_get_rxqueue(skb);
        xdp_init_buff(xdp, frame_sz, rxq: &rxqueue->xdp_rxq);

So xdp_buff's rxq is set to the netstack queue for the generic XDP
hook. And adding the check in netif_skb_check_for_xdp based on the
netstack queue should be correct, right?

> > We're in generic XDP hook where driver specifics should not matter here
> > IMHO.
>
> Well, IDK how helpful the flow below would be but:
>
> veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one()
>                                                                |
>                             | xdp_convert_frame_to_buff()   <-'
>     ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
>   can eat frags but now rxq | bpf_prog_run_xdp()
>          is veth's          |
>
> I just glanced at the code so >50% changes I'm wrong, but that's what
> I meant.

Thanks for the clarification, I thought that "re-stamps" means the:

    xdp->rxq->mem.type = frame->mem_type;

from veth_xdp_rcv_one in the XDP_TX/XDP_REDIRECT cases.

And yes, now I think the same issue can happen because veth sets the
memory model to MEM_TYPE_PAGE_SHARED but veth_convert_skb_to_xdp_buff
calls skb_pp_cow_data that uses page_pool for allocations. I'll try to
see if I can adapt the syzkaller repro to trigger it for confirmation.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-26  7:33         ` Octavian Purdila
@ 2025-09-26 11:24           ` Maciej Fijalkowski
  2025-09-26 19:40             ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej Fijalkowski @ 2025-09-26 11:24 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: Jakub Kicinski, davem, edumazet, pabeni, horms, ast, daniel, hawk,
	john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Fri, Sep 26, 2025 at 12:33:46AM -0700, Octavian Purdila wrote:
> On Thu, Sep 25, 2025 at 7:12 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 25 Sep 2025 11:42:04 +0200 Maciej Fijalkowski wrote:
> > > On Thu, Sep 25, 2025 at 12:53:53AM -0700, Octavian Purdila wrote:
> > > > On Wed, Sep 24, 2025 at 5:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > >
> > > > > On Wed, 24 Sep 2025 06:08:42 +0000 Octavian Purdila wrote:
> > >  [...]
> > > > >
> > > > > This can also happen on veth, right? And veth re-stamps the Rx queues.
> > >
> > > What do you mean by 're-stamps' in this case?
> > >
> > > >
> > > > I am not sure if re-stamps will have ill effects.
> > > >
> > > > The allocation and deallocation for this issue happens while
> > > > processing the same packet (receive skb -> skb_pp_cow_data ->
> > > > page_pool alloc ... __bpf_prog_run ->  bpf_xdp_adjust_tail).
> > > >
> > > > IIUC, if the veth re-stamps the RX queue to MEM_TYPE_PAGE_POOL
> > > > skb_pp_cow_data will proceed to allocate from page_pool and
> > > > bpf_xdp_adjust_tail will correctly free from page_pool.
> > >
> > > netif_get_rxqueue() gives you a pointer the netstack queue, not the driver
> > > one. Then you take the xdp_rxq from there. Do we even register memory
> > > model for these queues? Or am I missing something here.
> > >
> 
> Ah, yes, you are right. So my comment in the commit message about
> TUN/TAP registering a page shared memory model is wrong. But I think
> the fix is still correct for the reported syzkaller issue. From
> bpf_prog_run_generic_xdp:
> 
>         rxqueue = netif_get_rxqueue(skb);
>         xdp_init_buff(xdp, frame_sz, rxq: &rxqueue->xdp_rxq);
> 
> So xdp_buff's rxq is set to the netstack queue for the generic XDP
> hook. And adding the check in netif_skb_check_for_xdp based on the
> netstack queue should be correct, right?

Per my limited understanding your change is making skb_cow_data_for_xdp()
a dead code as I don't see mem model being registered for these stack
queues - netif_alloc_rx_queues() only calls xdp_rxq_info_reg() and
mem.type defaults to MEM_TYPE_PAGE_SHARED as it's defined as 0, which
means it's never going to be MEM_TYPE_PAGE_POOL.

IMHO that single case where we rewrite skb to memory backed by page pool
should have it reflected in mem.type so __xdp_return() potentially used in
bpf helpers could act correctly.

> 
> > > We're in generic XDP hook where driver specifics should not matter here
> > > IMHO.
> >
> > Well, IDK how helpful the flow below would be but:
> >
> > veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one()
> >                                                                |
> >                             | xdp_convert_frame_to_buff()   <-'
> >     ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
> >   can eat frags but now rxq | bpf_prog_run_xdp()
> >          is veth's          |
> >
> > I just glanced at the code so >50% changes I'm wrong, but that's what
> > I meant.
> 
> Thanks for the clarification, I thought that "re-stamps" means the:
> 
>     xdp->rxq->mem.type = frame->mem_type;
> 
> from veth_xdp_rcv_one in the XDP_TX/XDP_REDIRECT cases.
> 
> And yes, now I think the same issue can happen because veth sets the
> memory model to MEM_TYPE_PAGE_SHARED but veth_convert_skb_to_xdp_buff
> calls skb_pp_cow_data that uses page_pool for allocations. I'll try to
> see if I can adapt the syzkaller repro to trigger it for confirmation.

That is a good catch.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-26 11:24           ` Maciej Fijalkowski
@ 2025-09-26 19:40             ` Jakub Kicinski
  2025-09-30  0:01               ` Octavian Purdila
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2025-09-26 19:40 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: Octavian Purdila, davem, edumazet, pabeni, horms, ast, daniel,
	hawk, john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Fri, 26 Sep 2025 13:24:12 +0200 Maciej Fijalkowski wrote:
> On Fri, Sep 26, 2025 at 12:33:46AM -0700, Octavian Purdila wrote:
> > On Thu, Sep 25, 2025 at 7:12 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > Ah, yes, you are right. So my comment in the commit message about
> > TUN/TAP registering a page shared memory model is wrong. But I think
> > the fix is still correct for the reported syzkaller issue. From
> > bpf_prog_run_generic_xdp:
> > 
> >         rxqueue = netif_get_rxqueue(skb);
> >         xdp_init_buff(xdp, frame_sz, rxq: &rxqueue->xdp_rxq);
> > 
> > So xdp_buff's rxq is set to the netstack queue for the generic XDP
> > hook. And adding the check in netif_skb_check_for_xdp based on the
> > netstack queue should be correct, right?  
> 
> Per my limited understanding your change is making skb_cow_data_for_xdp()
> a dead code as I don't see mem model being registered for these stack
> queues - netif_alloc_rx_queues() only calls xdp_rxq_info_reg() and
> mem.type defaults to MEM_TYPE_PAGE_SHARED as it's defined as 0, which
> means it's never going to be MEM_TYPE_PAGE_POOL.

Hah, that's a great catch, how did I miss that..

The reason for the cow is that frags can be shared, we are not allowed
to modify them. It's orthogonal.

> IMHO that single case where we rewrite skb to memory backed by page pool
> should have it reflected in mem.type so __xdp_return() potentially used in
> bpf helpers could act correctly.
> 
> > > Well, IDK how helpful the flow below would be but:
> > >
> > > veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one()
> > >                                                                |
> > >                             | xdp_convert_frame_to_buff()   <-'
> > >     ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
> > >   can eat frags but now rxq | bpf_prog_run_xdp()
> > >          is veth's          |
> > >
> > > I just glanced at the code so >50% changes I'm wrong, but that's what
> > > I meant.  
> > 
> > Thanks for the clarification, I thought that "re-stamps" means the:
> > 
> >     xdp->rxq->mem.type = frame->mem_type;
> > 
> > from veth_xdp_rcv_one in the XDP_TX/XDP_REDIRECT cases.
> > 
> > And yes, now I think the same issue can happen because veth sets the
> > memory model to MEM_TYPE_PAGE_SHARED but veth_convert_skb_to_xdp_buff
> > calls skb_pp_cow_data that uses page_pool for allocations. I'll try to
> > see if I can adapt the syzkaller repro to trigger it for confirmation.  
> 
> That is a good catch.

FWIW I think all calls to xdp_convert_frame_to_buff() must come with
the hack that cpu_map_bpf_prog_run_xdp() is doing today. Declare rxq
on the stack and fill in the mem info there. I wonder if we should add
something to the API (xdp_convert_frame_to_buff()) to make sure people
don't forget to do this..

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-26 19:40             ` Jakub Kicinski
@ 2025-09-30  0:01               ` Octavian Purdila
  2025-09-30 17:41                 ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Octavian Purdila @ 2025-09-30  0:01 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Maciej Fijalkowski, davem, edumazet, pabeni, horms, ast, daniel,
	hawk, john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Fri, Sep 26, 2025 at 12:40 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 26 Sep 2025 13:24:12 +0200 Maciej Fijalkowski wrote:
> > On Fri, Sep 26, 2025 at 12:33:46AM -0700, Octavian Purdila wrote:
> > > On Thu, Sep 25, 2025 at 7:12 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > Ah, yes, you are right. So my comment in the commit message about
> > > TUN/TAP registering a page shared memory model is wrong. But I think
> > > the fix is still correct for the reported syzkaller issue. From
> > > bpf_prog_run_generic_xdp:
> > >
> > >         rxqueue = netif_get_rxqueue(skb);
> > >         xdp_init_buff(xdp, frame_sz, rxq: &rxqueue->xdp_rxq);
> > >
> > > So xdp_buff's rxq is set to the netstack queue for the generic XDP
> > > hook. And adding the check in netif_skb_check_for_xdp based on the
> > > netstack queue should be correct, right?
> >
> > Per my limited understanding your change is making skb_cow_data_for_xdp()
> > a dead code as I don't see mem model being registered for these stack
> > queues - netif_alloc_rx_queues() only calls xdp_rxq_info_reg() and
> > mem.type defaults to MEM_TYPE_PAGE_SHARED as it's defined as 0, which
> > means it's never going to be MEM_TYPE_PAGE_POOL.
>
> Hah, that's a great catch, how did I miss that..
>
> The reason for the cow is that frags can be shared, we are not allowed
> to modify them. It's orthogonal.
>

Could we use the same hack that you mention below (declare rxq on the
stack and fill in the mem info there) for do_xdp_generic?

@@ -5442,7 +5448,10 @@ int do_xdp_generic(const struct bpf_prog
*xdp_prog, struct sk_buff **pskb)
        struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;

        if (xdp_prog) {
-               struct xdp_buff xdp;
+               struct xdp_rxq_info rxq = {};
+               struct xdp_buff xdp = {
+                       .rxq = &rxq,
+               };
                u32 act;
                int err;

and then explicitly set the mem type:

@@ -5331,14 +5332,19 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff
*skb, struct xdp_buff *xdp,
        return act;
 }

-static int
-netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog)
+static int netif_skb_check_for_xdp(struct sk_buff **pskb,
+                                  const struct bpf_prog *prog,
+                                  struct xdp_rxq_info *rxq)
 {
        struct sk_buff *skb = *pskb;
        int err, hroom, troom;
+       struct page_pool *pool;

        local_lock_nested_bh(&system_page_pool.bh_lock);
-       err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool),
pskb, prog);
+       pool = this_cpu_read(system_page_pool.pool);
+       err = skb_cow_data_for_xdp(pool, pskb, prog);
+       rxq->mem.type = MEM_TYPE_PAGE_POOL;
+       rxq->mem.id = pool->xdp_mem_id;
        local_unlock_nested_bh(&system_page_pool.bh_lock);
        if (!err)
                return 0;


> > IMHO that single case where we rewrite skb to memory backed by page pool
> > should have it reflected in mem.type so __xdp_return() potentially used in
> > bpf helpers could act correctly.
> >
> > > > Well, IDK how helpful the flow below would be but:
> > > >
> > > > veth_xdp_xmit() -> [ptr ring] -> veth_xdp_rcv() -> veth_xdp_rcv_one()
> > > >                                                                |
> > > >                             | xdp_convert_frame_to_buff()   <-'
> > > >     ( "re-stamps" ;) ->     | xdp->rxq = &rq->xdp_rxq;
> > > >   can eat frags but now rxq | bpf_prog_run_xdp()
> > > >          is veth's          |
> > > >
> > > > I just glanced at the code so >50% changes I'm wrong, but that's what
> > > > I meant.
> > >
> > > Thanks for the clarification, I thought that "re-stamps" means the:
> > >
> > >     xdp->rxq->mem.type = frame->mem_type;
> > >
> > > from veth_xdp_rcv_one in the XDP_TX/XDP_REDIRECT cases.
> > >
> > > And yes, now I think the same issue can happen because veth sets the
> > > memory model to MEM_TYPE_PAGE_SHARED but veth_convert_skb_to_xdp_buff
> > > calls skb_pp_cow_data that uses page_pool for allocations. I'll try to
> > > see if I can adapt the syzkaller repro to trigger it for confirmation.
> >
> > That is a good catch.
>
> FWIW I think all calls to xdp_convert_frame_to_buff() must come with
> the hack that cpu_map_bpf_prog_run_xdp() is doing today. Declare rxq
> on the stack and fill in the mem info there. I wonder if we should add
> something to the API (xdp_convert_frame_to_buff()) to make sure people
> don't forget to do this..

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool
  2025-09-30  0:01               ` Octavian Purdila
@ 2025-09-30 17:41                 ` Jakub Kicinski
  0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2025-09-30 17:41 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: Maciej Fijalkowski, davem, edumazet, pabeni, horms, ast, daniel,
	hawk, john.fastabend, sdf, ahmed.zaki, aleksander.lobakin, toke,
	lorenzo, netdev, bpf, syzbot+ff145014d6b0ce64a173,
	Kuniyuki Iwashima

On Mon, 29 Sep 2025 17:01:49 -0700 Octavian Purdila wrote:
>         local_lock_nested_bh(&system_page_pool.bh_lock);
> -       err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool), pskb, prog);
> +       pool = this_cpu_read(system_page_pool.pool);
> +       err = skb_cow_data_for_xdp(pool, pskb, prog);
> +       rxq->mem.type = MEM_TYPE_PAGE_POOL;
> +       rxq->mem.id = pool->xdp_mem_id;
>         local_unlock_nested_bh(&system_page_pool.bh_lock);

Yes, LGTM. I _think_ that only skb_cow_data_for_xdp() has to be under
the lock here, but doesn't really matter.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-09-30 17:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-24  6:08 [PATCH net] xdp: use multi-buff only if receive queue supports page pool Octavian Purdila
2025-09-25  0:09 ` Jakub Kicinski
2025-09-25  7:53   ` Octavian Purdila
2025-09-25  9:42     ` Maciej Fijalkowski
2025-09-26  2:12       ` Jakub Kicinski
2025-09-26  7:33         ` Octavian Purdila
2025-09-26 11:24           ` Maciej Fijalkowski
2025-09-26 19:40             ` Jakub Kicinski
2025-09-30  0:01               ` Octavian Purdila
2025-09-30 17:41                 ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).