linux-kernel - Re: [PATCH] net: macb: Fix tx_ptr

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: <794443b3-2f6d-4400-8844-6c8cdffd3be2@linux.dev>
Date: Thu, 28 Aug 2025 12:01:34 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: Robert Hancock <robert.hancock@...ian.com>, Mike Galbraith
 <efault@....de>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 linux-kernel@...r.kernel.org, Nicolas Ferre <nicolas.ferre@...rochip.com>
Subject: Re: [PATCH] net: macb: Fix tx_ptr_lock locking

On 8/28/25 12:00, Sean Anderson wrote:
> macb_start_xmit can be called with bottom-halves disabled (e.g.
> transmitting from softirqs) as well as with interrupts disabled (with
> netpoll). Because of this, all other functions taking tx_ptr_lock must
> disable IRQs, and macb_start_xmit must only re-enable IRQs if they
> were already enabled.
> 
> Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
> Reported-by: Mike Galbraith <efault@....de>
> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> ---
> 
>  drivers/net/ethernet/cadence/macb_main.c | 25 ++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 16d28a8b3b56..b0a8dfa341ea 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1228,7 +1228,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	int packets = 0;
>  	u32 bytes = 0;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  	head = queue->tx_head;
>  	for (tail = queue->tx_tail; tail != head && packets < budget; tail++) {
>  		struct macb_tx_skb	*tx_skb;
> @@ -1291,7 +1291,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	    CIRC_CNT(queue->tx_head, queue->tx_tail,
>  		     bp->tx_ring_size) <= MACB_TX_WAKEUP_THRESH(bp))
>  		netif_wake_subqueue(bp->dev, queue_index);
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  
>  	return packets;
>  }
> @@ -1708,7 +1708,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>  	struct macb *bp = queue->bp;
>  	unsigned int head_idx, tbqp;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  
>  	if (queue->tx_head == queue->tx_tail)
>  		goto out_tx_ptr_unlock;
> @@ -1720,19 +1720,19 @@ static void macb_tx_restart(struct macb_queue *queue)
>  	if (tbqp == head_idx)
>  		goto out_tx_ptr_unlock;
>  
> -	spin_lock_irq(&bp->lock);
> +	spin_lock(&bp->lock);
>  	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
> -	spin_unlock_irq(&bp->lock);
> +	spin_unlock(&bp->lock);
>  
>  out_tx_ptr_unlock:
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  }
>  
>  static bool macb_tx_complete_pending(struct macb_queue *queue)
>  {
>  	bool retval = false;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  	if (queue->tx_head != queue->tx_tail) {
>  		/* Make hw descriptor updates visible to CPU */
>  		rmb();
> @@ -1740,7 +1740,7 @@ static bool macb_tx_complete_pending(struct macb_queue *queue)
>  		if (macb_tx_desc(queue, queue->tx_tail)->ctrl & MACB_BIT(TX_USED))
>  			retval = true;
>  	}
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  	return retval;
>  }
>  
> @@ -2308,6 +2308,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct macb_queue *queue = &bp->queues[queue_index];
>  	unsigned int desc_cnt, nr_frags, frag_size, f;
>  	unsigned int hdrlen;
> +	unsigned long flags;
>  	bool is_lso;
>  	netdev_tx_t ret = NETDEV_TX_OK;
>  
> @@ -2368,7 +2369,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		desc_cnt += DIV_ROUND_UP(frag_size, bp->max_tx_length);
>  	}
>  
> -	spin_lock_bh(&queue->tx_ptr_lock);
> +	spin_lock_irqsave(&queue->tx_ptr_lock, flags);
>  
>  	/* This is a hard error, log it. */
>  	if (CIRC_SPACE(queue->tx_head, queue->tx_tail,
> @@ -2392,15 +2393,15 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	netdev_tx_sent_queue(netdev_get_tx_queue(bp->dev, queue_index),
>  			     skb->len);
>  
> -	spin_lock_irq(&bp->lock);
> +	spin_lock(&bp->lock);
>  	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
> -	spin_unlock_irq(&bp->lock);
> +	spin_unlock(&bp->lock);
>  
>  	if (CIRC_SPACE(queue->tx_head, queue->tx_tail, bp->tx_ring_size) < 1)
>  		netif_stop_subqueue(dev, queue_index);
>  
>  unlock:
> -	spin_unlock_bh(&queue->tx_ptr_lock);
> +	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
>  
>  	return ret;
>  }

Sorry, this should be [PATCH net].

--Sean