linux-kernel - Re: [patch] net, bcmgenet: Fix locking of netpoll facing functions

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: <9b69b764-5691-4bcd-8784-eb657f13a3a4@broadcom.com>
Date: Thu, 28 Aug 2025 11:59:15 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Mike Galbraith <efault@....de>, lkml <linux-kernel@...r.kernel.org>
Cc: Doug Berger <opendmb@...il.com>, Breno Leitao <leitao@...ian.org>
Subject: Re: [patch] net, bcmgenet: Fix locking of netpoll facing functions

On 8/26/25 01:24, Mike Galbraith wrote:
> Lockdep reports ring->lock to not be irq safe during netpoll/netconsole
> session, resulting in a potential deadlock scenario.
> 
>   Chain exists of:
>     &host->lock --> target_list_lock --> &ring->lock
>    Possible interrupt unsafe locking scenario:
>          CPU0                    CPU1
>          ----                    ----
>     lock(&ring->lock);
>                                  local_irq_disable();
>                                  lock(&host->lock);
>                                  lock(target_list_lock);
>     <Interrupt>
>       lock(&host->lock);
>    *** DEADLOCK ***
> 
> Prevent that via use of irqsave/restore spinlock variant when polling.
> 
> Signed-off-by: Mike Galbraith <efault@....de>

Your patch did not make it to the adequate mailing list which should be 
at least netdev@...r.kernel.org. This is effectively a partial revert of 
b0447ecb533270cf857ebee1133cb8ff67115423 ("net: bcmgenet: relax lock 
constraints to reduce IRQ latency") therefore I would want Doug to chime 
in and review this.

Thanks!

> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c |   10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2022,14 +2022,15 @@ static int bcmgenet_tx_poll(struct napi_
>   		container_of(napi, struct bcmgenet_tx_ring, napi);
>   	unsigned int work_done = 0;
>   	struct netdev_queue *txq;
> +	unsigned long flags;
>   
> -	spin_lock(&ring->lock);
> +	spin_lock_irqsave(&ring->lock, flags);
>   	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
>   	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
>   		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>   		netif_tx_wake_queue(txq);
>   	}
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
>   
>   	if (work_done == 0) {
>   		napi_complete(napi);
> @@ -2128,6 +2129,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   	struct bcmgenet_tx_ring *ring = NULL;
>   	struct enet_cb *tx_cb_ptr;
>   	struct netdev_queue *txq;
> +	unsigned long flags;
>   	int nr_frags, index;
>   	dma_addr_t mapping;
>   	unsigned int size;
> @@ -2149,7 +2151,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   
>   	nr_frags = skb_shinfo(skb)->nr_frags;
>   
> -	spin_lock(&ring->lock);
> +	spin_lock_irqsave(&ring->lock, flags);
>   	if (ring->free_bds <= (nr_frags + 1)) {
>   		if (!netif_tx_queue_stopped(txq))
>   			netif_tx_stop_queue(txq);
> @@ -2239,7 +2241,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   		bcmgenet_tdma_ring_writel(priv, ring->index,
>   					  ring->prod_index, TDMA_PROD_INDEX);
>   out:
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
>   
>   	return ret;
>   
> 


-- 
Florian