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:   Tue, 30 Aug 2022 11:15:32 -0500
From:   Thinh Tran <thinhtr@...ux.vnet.ibm.com>
To:     Jakub Kicinski <kuba@...nel.org>
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

Thanks for reviewing it.

On 8/26/2022 8:44 PM, Jakub Kicinski wrote:
> 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
> 
Will correct it.

>> +		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?
> 

For the states of the driver, it already has bnx2x_netif_start()
and bnx2x_netif_stop() to handle NAPI functions.
But the bnx2x_nic_load() directly calls bnx2x_napi_enable() and 
_disable() in case of errors, and is being called from various functions 
while the driver in different states.

I will work with the suggestion.


>>   }
>>   
>>   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.
>

Will correct it.

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


Thinh Tran