Re: [PATCH] bnxt: prevent timer rearming during teardown

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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).

- [Medium] The commit message describes a bug that does not exist, and
  the change does not work the way the message says.

--

bnxt: prevent timer rearming during teardown

This patch adds a BNXT_STATE_OPEN check before the final mod_timer() in
bnxt_timer(). The goal is to stop a callback that is already running from
rearming the timer once __bnxt_close_nic() has cleared the flag. It is
tagged Fixes: c0c050c58d84 and Cc: stable.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d7728d0c5b6e6..c506b6160902f 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14521,7 +14521,8 @@ static void bnxt_timer(struct timer_list *t)
>  		bnxt_queue_sp_work(bp, BNXT_RING_COAL_NOW_SP_EVENT);
>  
>  bnxt_restart_timer:
> -	mod_timer(&bp->timer, jiffies + bp->current_interval);
> +	if (test_bit(BNXT_STATE_OPEN, &bp->state))
> +		mod_timer(&bp->timer, jiffies + bp->current_interval);

[Severity: Medium]
Is there actually a teardown race here? The commit message says:

  If teardown clears BNXT_STATE_OPEN while the callback is running, the
  callback can rearm the timer after __bnxt_close_nic() has started
  deleting it.

In __bnxt_close_nic(), the teardown sequence is:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:__bnxt_close_nic() {
    ...
	clear_bit(BNXT_STATE_OPEN, &bp->state);
	smp_mb__after_atomic();
    ...
	timer_delete_sync(&bp->timer);
    ...
}

timer_delete_sync() already handles a callback that rearms itself. While
the callback is running, __timer_delete_sync() keeps retrying:

kernel/time/timer.c:__timer_delete_sync() {
    ...
	do {
		ret = __try_to_del_timer_sync(timer, shutdown);

		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);
    ...
}

After the callback returns, __try_to_del_timer_sync() sees that
base->running_timer != timer and calls detach_if_pending(). That removes
any timer the callback rearmed. The kerneldoc for timer_delete_sync()
says:

  Upon exit the timer is not queued and the handler is not running on any
  CPU.

The rearmed timer could also fire before it is detached. In that case the
entry check in bnxt_timer() returns without rearming, because
BNXT_STATE_OPEN is already clear. The only other place that arms the
timer is __bnxt_open_nic(), and it does so after
set_bit(BNXT_STATE_OPEN).

So before this patch, the worst case looks like one extra mod_timer()
that timer_delete_sync() then detaches.

The commit message also says the new check "prevents a callback already
in progress from starting another timer period". But test_bit() and
mod_timer() are two separate steps with no lock around them. A
clear_bit() on another CPU can land between them, and the callback can
still rearm. The guarantee still comes from timer_delete_sync().

Would it be better to treat this as a cleanup? That would mean dropping
the Fixes: and Cc: stable tags and rewording the commit message so it
doesn't describe a teardown race fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930092806.2977763-1-runyu.xiao%40seu.edu.cn