netdev - Re: [PATCH net] xdp: use multi-buff only if receive queue supports page pool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aNZ33HRt+SxltbcP@boxer>
Date: Fri, 26 Sep 2025 13:24:12 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Octavian Purdila <tavip@...gle.com>
CC: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, <horms@...nel.org>,
	<ast@...nel.org>, <daniel@...earbox.net>, <hawk@...nel.org>,
	<john.fastabend@...il.com>, <sdf@...ichev.me>, <ahmed.zaki@...el.com>,
	<aleksander.lobakin@...el.com>, <toke@...hat.com>, <lorenzo@...nel.org>,
	<netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
	<syzbot+ff145014d6b0ce64a173@...kaller.appspotmail.com>, Kuniyuki Iwashima
	<kuniyu@...gle.com>
Subject: Re: [PATCH net] xdp: use multi-buff only if receive queue supports
 page pool

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@...nel.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@...nel.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.