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

[Martin Karsten <mkarsten@uwaterloo.ca>]

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