[PATCH] bnxt: prevent timer rearming during teardown

[PATCH] bnxt: prevent timer rearming during teardown

[Runyu Xiao]

bnxt_timer() checks the interface state before processing timer work, but
the final mod_timer() at bnxt_restart_timer is unconditional. 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.

Check BNXT_STATE_OPEN again before rearming so teardown prevents a callback
already in progress from starting another timer period.

Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d59bcca73..8a2e9e200 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14484,7 +14484,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);
 }
 
 static void bnxt_lock_sp(struct bnxt *bp)
-- 
2.34.1

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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