Re: [PATCH net-next] net: devmem: use page_pool_put_netmem_bulk

[Mina Almasry]

On Fri, Sep 25, 2026 at 5:49 AM Mina Almasry <almasrymina@xxxxxxxxxx> wrote:
>
> On Thu, Sep 24, 2026 at 3:36 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 1 · Medium: 0 · Low: 0
> >
> > If you believe none of the issues are legitimate, please reply to each
> > issue of Medium severity or higher and explain why the AI is wrong.
> > If any of them are legitimate and you plan to respin, please let patchwork
> > know by including "pw-bot: cr" as a separate line at the end of your reply
> > (one such reply per series is enough).
> >
> > - [High] sock_devmem_dontneed() (net/core/sock.c) now releases the last
> >   page_pool reference of devmem net_iovs via…
> >
> > --
> >
> > net: devmem: use page_pool_put_netmem_bulk
> >
> > This converts sock_devmem_dontneed() from a loop of napi_pp_put_page()
> > calls to a single page_pool_put_netmem_bulk() call per batch, so the
> > page_pool ring producer lock is taken once per batch instead of once per
> > netmem.
> >
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index 2948dffcc3e1b..a6b6d5b2051df 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> >
> > [ ... ]
> >
> > > @@ -1123,8 +1124,7 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen)
> > >                       netmems[netmem_num++] = netmem;
> > >                       if (netmem_num == ARRAY_SIZE(netmems)) {
> > >                               xa_unlock_bh(&sk->sk_user_frags);
> > > -                             for (k = 0; k < netmem_num; k++)
> > > -                                     WARN_ON_ONCE(!napi_pp_put_page(netmems[k]));
> > > +                             page_pool_put_netmem_bulk(netmems, ARRAY_SIZE(netmems));
> >
> > [Severity: High]
> > Can this reintroduce the page_pool use-after-free that commit
> > 271683bb2cf32e ("page_pool: Fix use-after-free in
> > page_pool_recycle_in_ring") addressed, when CONFIG_PAGE_POOL_STATS=y?
> >
> > The old path went through napi_pp_put_page() ->
> > page_pool_put_unrefed_netmem() -> page_pool_recycle_in_ring(), which
> > updates the stat while still holding the producer lock:
> >
> > net/core/page_pool.c:page_pool_recycle_in_ring() {
> >         in_softirq = page_pool_producer_lock(pool);
> >         ret = !__ptr_ring_produce(&pool->ring, (__force void *)netmem);
> >         if (ret)
> >                 recycle_stat_inc(pool, ring);
> >         page_pool_producer_unlock(pool, in_softirq);
> > }
> >
> > The bulk path touches the pool after the unlock:
> >
> > net/core/page_pool.c:page_pool_recycle_ring_bulk() {
> >         ...
> >         page_pool_producer_unlock(pool, in_softirq);
> >         recycle_stat_add(pool, ring, i);
> >
> > Once the netmems are published into pool->ring, does the setsockopt
> > thread still hold anything that keeps the pool alive?  The barrier in
> > page_pool_release() only covers work done inside the producer lock:
> >
> > net/core/page_pool.c:page_pool_release() {
> >         page_pool_scrub(pool);
> >         inflight = page_pool_inflight(pool, true);
> >         /* Acquire producer lock to make sure producers have exited. */
> >         in_softirq = page_pool_producer_lock(pool);
> >         page_pool_producer_unlock(pool, in_softirq);
> >         if (!inflight)
> >                 __page_pool_destroy(pool);
> > }
> >
> > so the sequence would be:
> >
> > CPU0 (setsockopt SO_DEVMEM_DONTNEED)
> >   page_pool_put_netmem_bulk()
> >     page_pool_recycle_ring_bulk()
> >       __ptr_ring_produce()          /* last inflight refs published */
> >       page_pool_producer_unlock()   /* spin_unlock_bh(): BH/preempt window */
> >
> > CPU1 (page_pool_release_retry() worker after deferred destroy)
> >   page_pool_release()
> >     page_pool_scrub() -> page_pool_empty_ring() -> page_pool_return_netmem()
> >     page_pool_inflight() == 0
> >     producer lock taken/released uncontended
> >     __page_pool_destroy() -> page_pool_uninit()
> >       free_percpu(pool->recycle_stats); kfree(pool);
> >
> > CPU0
> >       recycle_stat_add(pool, ring, i);   /* this_cpu_add on freed memory */
> >
> > Since the release is driven here by returning the last outstanding user
> > tokens, would it be better to move recycle_stat_add(pool, ring, i) inside
> > the producer-lock section of page_pool_recycle_ring_bulk() (matching
> > page_pool_recycle_in_ring()) before converting sock_devmem_dontneed() to
> > the bulk helper?
> >
>
> Thank you my dear bot. This seems like a real issue, a latent bug in
> the *_bulk() put. Let me use my pet LLM to develop a fix for this,
> send to net, and then respin this.
>
> Lets hold onto this for now Kaifeng, and resubmit after my fix goes into net.
>

I submitted a fix to net:

https://lore.kernel.org/netdev/20260925144127.1445667-1-almasrymina@xxxxxxxxxx/

I guess I want to make sure the fix goes in before we adopt _bulk in
devmem tcp, so defer this one I guess and repost when the net fix goes
in:

-- 
Thanks,
Mina

pw-bot: cr