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@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>
> > ---

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