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

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

[Mani Tofigh via B4 Relay]

From: Mani Tofigh <mt4022@columbia.edu>

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@columbia.edu>
---
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.

Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/napitimer.c
---
 include/linux/netdevice.h | 9 +++++++++
 net/core/dev.c            | 5 ++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 435be7880d7e..ad3685caf814 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);
 }
 
 /**
@@ -3025,8 +3030,12 @@ static inline void netif_napi_del_locked(struct napi_struct *napi)
  */
 static inline void netif_napi_del(struct napi_struct *napi)
 {
+	bool listed = test_bit(NAPI_STATE_LISTED, &napi->state);
+
 	__netif_napi_del(napi);
 	synchronize_net();
+	if (listed)
+		hrtimer_cancel(&napi->timer);
 }
 
 int netif_enable_cpu_rmap(struct net_device *dev, unsigned int num_irqs);
diff --git a/net/core/dev.c b/net/core/dev.c
index f32353b0467c..6d6384522a1b 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();

---
base-commit: 647b81ddefc6364290e67bf33c89d54971f8995e
change-id: 20261103-napi-timer-cancel-fa0a285fd581

Best regards,
--  
Mani Tofigh <mt4022@columbia.edu>

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@kernel.org> a écrit :
>
> From: Mani Tofigh <mt4022@columbia.edu>
>
> 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@columbia.edu>
> ---

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.

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

[Martin Karsten]

On 2026-11-03 13:37, Eric Dumazet wrote:
> Le mar. 3 nov. 2026 à 19:19, Mani Tofigh via B4 Relay
> <devnull+mt4022.columbia.edu@kernel.org> a écrit :
>>
>> From: Mani Tofigh <mt4022@columbia.edu>
>>
>> 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@columbia.edu>
>> ---

Thanks for reporting this. I'm very impressed by the reproducer!

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

I agree and I believe Sashiko has found this bug earlier [1]. I didn't 
have a reproducer at the time and maintainers didn't flag it, so I 
didn't speak up.

I thought this could be addressed more easily, by adding to each of the 
3 calls to hrtimer_start (maybe via subroutine as Eric suggests):

> 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);
> }

     hrtimer_start(&n->timer, ns_to_ktime(timeout),
             HRTIMER_MODE_REL_PINNED);
     if (unlikely(napi_is_scheduled(n)))
       hrtimer_cancel(&n->timer);

> And in __netif_napi_del_locked() :

This change is not needed then.

My reasoning is as follows: SCHED is being set in napi_disable_locked() 
before hrtimer_cancel() is called. hrtimer_start() and hrtimer_cancel() 
are atomic to each other and impose strict memory ordering via the timer 
lock (I assume?), so the SCHED bit is propagated, if hrtimer_cancel() in 
napi_disable() happens first. Memory is not yet released due to RCU. If 
the napi might be scheduled for some other reason, I believe it is still 
fine to cancel the timer.

I am not sure about hrtimer_try_to_cancel() vs hrtimer_cancel(). If 
hrtimer_try_to_cancel() fails, the timer callback might "running", but 
could it be on a different core and slow enough, so napi_disable() might 
complete and memory be freed before napi_watchdog() is done?

In my tests, including the reproducer, this works, but this stuff is 
always dicey...

Thanks,
Martin

[1] 
https://sashiko.dev/#/patchset/20260523012247.1574691-1-mkarsten%40uwaterloo.ca