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

[Eric Dumazet]

Le mar. 3 nov. 2026 à 19:19, Mani Tofigh via B4 Relay
<devnull+mt4022.columbia.edu@xxxxxxxxxx> a écrit :
>
> From: Mani Tofigh <mt4022@xxxxxxxxxxxx>
>
> A napi can be freed with its timer still queued.  napi_disable()
> cancels napi->timer, but napi_complete_done(), busy_poll_stop() and
> napi_suspend_irqs() start it without owning the napi and can do so
> right after the cancel.  netif_napi_del() only waits for that start to
> finish:
>
>   Task A (epoll_wait)                  Task B (queue reset)
>   napi_suspend_irqs()
>     rcu_read_lock()
>     napi = napi_by_id(id)
>                                        napi_disable_locked(napi)
>                                          hrtimer_cancel(&napi->timer)
>                                          napi_hash_del(napi)
>                                        netif_napi_del_locked(napi)
>                                          synchronize_net()
>                                          /* waits for Task A */
>     hrtimer_start(&napi->timer)
>     rcu_read_unlock()
>                                        kfree(rq)
>                                          /* timer still queued */
>
> With netdevsim and debug objects:
>
>   [    3.073483] ODEBUG: free active (active state 0) object: 000000001504358f object type: hrtimer hint: napi_watchdog+0x0/0xa0
>   [    3.073515] WARNING: lib/debugobjects.c:629 at debug_print_object+0x1a4/0x3b0, CPU#0: napitimer/66
>   [    3.079603] CPU: 0 UID: 0 PID: 66 Comm: napitimer Tainted: G        W            7.3.0-17087-gd865f2677c44 #1 PREEMPT
>   [    3.084822] RIP: 0010:debug_print_object+0x1ae/0x3b0
>   ...
>   [    3.103822] Call Trace:
>   [    3.104408]  <TASK>
>   [    3.105933]  debug_check_no_obj_freed+0x496/0x5f0
>   [    3.109988]  kfree+0x33a/0x510
>   [    3.110715]  netdev_rx_queue_reconfig+0x357/0x580
>   [    3.111780]  netdev_rx_queue_restart+0x8f/0xc0
>   [    3.113924]  nsim_qreset_write+0x24d/0x350
>   [    3.117832]  full_proxy_write+0x109/0x1d0
>   [    3.118741]  vfs_write+0x1e3/0x1250
>   [    3.124758]  __x64_sys_pwrite64+0x18c/0x250
>   [    3.129645]  do_syscall_64+0xfb/0x590
>   [    3.131385]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Without debug objects, napi_watchdog() later runs on freed memory.
>
> After napi_disable() and a grace period none of the three can reach the
> napi anymore.  Two need SCHED, which napi_disable() keeps, and
> napi_suspend_irqs() needs the napi hash, which napi_disable() removed it
> from.  So cancel the timer again after the grace period that
> netif_napi_del() already waits for, and in free_netdev(), where
> unregister has already waited for one.  With the fix, debug objects
> don't report a freed active timer anymore.
>
> This bug was identified and confirmed with the help of a deterministic
> hypervisor currently under development.  The C reproducer below however
> hits it within about 1s.
>
> Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> Fixes: 3fcbecbdeb04 ("net: Add control functions for irq suspension")
> Fixes: 58e2330bd455 ("net: napi: Avoid gro timer misfiring at end of busypoll")
> Signed-off-by: Mani Tofigh <mt4022@xxxxxxxxxxxx>
> ---

Bug seems real, but could we prepare something generic (in the core).

> Drivers that call __netif_napi_del{,_locked}() and then their own
> synchronize_net() (eg netdevsim on close, veth, virtio_net, bnxt and
> gro_cells) need the same cancel.  I can send that separately.

The list is longer than that : bnx2x, enic, ixgbe, myri10ge, dpaa and
bnge also use __netif_napi_del{,_locked}(). I would rather not touch 11
drivers for a core implementation detail.

Also the free_netdev() part assumes napi_disable() was called before
the synchronize_net() in unregister_netdevice_many_notify(), which is
not guaranteed (ndo_uninit(), or remove() handlers calling
napi_disable() between unregister_netdev() and free_netdev()).

All callers of __netif_napi_del_locked() must already respect an RCU
grace period before freeing the napi, and the three hrtimer_start()
callers run under RCU or with BH disabled.

So I think we can solve this in the core, with a store-buffer
pairing on NAPI_STATE_LISTED: the arming side re-checks LISTED after
hrtimer_start(), and __netif_napi_del_locked() cancels the timer after
clearing LISTED. One of the two must see the other.

Something like (untested):

/* Callers do not own @n, which can be deleted concurrently.
 * Pairs with test_and_clear_bit(NAPI_STATE_LISTED)
 * in __netif_napi_del_locked().
 */
static void napi_timer_start(struct napi_struct *n, unsigned long timeout)
{
        hrtimer_start(&n->timer, ns_to_ktime(timeout),
                      HRTIMER_MODE_REL_PINNED);
        smp_mb();
        if (unlikely(!test_bit(NAPI_STATE_LISTED, &n->state)))
                hrtimer_try_to_cancel(&n->timer);
}

And in __netif_napi_del_locked() :

        if (!test_and_clear_bit(NAPI_STATE_LISTED, &napi->state))
                return;

        /* Full barrier implied by test_and_clear_bit()
         * pairs with napi_timer_start().
         */
        hrtimer_cancel(&napi->timer);

napi_complete_done(), busy_poll_stop() and napi_suspend_irqs() would
use napi_timer_start().

If hrtimer_try_to_cancel() fails, napi_watchdog() is running, either
from hard irq or softirq context. It does not rearm the timer, and the
grace period the caller of __netif_napi_del_locked() has to observe
before freeing the napi also waits for it.

A timer armed between napi_disable() and __netif_napi_del_locked()
is harmless : napi_watchdog() can not grab NAPI_STATE_SCHED.

This removes the need for the 'listed' dance in netif_napi_del() and
the netdev_napi_exit() change, and covers all drivers.

The extra smp_mb() is only paid when the timer is armed, next to
an hrtimer_start() which is already much more expensive.

Can you give this a try with your reproducer ?

Thanks.