Re: [PATCH net] net: cancel napi->timer after the grace period in netif_napi_del()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] The fix only adds the post-grace-period hrtimer_cancel() to the
  inline wrappers netif_napi_del() and netif_napi_del_locked()…
- [Medium] netdev_napi_exit() (net/core/dev.c:12453-12461) runs
  hrtimer_cancel(&p->timer) before __netif_napi_del_locked() and before…

--

net: cancel napi->timer after the grace period in netif_napi_del()

This patch adds a second hrtimer_cancel() of napi->timer after the
synchronize_net() in netif_napi_del() and netif_napi_del_locked(). It
targets a timer re-armed by napi_complete_done(), busy_poll_stop() or
napi_suspend_irqs() after napi_disable() has cancelled it. It also
cancels the timer in netdev_napi_exit(), relying on unregister having
already waited for a grace period.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 435be7880d7e5..ad3685caf8147 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3013,8 +3013,13 @@ static inline void __netif_napi_del(struct napi_struct *napi)
>  
>  static inline void netif_napi_del_locked(struct napi_struct *napi)
>  {
> +	bool listed = test_bit(NAPI_STATE_LISTED, &napi->state);
> +
>  	__netif_napi_del_locked(napi);
>  	synchronize_net();
> +	/* napi->timer can be started until a grace period after napi_disable() */
> +	if (listed)
> +		hrtimer_cancel(&napi->timer);
>  }

[Severity: High]
Does this close the bug for callers that call __netif_napi_del() or
__netif_napi_del_locked() directly and then run their own grace period?
The new cancel only exists in the two inline wrappers. Those direct
callers can still free or re-initialise the napi while napi->timer is
queued.

The netdevsim driver used by the reproducer is one of them, in
nsim_del_napi():

drivers/net/netdevsim/netdev.c:nsim_del_napi() {
    ...
		napi_disable_locked(&rq->napi);
		__netif_napi_del_locked(&rq->napi);
	}
	synchronize_net();
    ...
}

Nothing cancels the timer after that synchronize_net(). Later,
nsim_queue_free() only cancels rq->napi_timer before kfree(rq). On
reopen, netif_napi_add_config_locked()->hrtimer_setup() runs on a timer
that may still be queued.

The same pattern shows up in veth_napi_del_range() and in
virtnet_free_queues(), which is followed by kfree(vi->rq). It also shows
up in bnxt, enic, bnx2x, myri10ge, dpaa and bnge.

Two callers have no synchronous point after the grace period at all:

net/core/gro_cells.c:gro_cells_destroy() {
    ...
		napi_disable(&cell->napi);
		__netif_napi_del(&cell->napi);
		__skb_queue_purge(&cell->napi_skbs);
	}
    ...
		call_rcu(&defer->rcu, percpu_free_defer_callback);
    ...
}

ixgbe_free_q_vector() is similar: __netif_napi_del() followed by
kfree_rcu(q_vector, rcu). The note below the --- line suggests sending
the same cancel separately, but there is nowhere to put it in these two
paths. Would they need a different approach?

All of these paths share the window described in the commit message.
napi_complete_done() clears SCHED in its try_cmpxchg() before it calls
hrtimer_start(). busy_poll_stop() calls clear_bit(NAPI_STATE_SCHED)
before hrtimer_start(). napi_suspend_irqs() re-arms a napi it found
only through napi_by_id() under RCU.

The kernel-doc for __netif_napi_del() also still says:

 * Warning: caller must observe RCU grace period before freeing memory
 * containing @napi. Drivers might want to call this helper to combine
 * all the needed RCU grace periods into a single one.

That contradicts the new comment above, and the driver comments in
virtio_net, ixgbe and bnxt repeat it. Should the documented contract be
updated as well?

The remaining gap is only described below the --- line, so it won't be
in git history. That note also leaves out ixgbe, enic, bnx2x, myri10ge,
dpaa and bnge.

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f32353b0467cd..6d6384522a1bb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12450,9 +12450,12 @@ static void netdev_napi_exit(struct net_device *dev)
>  	if (!list_empty(&dev->napi_list)) {
>  		struct napi_struct *p, *n;
>  
> +		/* unregister has waited for a grace period after napi_disable() */
>  		netdev_lock(dev);
> -		list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
> +		list_for_each_entry_safe(p, n, &dev->napi_list, dev_list) {
> +			hrtimer_cancel(&p->timer);
>  			__netif_napi_del_locked(p);
> +		}
>  		netdev_unlock(dev);
>  
>  		synchronize_net();

[Severity: Medium]
Does the premise in this comment hold for every device that reaches
free_netdev()? Here the cancel runs before this function's own
synchronize_net(). That is the reverse of the order used in
netif_napi_del_locked().

unregister_netdevice_many_notify() calls synchronize_net() after close
and after ndo_uninit, so napis disabled in ndo_stop or ndo_uninit are
covered. However, free_netdev() also calls netdev_napi_exit() for
devices that were never unregistered. These include
NETREG_UNINITIALIZED probe-failure unwinds and NETREG_DUMMY devices
from alloc_netdev_dummy().

For example, mal_remove() in drivers/net/ethernet/ibm/emac/mal.c does:

		napi_disable(&mal->napi);
	...
	free_netdev(mal->dummy_dev);

There is no grace period in between. A registered device has the same
gap if its driver calls napi_disable() in the remove path after
unregister_netdev() and leaves the napi listed for free_netdev() to
delete.

On those paths, an in-flight start can arm the timer after this early
cancel:

  napi_complete_done() between its try_cmpxchg() and hrtimer_start()
  busy_poll_stop() between clear_bit() and hrtimer_start()
  a napi_suspend_irqs() reader

The synchronize_net() below then only waits for that start to finish,
and the napi is freed with its timer queued.

In practice, the timeouts default to 0 on unregistered and dummy
devices, and netlink napi-set requires NETREG_REGISTERED. The realistic
trigger is a registered device with gro_flush_timeout, defer_hard_irqs
or irq_suspend_timeout set. I couldn't find an in-tree driver that hits
this today.

Could netdev_napi_exit() do the grace period itself, for example a
synchronize_net() before the loop, instead of relying on the caller?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103-napi-timer-cancel-v1-1-55c631ad5286%40columbia.edu