[PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Bryan Cochrane]

netif_close() must be called with RTNL held in addition to the netdev
instance lock: __dev_close_many() asserts it, and the NETDEV_GOING_DOWN
and NETDEV_DOWN notifier chains it runs rely on it for serialization.

Commit 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
replaced rtnl_lock() with netdev_lock() in bnxt_fw_reset_task() and
bnxt_fw_reset(). That is sufficient for the paths that only tear the
NIC down internally with __bnxt_close_nic(), but the abort paths close
the netdev via bnxt_fw_reset_abort() -> netif_close(), and the error
path in bnxt_fw_reset() calls netif_close() directly. Those now run
without RTNL and trigger a WARN_ONCE() cascade when a firmware reset is
aborted:

  RTNL: assertion failed at net/core/dev.c (1740)
  WARNING: net/core/dev.c:1740 at __dev_close_many+0x1be/0x1f0
  Workqueue: bnxt_pf_wq bnxt_fw_reset_task [bnxt_en]
  Call Trace:
   netif_close_many+0xb5/0x190
   netif_close+0xa2/0xd0
   bnxt_fw_reset_task.cold+0x8d/0x213 [bnxt_en]
   process_one_work+0x19d/0x3a0
   worker_thread+0x1af/0x320
   kthread+0xe3/0x120

followed by the same assertion from call_netdevice_notifiers_info(),
fib_rules_event(), inetdev_event(), rtnl_fill_ifinfo(),
devlink_compat_phys_port_name_get() and ip_mc_down().

This was observed on a machine in real use with a BCM57504 (4 PFs,
FW 236.1.153.0) running v7.1.6: the firmware crashed, announced a
"Fatal firmware reset event" and then failed to come back within the
advertised max wait, so bnxt_fw_reset_task() aborted the reset on all
ports:

  bnxt_en 0000:c1:00.1 eno16905np1: Firmware reset aborted
  bnxt_en 0000:c1:00.1 eno16905np1: fw_health_status 0x448100

Take RTNL (before the netdev instance lock, matching the OPENING state
which already uses rtnl_trylock()) in the fw_reset_abort path of
bnxt_fw_reset_task() and route the POLL_VF abort through that same
path.

In bnxt_fw_reset() only the error path that closes the device after
bnxt_get_registered_vfs() fails needs RTNL: drop the instance lock,
take RTNL and the instance lock, then clear BNXT_STATE_IN_FW_RESET and
close. The normal reset flow, which may run on many VFs at the same
time, keeps running without RTNL. BNXT_STATE_IN_FW_RESET stays set
across the lock switch, so a concurrent close marks the reset aborted
as it does today and a concurrent recovery request backs off.

The bug was found by inspecting the kernel log of the affected machine
and the driver source; the fix was prepared with the help of an AI
coding assistant (Gemini based) and reviewed by the submitter. It is
build tested (allmodconfig with W=1, plus the same change on top of
v7.1.6 built as a module for the affected BCM57504 host). The aborted
reset path itself only runs when the firmware fails to return from a
reset, which could not be reproduced on demand.

Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan Cochrane <bcochrane@google.com>
---
v2:
 - Keep bnxt_lock_sp() in bnxt_fw_reset() and take RTNL only on the
   bnxt_get_registered_vfs() error path, so that the normal FW reset
   flow does not add RTNL contention when many VFs recover at the same
   time (Michael Chan).
 - Dropped Stanislav's Acked-by because of the change above.
v1: https://lore.kernel.org/netdev/20261029032832.1511180-1-bcochrane@google.com/
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c4cacc8ffe..1ea1e8266eab 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14860,9 +14860,16 @@ void bnxt_fw_reset(struct bnxt *bp)
 		if (n < 0) {
 			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
 				   n);
+			/* netif_close() requires RTNL, which must be taken
+			 * before the netdev instance lock.
+			 */
+			netdev_unlock(bp->dev);
+			rtnl_lock();
+			netdev_lock(bp->dev);
 			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
 			netif_close(bp->dev);
