netdev - RE: [EXT] [PATCH v2] 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]

Message-ID: <BY3PR18MB461200F00B27327499F9FEC8AB4D9@BY3PR18MB4612.namprd18.prod.outlook.com>
Date:   Mon, 19 Sep 2022 13:53:14 +0000
From:   Manish Chopra <manishc@...vell.com>
To:     Thinh Tran <thinhtr@...ux.vnet.ibm.com>,
        "kuba@...nel.org" <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <aelior@...vell.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Sudarsana Reddy Kalluru <skalluru@...vell.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Alok Prasad <palok@...vell.com>
Subject: RE: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch
 configuration

Hello Thinh,

> -----Original Message-----
> From: Thinh Tran <thinhtr@...ux.vnet.ibm.com>
> Sent: Saturday, September 17, 2022 1:21 AM
> To: kuba@...nel.org
> Cc: netdev@...r.kernel.org; Ariel Elior <aelior@...vell.com>;
> davem@...emloft.net; Manish Chopra <manishc@...vell.com>; Sudarsana
> Reddy Kalluru <skalluru@...vell.com>; pabeni@...hat.com;
> edumazet@...gle.com; Thinh Tran <thinhtr@...ux.vnet.ibm.com>
> Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configuration
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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.
> 
> Signed-off-by: Thinh Tran <thinhtr@...ux.vnet.ibm.com>
> 
>  v2:
>    - Check the state of the NIC before calling disable nappi
>      and freeing the IRQ
>    - Prevent recurrence of TX timeout by turning off the carrier,
>      calling netif_carrier_off() in bnx2x_tx_timeout()
>    - Check and bail out early if fp->page_pool already freed
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  2 +
>  .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c   | 27 +++++++++----
>  .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h   |  3 ++
>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 38 +++++++++----------
> .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c  | 17 +++++----
>  5 files changed, 53 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index dd5945c4bfec..11280f0eb75b 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -1509,6 +1509,8 @@ struct bnx2x {
>  	bool			cnic_loaded;
>  	struct cnic_eth_dev	*(*cnic_probe)(struct net_device *);
> 
> +	bool			nic_stopped;
> +

There is an already 'state' variable which holds the NIC state whether it was opened or closed.
Perhaps we could use that easily in bnx2x_io_slot_reset() to prevent multiple NAPI disablement
instead of adding a newer one. Please see below for ex -

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 962253db25b8..c8e9b47208ed 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev)
                }
                bnx2x_drain_tx_queues(bp);
                bnx2x_send_unload_req(bp, UNLOAD_RECOVERY);
-               bnx2x_netif_stop(bp, 1);
-               bnx2x_del_all_napi(bp);

-               if (CNIC_LOADED(bp))
-                       bnx2x_del_all_napi_cnic(bp);
+               if (bp->state == BNX2X_STATE_OPEN) {
+                       bnx2x_netif_stop(bp, 1);
+                       bnx2x_del_all_napi(bp);

-               bnx2x_free_irq(bp);
+                       if (CNIC_LOADED(bp))
+                               bnx2x_del_all_napi_cnic(bp);
+
+                       bnx2x_free_irq(bp);
+               }

                /* Report UNLOAD_DONE to MCP */
                bnx2x_send_unload_done(bp, true);

Thanks,
Manish