Re: [PATCH net-next v1 06/11] page_pool: Coordinate refill with memory providers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 2

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).

- [Medium] net/xdp/xsk_pp.c: xsk_pp_refill_done() can read freed memory.
- [Medium] Provider wakeups that land on a disabled NAPI are dropped
  silently, and xsk_pp_release_netmem() clears pp->starved before it…
- [Medium] net: wrong mp_napi state after netdev_rx_queue_reconfig()
  error recovery
- [Medium] net: dangling rxq->napi left by netif_mp_napi_del() and
  republished
- [Low] Not confirmed (baseline: false positive).
- [Low] Not confirmed (baseline: false positive).

--

page_pool: Coordinate refill with memory providers

This commit adds a refill_done memory provider op and a
page_pool_refill_done() helper, so drivers can report the end of each RX
ring refill. While a queue runs, the core publishes its NAPI in
rxq->mp_napi, and a provider can use it to wake the queue when buffers
come back. The xsk provider uses this to track starvation, and AF_XDP
socket wakeups go through it instead of ndo_xsk_wakeup.

> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index 4e2f906fbc735..3fe904b56791b 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -116,6 +116,94 @@ bool netif_all_mps_have_cap(struct net_device *dev, u32 cap)
>  	return true;
>  }
>  
> +/**
> + * netif_mp_wake - schedule the NAPI that refills a memory provider queue

[ ... ]

> +bool netif_mp_wake(struct netdev_rx_queue *rxq)
> +{
> +	struct napi_struct *napi;
> +	bool running = false;
> +
> +	rcu_read_lock();
> +	napi = rcu_dereference(rxq->mp_napi);
> +	if (napi) {
> +		local_bh_disable();
> +		napi_schedule(napi);
> +		local_bh_enable();
> +		running = true;
> +	}
> +	rcu_read_unlock();
> +
> +	return running;
> +}
> +
> +/* Point wakeups at the NAPI of the running queue, and schedule it once: a
> + * refill during the start may have found no buffers.
> + */
> +void netif_mp_update_napi(struct netdev_rx_queue *rxq)
> +{
> +	const struct memory_provider_ops *ops = rxq->mp_params.mp_ops;
> +	struct napi_struct *napi = NULL;
> +
> +	if (rxq->mp_armed && ops && ops->refill_done)
> +		napi = rxq->napi;
> +	if (rcu_access_pointer(rxq->mp_napi) == napi)
> +		return;
> +
> +	rcu_assign_pointer(rxq->mp_napi, napi);
> +	if (napi)
> +		netif_mp_wake(rxq);
> +}

[Severity: Medium]
Can this leave rxq->napi dangling for netif_mp_update_napi() to pick
up again?

netif_mp_napi_del() clears dev->_rx[i].mp_napi when it matches the
NAPI being deleted, but dev->_rx[i].napi still points at it.
__dev_close_many() and netdev_rx_queue_reconfig() disarm the queue
around ndo_stop() and ndo_queue_stop(). While it is disarmed, mp_napi
is already NULL, so this loop does not touch the queue at all.

Suppose the driver deletes the NAPI without first calling
netif_queue_set_napi() with a NULL napi. Suppose it also does not
register another NAPI for that queue before the next
netif_mp_arm_napi() or netif_mp_arm_dev(). Then
netif_mp_update_napi() reads the stale rxq->napi and publishes it in
rxq->mp_napi. It then calls netif_mp_wake(), which does
napi_schedule() on a deleted napi_struct.

For drivers that free the NAPI together with their queue vectors, this
is a use-after-free. Every later provider wakeup also goes through the
same pointer.

Should netif_mp_napi_del() also clear rxq->napi when it matches, or
should netif_mp_update_napi() only publish a NAPI that is still
listed? If drivers are expected to clear the association themselves,
could netmem.rst item 8 say so?

[Severity: Medium]
Can this kick be lost when it lands on a disabled NAPI?

Drivers often call netif_queue_set_napi() during ring setup, before
napi_enable(). If the NAPI is disabled, napi_schedule_prep() sees
NAPIF_STATE_SCHED already set and only sets NAPIF_STATE_MISSED.
napi_enable_locked() then clears SCHED/NPSVC/DISABLED without acting on
MISSED, so no poll runs.

netif_mp_wake() still returns true, because it ignores the result of
napi_schedule().

If the driver reuses the same napi_struct across a restart, the early
return on rcu_access_pointer(rxq->mp_napi) == napi skips the kick
entirely.

The core re-kicks only from netif_mp_arm_dev() and netif_mp_arm_napi().
Driver-internal restarts (ethtool -G/-L, MTU or XDP changes, resets)
keep mp_armed set, so this kick is the only one they get.

The starved state doesn't recover either. xsk_pp_release_netmem() clears
pp->starved before it knows whether the wakeup reached an enabled NAPI:

	starved = pp->starved;
	WRITE_ONCE(pp->starved, false);
	spin_unlock_bh(&pp->lock);

	/* A starved queue may wait for this release */
	if (starved)
		xsk_pp_wake(pp);

Here is one sequence:

- The new rings are filled while the old pool still holds the chunks,
  so the refill finds nothing and sets starved.