-			goto fw_reset_exit;
+			bnxt_rtnl_unlock_sp(bp);
+			return;
 		} else if (n > 0) {
 			u16 vf_tmo_dsecs = n * 10;
 
@@ -15547,9 +15554,8 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		bp->fw_reset_timestamp = jiffies;
 		netdev_lock(bp->dev);
 		if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) {
-			bnxt_fw_reset_abort(bp, rc);
 			netdev_unlock(bp->dev);
-			goto ulp_start;
+			goto fw_reset_abort;
 		}
 		bnxt_fw_reset_close(bp);
 		if (bp->fw_cap & BNXT_FW_CAP_ERR_RECOVER_RELOAD) {
@@ -15682,9 +15688,11 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		netdev_err(bp->dev, "fw_health_status 0x%x\n", sts);
 	}
 fw_reset_abort:
+	rtnl_lock();
 	netdev_lock(bp->dev);
 	bnxt_fw_reset_abort(bp, rc);
 	netdev_unlock(bp->dev);
+	rtnl_unlock();
 ulp_start:
 	if (!rc)
 		bnxt_ulp_start(bp);

base-commit: c3e67bd82b9b9b1a5118f09a429ccefe2aab0741
-- 
2.56.0.385.gac671c819a-goog

Re: [PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On Mon, Nov 2, 2026 at 8:22 PM Bryan Cochrane <bcochrane@google.com> wrote:

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe..1ea1e8266eab 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14860,9 +14860,16 @@ void bnxt_fw_reset(struct bnxt *bp)
>                 if (n < 0) {
>                         netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
>                                    n);
> +                       /* netif_close() requires RTNL, which must be taken
> +                        * before the netdev instance lock.
> +                        */
> +                       netdev_unlock(bp->dev);
> +                       rtnl_lock();
> +                       netdev_lock(bp->dev);

If you need to respin, you should be able to call bnxt_rtnl_lock_sp()
instead of the 2 locking calls.  But this is fine too.  Thanks.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Stanislav Fomichev]

On 11/02, Bryan Cochrane wrote:
> netif_close() must be called with RTNL held in addition to the netdev
> instance lock: __dev_close_many() asserts it, and the NETDEV_GOING_DOWN
> and NETDEV_DOWN notifier chains it runs rely on it for serialization.
> 
> Commit 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
> replaced rtnl_lock() with netdev_lock() in bnxt_fw_reset_task() and
> bnxt_fw_reset(). That is sufficient for the paths that only tear the
> NIC down internally with __bnxt_close_nic(), but the abort paths close
> the netdev via bnxt_fw_reset_abort() -> netif_close(), and the error
> path in bnxt_fw_reset() calls netif_close() directly. Those now run
> without RTNL and trigger a WARN_ONCE() cascade when a firmware reset is
> aborted:
> 
>   RTNL: assertion failed at net/core/dev.c (1740)
>   WARNING: net/core/dev.c:1740 at __dev_close_many+0x1be/0x1f0
>   Workqueue: bnxt_pf_wq bnxt_fw_reset_task [bnxt_en]
>   Call Trace:
>    netif_close_many+0xb5/0x190
>    netif_close+0xa2/0xd0
>    bnxt_fw_reset_task.cold+0x8d/0x213 [bnxt_en]
>    process_one_work+0x19d/0x3a0
>    worker_thread+0x1af/0x320
>    kthread+0xe3/0x120
> 
> followed by the same assertion from call_netdevice_notifiers_info(),
> fib_rules_event(), inetdev_event(), rtnl_fill_ifinfo(),
> devlink_compat_phys_port_name_get() and ip_mc_down().
> 
> This was observed on a machine in real use with a BCM57504 (4 PFs,
> FW 236.1.153.0) running v7.1.6: the firmware crashed, announced a
> "Fatal firmware reset event" and then failed to come back within the
> advertised max wait, so bnxt_fw_reset_task() aborted the reset on all
> ports:
> 
>   bnxt_en 0000:c1:00.1 eno16905np1: Firmware reset aborted
>   bnxt_en 0000:c1:00.1 eno16905np1: fw_health_status 0x448100
> 
> Take RTNL (before the netdev instance lock, matching the OPENING state
> which already uses rtnl_trylock()) in the fw_reset_abort path of
> bnxt_fw_reset_task() and route the POLL_VF abort through that same
> path.
> 
> In bnxt_fw_reset() only the error path that closes the device after
> bnxt_get_registered_vfs() fails needs RTNL: drop the instance lock,
> take RTNL and the instance lock, then clear BNXT_STATE_IN_FW_RESET and
> close. The normal reset flow, which may run on many VFs at the same
> time, keeps running without RTNL. BNXT_STATE_IN_FW_RESET stays set
> across the lock switch, so a concurrent close marks the reset aborted
> as it does today and a concurrent recovery request backs off.
> 
> The bug was found by inspecting the kernel log of the affected machine
> and the driver source; the fix was prepared with the help of an AI
> coding assistant (Gemini based) and reviewed by the submitter. It is
> build tested (allmodconfig with W=1, plus the same change on top of
> v7.1.6 built as a module for the affected BCM57504 host). The aborted
> reset path itself only runs when the firmware fails to return from a
> reset, which could not be reproduced on demand.
> 
> Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan Cochrane <bcochrane@google.com>
> ---
> v2:
>  - Keep bnxt_lock_sp() in bnxt_fw_reset() and take RTNL only on the
>    bnxt_get_registered_vfs() error path, so that the normal FW reset
>    flow does not add RTNL contention when many VFs recover at the same
>    time (Michael Chan).
>  - Dropped Stanislav's Acked-by because of the change above.

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (2) · Medium: 0 (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).

