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

[Mina Almasry]

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.

-- 
Thanks,
Mina