- The old pool then releases the chunks while mp_napi is NULL or the
  NAPI is still disabled, so the wakeup is dropped.
- Later releases don't retry, and the empty ring raises no IRQ.

Wouldn't that leave RX stalled, which is the problem this patch sets out
to fix?

fbnic avoids this because fbnic_napi_enable() and __fbnic_nv_restart()
write FBNIC_INTR_SET after napi_enable_locked(). However, netmem.rst item
8 only says "the core schedules it when provider buffers become
available", and does not ask drivers to force a poll after napi_enable().

Should the documented contract require that, or should the core handle
the disabled-NAPI case itself?

[ ... ]

> diff --git a/net/xdp/xsk_pp.c b/net/xdp/xsk_pp.c
> index c2214e718cdb3..a77213f7fef6b 100644
> --- a/net/xdp/xsk_pp.c
> +++ b/net/xdp/xsk_pp.c

[ ... ]

> @@ -232,11 +253,47 @@ static bool xsk_pp_release_netmem(struct page_pool *pool, netmem_ref netmem)

[ ... ]

> +static bool xsk_pp_refill_done(struct page_pool *pool, enum pp_refill state)
> +{
> +	struct xsk_pp *pp = pool->mp_priv;
> +	struct xsk_buff_pool *xp = pp->pool;
> +	bool wait = xp->uses_need_wakeup;

[Severity: Medium]
Can this read a freed xsk_buff_pool?

pp->pool is dereferenced in the initializer, before the !pp->fq check
that handles a detached provider. So the read happens even on the
PP_REFILL_FULL fast path.

The xsk_pp can outlive the buff pool. Each page pool built on it holds a
ref taken in xsk_pp_init(), but xp_destroy() drops only the buff pool's
own ref before freeing it:

net/xdp/xsk_buff_pool.c:xp_destroy() {
    ...
	if (pool->pp)
		xsk_pp_put(pool->pp);
	kvfree(pool->tx_descs);
	kvfree(pool->heads);
	kvfree(pool);
}

One way to get there is a failed queue reconfig during unbind, after
which the old queue keeps running on the provider's page pools. The
reconfig can fail in kvzalloc() or ndo_queue_mem_alloc(), or in
ndo_queue_start(), whose error path restarts old_mem:

xsk_pp_unbind()
  netif_mp_close_rxq()
    __netif_mp_close_rxq()
      netdev_rx_queue_reconfig()   <- fails, only WARNs
  xsk_pp_detach()                  <- error ignored, fq = NULL
xp_destroy()                       <- kvfree(pool)

After that, the old queue's NAPI still calls page_pool_refill_done()
after each refill. That reaches xsk_pp_refill_done(), which reads
xp->uses_need_wakeup from the freed pool.

Before this patch, the provider callbacks that can run after detach
either didn't touch pp->pool or checked pp->fq first.

Could uses_need_wakeup be cached in struct xsk_pp when it is created, or
read only after pp->fq is confirmed under pp->lock?

> +	bool more;
> +
> +	if (state == PP_REFILL_FULL && !READ_ONCE(pp->starved))
> +		return true;
> +
> +	spin_lock_bh(&pp->lock);
> +	if (!pp->fq) {
> +		spin_unlock_bh(&pp->lock);
> +		return true;
> +	}

[ ... ]

> @@ -183,8 +273,13 @@ static int netdev_rx_queue_reconfig(struct net_device *dev,
>  		     "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
>  		     rxq_idx);
>  		qops->ndo_queue_mem_free(dev, old_mem);
> +		goto err_free_new_queue_mem;
>  	}
>  
> +err_arm_old_queue:
> +	/* The old queue may run */
> +	netif_mp_arm_napi(rxq);
> +
>  err_free_new_queue_mem:
>  	qops->ndo_queue_mem_free(dev, new_mem);
>  

[Severity: Medium]
Does this arm the queue from the right provider state?

When netdev_rx_queue_reconfig() runs, rxq->mp_params already holds the
target configuration. It does not hold the configuration of the queue
that is restarted on this error path.

In __netif_mp_open_rxq(), rxq->mp_params = *p is written before the
reconfig. If ndo_queue_stop() fails, or ndo_queue_start() fails and the
old queue is restarted, netif_mp_arm_napi() here sees the new
provider's refill_done. It publishes rxq->napi in rxq->mp_napi and
kicks it. The error then goes back to __netif_mp_open_rxq(), which
zeroes rxq->mp_params at err_clear_mp but never resyncs mp_napi.

That leaves mp_napi published for a queue with no provider installed,
which breaks the rule documented in struct netdev_rx_queue. A later
netif_mp_wake() on that rxq would schedule it. The early return in
netif_mp_update_napi() would also compare against a pointer that
should not be there.

The close direction is the mirror image. __netif_mp_close_rxq() clears
rxq->mp_params before the reconfig. If the stop or the restart then
fails, netif_mp_update_napi() sees no mp_ops and leaves mp_napi NULL,
even though the old queue keeps running on the provider's page pools.

Should the error path arm from the configuration that is running
again, or should __netif_mp_open_rxq() and __netif_mp_close_rxq() call
netif_mp_update_napi() once they have settled rxq->mp_params?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org