- [High] New deadlock in bnxt_fw_reset().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same Fixes
  commit (004b5008016a) left another netif_close()-without-RTNL site…
- [High] This problem wasn't introduced by this patch, but the same
  netif_close()-without-RTNL bug is still reachable through the TPH…
- [Medium] This problem wasn't introduced by this patch, but the
  bnxt_get_registered_vfs() failure path in bnxt_fw_reset() leaves the…

--

bnxt_en: Take RTNL before netif_close() when aborting FW reset

This patch takes RTNL before the netdev instance lock on the FW reset
abort paths in bnxt_fw_reset_task(). It does the same on the
bnxt_get_registered_vfs() failure path in bnxt_fw_reset(), so that
netif_close() runs with RTNL held. The goal is to fix the ASSERT_RTNL()
warnings introduced by commit 004b5008016a ("eth: bnxt: remove most
dependencies on RTNL").

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe3..1ea1e8266eabf 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14860,9 +14860,16 @@ void bnxt_fw_reset(struct bnxt *bp)
>  		if (n < 0) {
>  			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
>  				   n);
> +			/* netif_close() requires RTNL, which must be taken
> +			 * before the netdev instance lock.
> +			 */
> +			netdev_unlock(bp->dev);
> +			rtnl_lock();
> +			netdev_lock(bp->dev);

[Severity: High]
Can a concurrent devlink health recover hang the sp_task here while it
holds RTNL?

bnxt_sp_task() is not the only caller of bnxt_fw_reset(). When
devlink_health_report() returns -ECANCELED,
bnxt_devlink_health_fw_report() calls __bnxt_fw_recover() from the sp_task
without the devlink lock.

At that point the reporter is already in the error state. A userspace
"devlink health recover" can then reach bnxt_fw_reset() through
bnxt_fw_recover() at the same time.

Before this patch the instance lock was held all the way through
netif_close(). The new unlock/relock window seems to allow this ordering:

CPU1 (bnxt_sp_task)
bnxt_devlink_health_fw_report()
  __bnxt_fw_recover()
    bnxt_fw_reset()
      bnxt_lock_sp()          /* clears IN_SP_TASK */
      set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
      bnxt_get_registered_vfs() returns < 0
      netdev_unlock(bp->dev);

CPU2 (devlink health recover)
devlink_health_reporter_recover()
  bnxt_fw_recover()
    __bnxt_fw_recover()
      bnxt_fw_reset()
        bnxt_lock_sp()        /* now gets the instance lock */
        IN_FW_RESET is set, nothing to do
        bnxt_unlock_sp()
          set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
          netdev_unlock(bp->dev);

CPU1
      rtnl_lock();
      netdev_lock(bp->dev);
      netif_close(bp->dev)
        bnxt_close()->bnxt_close_nic()->__bnxt_close_nic()
          while (bnxt_drv_busy(bp))
              msleep(20);

bnxt_drv_busy() tests BNXT_STATE_IN_SP_TASK. Normally only the sp_task
clears that bit, and the sp_task is the thread stuck in this loop.

Would this spin forever with rtnl_mutex and the instance lock held?

CPU2 can wait on netdev_lock for the whole time the func_qcfg HWRM call is
timing out, so the window may be wide.

The commit message says:

    BNXT_STATE_IN_FW_RESET stays set across the lock switch, so a
    concurrent close marks the reset aborted as it does today and a
    concurrent recovery request backs off.

The back-off path goes through bnxt_unlock_sp(). Isn't that the code that
sets the bit causing the hang?

It is older behavior that bnxt_unlock_sp() and bnxt_rtnl_unlock_sp() set
IN_SP_TASK no matter who the caller is. The comment in bnxt_lock_sp()
assumes the caller is always bnxt_sp_task(). The new window is what makes
the problem reachable.

>  			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
>  			netif_close(bp->dev);
> -			goto fw_reset_exit;
> +			bnxt_rtnl_unlock_sp(bp);
> +			return;

[Severity: Medium]
This is a pre-existing issue, but this path returns without calling
bnxt_ulp_start(). Nothing undoes the bnxt_ulp_stop() at the top of
bnxt_fw_reset(). Does this leave the RoCE/aux driver suspended after the
abort?

The old goto fw_reset_exit had the same problem. Neither bnxt_close() nor
a later bnxt_open() resumes the ULP.

bnxt_hwrm_if_change() only queues BNXT_RESTART_ULP_SP_EVENT when
BNXT_STATE_FW_RESET_DET is set. That won't happen if the func_qcfg failure
was transient.

The matching abort for the POLL_VF state in bnxt_fw_reset_task() reaches
ulp_start with rc == 0, so it does restart the ULP:

ulp_start:
	if (!rc)
		bnxt_ulp_start(bp);

Since this path is being rewritten anyway, could the ULP be restarted here
too?

>  		} else if (n > 0) {
>  			u16 vf_tmo_dsecs = n * 10;
>  

[ ... ]

> @@ -15682,9 +15688,11 @@ static void bnxt_fw_reset_task(struct work_struct *work)
>  		netdev_err(bp->dev, "fw_health_status 0x%x\n", sts);
>  	}
>  fw_reset_abort:
> +	rtnl_lock();
>  	netdev_lock(bp->dev);
>  	bnxt_fw_reset_abort(bp, rc);
>  	netdev_unlock(bp->dev);
> +	rtnl_unlock();

