netdev - Re: [PATCH net v2 01/12] net/sonic: Add mutual exclusion for accessing shared state

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]

Date:   Tue, 21 Jan 2020 14:19:51 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Finn Thain <fthain@...egraphics.com.au>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Chris Zankel <chris@...kel.net>,
        Laurent Vivier <laurent@...ier.eu>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2 01/12] net/sonic: Add mutual exclusion for
 accessing shared state



On 1/21/20 1:22 PM, Finn Thain wrote:
> The netif_stop_queue() call in sonic_send_packet() races with the
> netif_wake_queue() call in sonic_interrupt(). This causes issues
> like "NETDEV WATCHDOG: eth0 (macsonic): transmit queue 0 timed out".
> Fix this by disabling interrupts when accessing tx_skb[] and next_tx.
> Update a comment to clarify the synchronization properties.
> 
> Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
> Tested-by: Stan Johnson <userm57@...oo.com>
> Signed-off-by: Finn Thain <fthain@...egraphics.com.au>

> @@ -284,9 +287,16 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  	struct net_device *dev = dev_id;
>  	struct sonic_local *lp = netdev_priv(dev);
>  	int status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&lp->lock, flags);


This is a hard irq handler, no need to block hard irqs.

spin_lock() here is enough.

> +
> +	status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> +	if (!status) {
> +		spin_unlock_irqrestore(&lp->lock, flags);
>  
> -	if (!(status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT))
>  		return IRQ_NONE;
> +	}
>  
>  	do {
>  		if (status & SONIC_INT_PKTRX) {
> @@ -300,11 +310,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  			int td_status;
>  			int freed_some = 0;
>  
> -			/* At this point, cur_tx is the index of a TD that is one of:
> -			 *   unallocated/freed                          (status set   & tx_skb[entry] clear)
> -			 *   allocated and sent                         (status set   & tx_skb[entry] set  )
> -			 *   allocated and not yet sent                 (status clear & tx_skb[entry] set  )
> -			 *   still being allocated by sonic_send_packet (status clear & tx_skb[entry] clear)
> +			/* The state of a Transmit Descriptor may be inferred
> +			 * from { tx_skb[entry], td_status } as follows.
> +			 * { clear, clear } => the TD has never been used
> +			 * { set,   clear } => the TD was handed to SONIC
> +			 * { set,   set   } => the TD was handed back
> +			 * { clear, set   } => the TD is available for re-use
>  			 */
>  
>  			netif_dbg(lp, intr, dev, "%s: tx done\n", __func__);
> @@ -406,7 +417,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  		/* load CAM done */
>  		if (status & SONIC_INT_LCD)
>  			SONIC_WRITE(SONIC_ISR, SONIC_INT_LCD); /* clear the interrupt */
> -	} while((status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT));
> +
> +		status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> +	} while (status);
> +
> +	spin_unlock_irqrestore(&lp->lock, flags);
> +
>  	return IRQ_HANDLED;