netdev - Re: [RFC v4 03/18] page_pool: use netmem alloc/put APIs in __page_pool_alloc_page

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHS8izND_JonvNqJm4XpXm-sk9+v6KCGqeKb7ZUSAWoyckUY6A@mail.gmail.com>
Date: Thu, 5 Jun 2025 13:34:25 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Byungchul Park <byungchul@...com>, willy@...radead.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, kernel_team@...ynix.com, 
	kuba@...nel.org, ilias.apalodimas@...aro.org, harry.yoo@...cle.com, 
	hawk@...nel.org, akpm@...ux-foundation.org, davem@...emloft.net, 
	john.fastabend@...il.com, andrew+netdev@...n.ch, toke@...hat.com, 
	tariqt@...dia.com, edumazet@...gle.com, pabeni@...hat.com, saeedm@...dia.com, 
	leon@...nel.org, ast@...nel.org, daniel@...earbox.net, david@...hat.com, 
	lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, vbabka@...e.cz, 
	rppt@...nel.org, surenb@...gle.com, mhocko@...e.com, horms@...nel.org, 
	linux-rdma@...r.kernel.org, bpf@...r.kernel.org, vishal.moola@...il.com
Subject: Re: [RFC v4 03/18] page_pool: use netmem alloc/put APIs in __page_pool_alloc_page_order()

On Thu, Jun 5, 2025 at 1:26 PM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> On 6/5/25 20:39, Mina Almasry wrote:
> > On Thu, Jun 5, 2025 at 3:25 AM Pavel Begunkov <asml.silence@...il.com> wrote:
> >>
> >> On 6/4/25 03:52, Byungchul Park wrote:
> >>> Use netmem alloc/put APIs instead of page alloc/put APIs and make it
> >>> return netmem_ref instead of struct page * in
> >>> __page_pool_alloc_page_order().
> >>>
> >>> Signed-off-by: Byungchul Park <byungchul@...com>
> >>> Reviewed-by: Mina Almasry <almasrymina@...gle.com>
> >>> ---
> >>>    net/core/page_pool.c | 26 +++++++++++++-------------
> >>>    1 file changed, 13 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >>> index 4011eb305cee..523354f2db1c 100644
> >>> --- a/net/core/page_pool.c
> >>> +++ b/net/core/page_pool.c
> >>> @@ -518,29 +518,29 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t g
> >>>        return false;
> >>>    }
> >>>
> >>> -static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
> >>> -                                              gfp_t gfp)
> >>> +static netmem_ref __page_pool_alloc_page_order(struct page_pool *pool,
> >>> +                                            gfp_t gfp)
> >>>    {
> >>> -     struct page *page;
> >>> +     netmem_ref netmem;
> >>>
> >>>        gfp |= __GFP_COMP;
> >>> -     page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
> >>> -     if (unlikely(!page))
> >>> -             return NULL;
> >>> +     netmem = alloc_netmems_node(pool->p.nid, gfp, pool->p.order);
> >>> +     if (unlikely(!netmem))
> >>> +             return 0;
> >>>
> >>> -     if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page), gfp))) {
> >>> -             put_page(page);
> >>> -             return NULL;
> >>> +     if (pool->dma_map && unlikely(!page_pool_dma_map(pool, netmem, gfp))) {
> >>> +             put_netmem(netmem);
> >>
> >> It's a bad idea to have {put,get}_netmem in page pool's code, it has a
> >> different semantics from what page pool expects for net_iov. I.e.
> >> instead of releasing the netmem and allowing it to be reallocated by
> >> page pool, put_netmem(niov) will drop a memory provider reference and
> >> leak the net_iov. Depending on implementation it might even underflow
> >> mp refs if a net_iov is ever passed here.
> >>
> >
> > Hmm, put_netmem (I hope) is designed and implemented to do the right
> > thing no matter what netmem you pass it (and it needs to, because we
> > can't predict what netmem will be passed to it):
> >
> > - For non-pp pages, it drops a page ref.
> > - For pp pages, it drops a pp ref.
> > - For non-pp net_iovs (devmem TX), it drops a net_iov ref (which for
> > devmem net_iovs is a binding ref)
> > - For pp net_iovs, it drops a niov->pp ref (the same for both iouring
> > and devmem).
>
> void put_netmem(netmem_ref netmem)
> {
>         struct net_iov *niov;
>
>         if (netmem_is_net_iov(netmem)) {
>                 niov = netmem_to_net_iov(netmem);
>                 if (net_is_devmem_iov(niov))
>                         net_devmem_put_net_iov(netmem_to_net_iov(netmem));
>                 return;
>         }
>
>         put_page(netmem_to_page(netmem));
> }
> EXPORT_SYMBOL(put_netmem);
>
> void net_devmem_put_net_iov(struct net_iov *niov)
> {
>         net_devmem_dmabuf_binding_put(net_devmem_iov_binding(niov));
> }
>
> Am I looking at an outdated version? for devmem net_iov it always puts
> the binding and not niov refs, and it's always does put_page for pages.
> And it'd also silently ignore io_uring. And we're also patching early
> alloc/init failures in this series, so gauging if it's pp or non-pp
> originated struct page might be dangerous and depend on init order. We
> don't even need to think about all that if we continue to use put_page,
> which is why I think it's a much better option.
>

Oh, my bad. I was thinking of skb_page_unref, which actually handles
all net_iov/page types correctly. You're right, put_netmem doesn't
actually do that.

In that case reverting to put_page would be better here indeed.

-- 
Thanks,
Mina