[Severity: High]
This isn't a bug introduced by this patch, but commit 004b5008016a seems
to have left one more netif_close() call without RTNL that this patch does
not cover. bnxt_vf_reps_destroy() holds only the instance lock:

drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:bnxt_vf_reps_destroy() {
    ...
	netdev_lock(bp->dev);
	if (netif_running(bp->dev)) {
		bnxt_close_nic(bp, false, false);
		closed = true;
	}
    ...
		bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
		bnxt_open_nic(bp, false, false);
    ...
}

bnxt_open_nic() calls netif_close() on any failure. That includes the
immediate -EIO when BNXT_STATE_ABORT_ERR is set:

	if (rc) {
		netdev_err(bp->dev, "nic open fail (rc: %x)\n", rc);
		netif_close(bp->dev);
	}

There are two callers, and both hold only the devlink instance lock:

- bnxt_dl_eswitch_mode_set() for DEVLINK_ESWITCH_MODE_LEGACY
- __bnxt_sriov_disable()

Wouldn't this hit the same ASSERT_RTNL() in __dev_close_many()? Wouldn't
it also run the NETDEV_GOING_DOWN/NETDEV_DOWN notifiers without RTNL?

If this patch is backported to stable on its own, this site from the same
Fixes commit would still be there.

[Severity: High]
This is also a pre-existing issue, but the same netif_close() without RTNL
looks reachable through the TPH queue restart path. bnxt_tph_update()
runs from bnxt_sp_task() under bnxt_lock_sp(), which takes only the
instance lock:

bnxt_sp_task()
  bnxt_tph_update()
    bnxt_lock_sp()
    netdev_rx_queue_restart()
      netdev_rx_queue_reconfig()   /* only asserts the instance lock */
        bnxt_queue_start()

bnxt_queue_start() closes the device on its error path. This happens if
any of these fail: bnxt_hwrm_rx_ring_alloc(),
bnxt_hwrm_cp_ring_alloc_p5(), bnxt_hwrm_rx_agg_ring_alloc() or
bnxt_tx_queue_start().

