netdev - Re: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation

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: <1290529777.3046.99.camel@edumazet-laptop>
Date:	Tue, 23 Nov 2010 17:29:37 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Vladislav Zolotarov <vladz@...adcom.com>
Cc:	Dave Miller <davem@...emloft.net>,
	Eilon Greenstein <eilong@...adcom.com>,
	netdev list <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] bnx2x: Resolving a possible dead-lock
 situation

Le mardi 23 novembre 2010 à 18:15 +0200, Vladislav Zolotarov a écrit :
> There is a possible dead-lock situation between sch_direct_xmit() 
> (called from soft_IRQ context) and bnx2x_tx_int() when called from 
> an ethtool self-test flow (syscall context).
> 
> To prevent a dead-lock, disable bottom-halves on a local CPU when taking
> a tx_lock from bnx2x_tx_int() (use __netif_tx_lock_bh(txq)).
> 
> The flow in the bnx2x_tx_int() where tx_lock is taken should be hit
> very rarely thus performance penalty of this change should be minimal.
> 
> Signed-off-by: Vladislav Zolotarov <vladz@...adcom.com>
> Signed-off-by: Eilon Greenstein <eilong@...adcom.com>
> ---
>  drivers/net/bnx2x/bnx2x_cmn.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index 94d5f59..5189788 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -144,14 +144,14 @@ int bnx2x_tx_int(struct bnx2x_fastpath *fp)
>  		 * stops the queue
>  		 */
>  
> -		__netif_tx_lock(txq, smp_processor_id());
> +		__netif_tx_lock_bh(txq);
>  
>  		if ((netif_tx_queue_stopped(txq)) &&
>  		    (bp->state == BNX2X_STATE_OPEN) &&
>  		    (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3))
>  			netif_tx_wake_queue(txq);
>  
> -		__netif_tx_unlock(txq);
> +		__netif_tx_unlock_bh(txq);
>  	}
>  	return 0;
>  }

That seems strange. Even if performance penalty is not minimal, it
should be avoided.

If problem comes from ethtool, why not preventing BH in ethtool itself ?

diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index d02ffbd..1f7d4e6 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1499,9 +1499,11 @@ static int bnx2x_run_loopback(struct bnx2x *bp, int loopback_mode, u8 link_up)
 	 * updates that have been performed while interrupts were
 	 * disabled.
 	 */
-	if (bp->common.int_block == INT_BLOCK_IGU)
+	if (bp->common.int_block == INT_BLOCK_IGU) {
+		local_bh_disable();
 		bnx2x_tx_int(fp_tx);
-
+		local_bh_enable();
+	}
 	rx_idx = le16_to_cpu(*fp_rx->rx_cons_sb);
 	if (rx_idx != rx_start_idx + num_pkts)
 		goto test_loopback_exit;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html