netdev - Re: [PATCH net-next 3/3] eth: bnxt: handle invalid Tx completions more gracefully

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: <774e2719376723595425067ab3a6f59b72c50bc2.camel@redhat.com>
Date: Tue, 11 Jul 2023 12:10:28 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, michael.chan@...adcom.com
Subject: Re: [PATCH net-next 3/3] eth: bnxt: handle invalid Tx completions
 more gracefully

On Mon, 2023-07-10 at 13:56 -0700, Jakub Kicinski wrote:
> Invalid Tx completions should never happen (tm) but when they do
> they crash the host, because driver blindly trusts that there is
> a valid skb pointer on the ring.
> 
> The completions I've seen appear to be some form of FW / HW
> miscalculation or staleness, they have typical (small) values
> (<100), but they are most often higher than number of queued
> descriptors. They usually happen after boot.
> 
> Instead of crashing print a warning and schedule a reset.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 +++++++++++++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h |  1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index b8ddad33b01a..bfa56f35d2e0 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -690,6 +690,16 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>  		skb = tx_buf->skb;
>  		tx_buf->skb = NULL;
>  
> +		if (unlikely(!skb)) {
> +			netdev_err(bp->dev, "Invalid Tx completion (ring:%d tx_pkts:%d cons:%u prod:%u i:%d)",
> +				   txr->txq_index, bnapi->tx_pkts,
> +				   txr->tx_cons, txr->tx_prod, i);
> +			WARN_ON_ONCE(1);
> +			bnapi->tx_fault = 1;
> +			bnxt_queue_sp_work(bp, BNXT_RESET_TASK_SP_EVENT);
> +			return;
> +		}
> +
>  		tx_bytes += skb->len;
>  
>  		if (tx_buf->is_push) {
> @@ -2576,7 +2586,7 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
>  
>  static void __bnxt_poll_work_done(struct bnxt *bp, struct bnxt_napi *bnapi)
>  {
> -	if (bnapi->tx_pkts) {
> +	if (bnapi->tx_pkts && !bnapi->tx_fault) {
>  		bnapi->tx_int(bp, bnapi, bnapi->tx_pkts);
>  		bnapi->tx_pkts = 0;
>  	}
> @@ -9429,6 +9439,8 @@ static void bnxt_enable_napi(struct bnxt *bp)
>  		struct bnxt_napi *bnapi = bp->bnapi[i];
>  		struct bnxt_cp_ring_info *cpr;
>  
> +		bnapi->tx_fault = 0;
> +
>  		cpr = &bnapi->cp_ring;
>  		if (bnapi->in_reset)
>  			cpr->sw_stats.rx.rx_resets++;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 080e73496066..08ce9046bfd2 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1008,6 +1008,7 @@ struct bnxt_napi {
>  					  int);
>  	int			tx_pkts;
>  	u8			events;
> +	u8			tx_fault:1;

Since there are still a few holes avail, I would use a plain u8 (or
bool) to help the compiler emit better code.

Cheers,

Paolo