err_reset:
	netdev_err(bp->dev, "Unexpected HWRM error during queue start rc: %d\n",
		   rc);
	napi_enable_locked(&bnapi->napi);
	bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons);
	netif_close(dev);

Can this trigger the same ASSERT_RTNL() warnings when the HWRM calls fail
on unhealthy firmware?

>  ulp_start:
>  	if (!rc)
>  		bnxt_ulp_start(bp);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103042210.2984373-1-bcochrane%40google.com

Re: [PATCH net v2] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Bryan Cochrane]

On Tue, 3 Nov 2026, <netdev-bot> wrote:
> - [High] New deadlock in bnxt_fw_reset().
[...]
> CPU1
>       rtnl_lock();
>       netdev_lock(bp->dev);
>       netif_close(bp->dev)
>         bnxt_close()->bnxt_close_nic()->__bnxt_close_nic()
>           while (bnxt_drv_busy(bp))
>               msleep(20);
>
> bnxt_drv_busy() tests BNXT_STATE_IN_SP_TASK. Normally only the sp_task
> clears that bit, and the sp_task is the thread stuck in this loop.
>
> Would this spin forever with rtnl_mutex and the instance lock held?

Yes, this is a valid finding, thanks. bnxt_fw_recover() can enter
bnxt_fw_reset() from the devlink health recover path as soon as the
reporter is in the error state, and bnxt_unlock_sp() then sets
BNXT_STATE_IN_SP_TASK although the caller is not the sp_task. Before v2
the instance lock was held across netif_close(), so this could not
interleave; the unlock/relock window in v2 makes it reachable, and the
thread that would spin in __bnxt_close_nic() is the sp_task itself, so
nothing would ever clear the bit.

v3 will not drop and retake the locks in bnxt_fw_reset() at all. The
bnxt_get_registered_vfs() failure sets fw_reset_state to
BNXT_FW_RESET_STATE_ABORT and queues bnxt_fw_reset_task(), which then
performs the abort through the fw_reset_abort path that already takes
RTNL before the instance lock. bnxt_fw_reset() keeps the instance lock
from bnxt_lock_sp() to bnxt_unlock_sp() as before, with no netif_close()
in it.

The unconditional set_bit(BNXT_STATE_IN_SP_TASK) in bnxt_unlock_sp()
when bnxt_fw_reset()/bnxt_fw_exception() are entered via devlink is
pre-existing and not specific to this fix; I'd rather address it in a
separate patch.

> [Severity: Medium]
> This is a pre-existing issue, but this path returns without calling
> bnxt_ulp_start(). Nothing undoes the bnxt_ulp_stop() at the top of
> bnxt_fw_reset(). Does this leave the RoCE/aux driver suspended after the
> abort?
[...]
> Since this path is being rewritten anyway, could the ULP be restarted here
> too?

Agreed. With the abort routed through bnxt_fw_reset_task() in v3, rc is
0 at the ulp_start label and bnxt_ulp_start() runs, the same as for the
POLL_VF abort.

> [Severity: High]
> This isn't a bug introduced by this patch, but commit 004b5008016a seems
> to have left one more netif_close() call without RTNL that this patch does
> not cover. bnxt_vf_reps_destroy() holds only the instance lock:
[...]
> If this patch is backported to stable on its own, this site from the same
> Fixes commit would still be there.

Agreed that bnxt_open_nic() -> netif_close() under bnxt_vf_reps_destroy()
has the same problem, but it is a different call path (devlink eswitch
mode change and SR-IOV disable) with its own locking context, so I'd
prefer to keep it out of this fix and handle it separately. The two
fixes can be backported independently.

> [Severity: High]
> This is also a pre-existing issue, but the same netif_close() without RTNL
> looks reachable through the TPH queue restart path. bnxt_tph_update()
> runs from bnxt_sp_task() under bnxt_lock_sp(), which takes only the
> instance lock:
[...]
> Can this trigger the same ASSERT_RTNL() warnings when the HWRM calls fail
> on unhealthy firmware?

