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

[netdev-bot+sashiko@kernel.org]

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