netdev - Re: [PATCH] bnx2x: Fix error recovering in switch configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Aug 2022 18:44:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Thinh Tran <thinhtr@...ux.vnet.ibm.com>
Cc:     netdev@...r.kernel.org, aelior@...vell.com, davem@...emloft.net,
        manishc@...vell.com, skalluru@...vell.com
Subject: Re: [PATCH] bnx2x: Fix error recovering in switch configuration

On Thu, 25 Aug 2022 20:00:29 +0000 Thinh Tran wrote:
> As the BCM57810 and other I/O adapters are connected
> through a PCIe switch, the bnx2x driver causes unexpected
> system hang/crash while handling PCIe switch errors, if 
> its error handler is called after other drivers' handlers.
> 
> In this case, after numbers of bnx2x_tx_timout(), the
> bnx2x_nic_unload() is  called, frees up resources and
> calls bnx2x_napi_disable(). Then when EEH calls its
> error handler, the bnx2x_io_error_detected() and
> bnx2x_io_slot_reset() also calling bnx2x_napi_disable()
> and freeing the resources.
> 
> This patch will:
> - reduce the numbers of bnx2x_panic_dump() while in
>   bnx2x_tx_timeout(), avoid filling up dmesg buffer.
> - use checking new napi_enable flags to prevent calling 
>   disable again which causing system hangs.
> - cheking if fp->page_pool already freed avoid system
>   crash.

> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index 712b5595bc39..bb8d91f44642 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)
>  static void bnx2x_napi_enable_cnic(struct bnx2x *bp)
>  {
>  	int i;
> +	if (bp->cnic_napi_enable)

empty line between variables and code, pls

> +		return;
>  
>  	for_each_rx_queue_cnic(bp, i) {
>  		napi_enable(&bnx2x_fp(bp, i, napi));
>  	}
> +	bp->cnic_napi_enable = true;

The concept of not calling napi_enable() / disable()
feels a little wrong. It's the state of the driver,
not the NAPI that's the problem so perhaps you could
a appropriately named bool for that (IDK, maybe 
nic_stopped) and prevent coming into the NAPI handling
functions completely?

Is all other code in the driver on the path in question 
really idempotent?

>  }
>  
>  static void bnx2x_napi_enable(struct bnx2x *bp)
>  {
>  	int i;
> +	if (bp->napi_enable)
> +		return;
>  
>  	for_each_eth_queue(bp, i) {
>  		napi_enable(&bnx2x_fp(bp, i, napi));
>  	}
> +	bp->napi_enable = true;
>  }
>  
>  static void bnx2x_napi_disable_cnic(struct bnx2x *bp)
>  {
>  	int i;
> +	if (!bp->cnic_napi_enable)
> +		return;
>  
>  	for_each_rx_queue_cnic(bp, i) {
>  		napi_disable(&bnx2x_fp(bp, i, napi));
>  	}
> +	bp->cnic_napi_enable = false;
>  }
>  
>  static void bnx2x_napi_disable(struct bnx2x *bp)
>  {
>  	int i;
> +	if (!bp->napi_enable)
> +		return;
>  
>  	for_each_eth_queue(bp, i) {
>  		napi_disable(&bnx2x_fp(bp, i, napi));
>  	}
> +	bp->napi_enable = false;
>  }
>  
>  void bnx2x_netif_start(struct bnx2x *bp)
> @@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp)
>  	}
>  
>  	/* Add all CNIC NAPI objects */
> +	bp->cnic_napi_enable = false;
>  	bnx2x_add_all_napi_cnic(bp);
>  	DP(NETIF_MSG_IFUP, "cnic napi added\n");
>  	bnx2x_napi_enable_cnic(bp);
> @@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
>  	 */
>  	bnx2x_setup_tc(bp->dev, bp->max_cos);
>  
> +	bp->tx_timeout_cnt = 0;
>  	/* Add all NAPI objects */
> +	bp->napi_enable = false;
>  	bnx2x_add_all_napi(bp);
>  	DP(NETIF_MSG_IFUP, "napi added\n");
>  	bnx2x_napi_enable(bp);
> @@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue)
>  	 */
>  	if (!bp->panic)
>  #ifndef BNX2X_STOP_ON_ERROR
> -		bnx2x_panic_dump(bp, false);
> +	{
> +		if (++bp->tx_timeout_cnt > 3) {
> +			bnx2x_panic_dump(bp, false);
> +			bp->tx_timeout_cnt = 0;
> +		} else {
> +			netdev_err(bp->dev, "TX timeout %d times\n", bp->tx_timeout_cnt);
> +		}
> +	}

Please put this code in a helper function so that the oddly looking
brackets are not needed.

>  #else
>  		bnx2x_panic();
>  #endif
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> index d8b1824c334d..7e1d38a2c7ec 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> @@ -1018,6 +1018,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp,
>  	if (fp->mode == TPA_MODE_DISABLED)
>  		return;
>  
> +	if (!fp->page_pool.page)
> +		return;

See, another thing that's not idempotent. Better to bail higher up,
in the callee.

>  	for (i = 0; i < last; i++)
>  		bnx2x_free_rx_sge(bp, fp, i);
>