Also pre-existing and known: the netif_close() in the bnxt_queue_start()
error path was added by commit 325eb217e41f ("bnxt_en: bring back
rtnl_lock() in the bnxt_open() path") precisely because that path does
not hold RTNL. It is outside the scope of this patch.

pw-bot: cr


On Tue, 3 Nov 2026 at 20:25, <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (2) · Medium: 0 (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).
>
> - [High] New deadlock in bnxt_fw_reset().
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the same Fixes
>   commit (004b5008016a) left another netif_close()-without-RTNL site…
> - [High] This problem wasn't introduced by this patch, but the same
>   netif_close()-without-RTNL bug is still reachable through the TPH…
> - [Medium] This problem wasn't introduced by this patch, but the
>   bnxt_get_registered_vfs() failure path in bnxt_fw_reset() leaves the…
>
> --
>
> bnxt_en: Take RTNL before netif_close() when aborting FW reset
>
> This patch takes RTNL before the netdev instance lock on the FW reset
> abort paths in bnxt_fw_reset_task(). It does the same on the
> bnxt_get_registered_vfs() failure path in bnxt_fw_reset(), so that
> netif_close() runs with RTNL held. The goal is to fix the ASSERT_RTNL()
> warnings introduced by commit 004b5008016a ("eth: bnxt: remove most
> dependencies on RTNL").
>
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 33c4cacc8ffe3..1ea1e8266eabf 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -14860,9 +14860,16 @@ void bnxt_fw_reset(struct bnxt *bp)
> >               if (n < 0) {
> >                       netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
> >                                  n);
> > +                     /* netif_close() requires RTNL, which must be taken
> > +                      * before the netdev instance lock.
> > +                      */
> > +                     netdev_unlock(bp->dev);
> > +                     rtnl_lock();
> > +                     netdev_lock(bp->dev);
>
> [Severity: High]
> Can a concurrent devlink health recover hang the sp_task here while it
> holds RTNL?
>
> bnxt_sp_task() is not the only caller of bnxt_fw_reset(). When
> devlink_health_report() returns -ECANCELED,
> bnxt_devlink_health_fw_report() calls __bnxt_fw_recover() from the sp_task
> without the devlink lock.
>
> At that point the reporter is already in the error state. A userspace
> "devlink health recover" can then reach bnxt_fw_reset() through
> bnxt_fw_recover() at the same time.
>
> Before this patch the instance lock was held all the way through
> netif_close(). The new unlock/relock window seems to allow this ordering:
>
> CPU1 (bnxt_sp_task)
> bnxt_devlink_health_fw_report()
>   __bnxt_fw_recover()
>     bnxt_fw_reset()
>       bnxt_lock_sp()          /* clears IN_SP_TASK */
>       set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
>       bnxt_get_registered_vfs() returns < 0
>       netdev_unlock(bp->dev);
>
> CPU2 (devlink health recover)
> devlink_health_reporter_recover()
>   bnxt_fw_recover()
>     __bnxt_fw_recover()
>       bnxt_fw_reset()
>         bnxt_lock_sp()        /* now gets the instance lock */
>         IN_FW_RESET is set, nothing to do
>         bnxt_unlock_sp()
>           set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
>           netdev_unlock(bp->dev);
>
> CPU1
>       rtnl_lock();
>       netdev_lock(bp->dev);
>       netif_close(bp->dev)
>         bnxt_close()->bnxt_close_nic()->__bnxt_close_nic()
>           while (bnxt_drv_busy(bp))
>               msleep(20);
>
> bnxt_drv_busy() tests BNXT_STATE_IN_SP_TASK. Normally only the sp_task
> clears that bit, and the sp_task is the thread stuck in this loop.
>
> Would this spin forever with rtnl_mutex and the instance lock held?
>
> CPU2 can wait on netdev_lock for the whole time the func_qcfg HWRM call is
> timing out, so the window may be wide.
>
> The commit message says:
>
>     BNXT_STATE_IN_FW_RESET stays set across the lock switch, so a
>     concurrent close marks the reset aborted as it does today and a
>     concurrent recovery request backs off.
>
> The back-off path goes through bnxt_unlock_sp(). Isn't that the code that
> sets the bit causing the hang?
>
> It is older behavior that bnxt_unlock_sp() and bnxt_rtnl_unlock_sp() set
> IN_SP_TASK no matter who the caller is. The comment in bnxt_lock_sp()
> assumes the caller is always bnxt_sp_task(). The new window is what makes
> the problem reachable.
>
> >                       clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
> >                       netif_close(bp->dev);
> > -                     goto fw_reset_exit;
> > +                     bnxt_rtnl_unlock_sp(bp);
> > +                     return;
>
> [Severity: Medium]
> This is a pre-existing issue, but this path returns without calling
> bnxt_ulp_start(). Nothing undoes the bnxt_ulp_stop() at the top of
> bnxt_fw_reset(). Does this leave the RoCE/aux driver suspended after the
> abort?
>
> The old goto fw_reset_exit had the same problem. Neither bnxt_close() nor
> a later bnxt_open() resumes the ULP.
>
> bnxt_hwrm_if_change() only queues BNXT_RESTART_ULP_SP_EVENT when
> BNXT_STATE_FW_RESET_DET is set. That won't happen if the func_qcfg failure
> was transient.
>
> The matching abort for the POLL_VF state in bnxt_fw_reset_task() reaches
> ulp_start with rc == 0, so it does restart the ULP:
>
> ulp_start:
>         if (!rc)
>                 bnxt_ulp_start(bp);
>
> Since this path is being rewritten anyway, could the ULP be restarted here
> too?
>
> >               } else if (n > 0) {
> >                       u16 vf_tmo_dsecs = n * 10;
> >
>
> [ ... ]
>
> > @@ -15682,9 +15688,11 @@ static void bnxt_fw_reset_task(struct work_struct *work)
> >               netdev_err(bp->dev, "fw_health_status 0x%x\n", sts);
> >       }
> >  fw_reset_abort:
> > +     rtnl_lock();
> >       netdev_lock(bp->dev);
> >       bnxt_fw_reset_abort(bp, rc);
> >       netdev_unlock(bp->dev);
> > +     rtnl_unlock();
>
> [Severity: High]
> This isn't a bug introduced by this patch, but commit 004b5008016a seems
> to have left one more netif_close() call without RTNL that this patch does
> not cover. bnxt_vf_reps_destroy() holds only the instance lock:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:bnxt_vf_reps_destroy() {
>     ...
>         netdev_lock(bp->dev);
>         if (netif_running(bp->dev)) {
>                 bnxt_close_nic(bp, false, false);
>                 closed = true;
>         }
>     ...
>                 bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>                 bnxt_open_nic(bp, false, false);
>     ...
> }
>
> bnxt_open_nic() calls netif_close() on any failure. That includes the
> immediate -EIO when BNXT_STATE_ABORT_ERR is set:
>
>         if (rc) {
>                 netdev_err(bp->dev, "nic open fail (rc: %x)\n", rc);
>                 netif_close(bp->dev);
>         }
>
> There are two callers, and both hold only the devlink instance lock:
>
> - bnxt_dl_eswitch_mode_set() for DEVLINK_ESWITCH_MODE_LEGACY
> - __bnxt_sriov_disable()
>
> Wouldn't this hit the same ASSERT_RTNL() in __dev_close_many()? Wouldn't
> it also run the NETDEV_GOING_DOWN/NETDEV_DOWN notifiers without RTNL?
>
> If this patch is backported to stable on its own, this site from the same
> Fixes commit would still be there.
>
> [Severity: High]
> This is also a pre-existing issue, but the same netif_close() without RTNL
> looks reachable through the TPH queue restart path. bnxt_tph_update()
> runs from bnxt_sp_task() under bnxt_lock_sp(), which takes only the
> instance lock:
>
> bnxt_sp_task()
>   bnxt_tph_update()
>     bnxt_lock_sp()
>     netdev_rx_queue_restart()
>       netdev_rx_queue_reconfig()   /* only asserts the instance lock */
>         bnxt_queue_start()
>
> bnxt_queue_start() closes the device on its error path. This happens if
> any of these fail: bnxt_hwrm_rx_ring_alloc(),
> bnxt_hwrm_cp_ring_alloc_p5(), bnxt_hwrm_rx_agg_ring_alloc() or
> bnxt_tx_queue_start().
>
> err_reset:
>         netdev_err(bp->dev, "Unexpected HWRM error during queue start rc: %d\n",
>                    rc);
>         napi_enable_locked(&bnapi->napi);
>         bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons);
>         netif_close(dev);
>
> Can this trigger the same ASSERT_RTNL() warnings when the HWRM calls fail
> on unhealthy firmware?
>
> >  ulp_start:
> >       if (!rc)
> >               bnxt_ulp_start(bp);
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103042210.2984373-1-bcochrane%40google.com