netdev - Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [8/8]

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] [day] [month] [year] [list]

Message-ID: <20070719144502.GG12892@xyzzy.farnsworth.org>
Date:	Thu, 19 Jul 2007 07:45:02 -0700
From:	"Dale Farnsworth" <dale@...nsworth.org>
To:	"Steven J. Hill" <sjhill@...litydiluted.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [8/8]

On Thu, Jul 19, 2007 at 05:02:04AM +0000, Steven J. Hill wrote:
> Fix long standing panic with regards to descriptors and locking.
> 
> Signed-off-by: Steven J. Hill <sjhill1@...kwellcollins.com>
> ---
> 
> diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
> --- linux-2.6.22.1/drivers/net/mv643xx_eth.c	2007-07-18 22:55:11.000000000 -0500
> +++ linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c	2007-07-18 22:39:37.000000000 -0500
> @@ -350,13 +350,6 @@
>  
>  	while (mp->tx_desc_count > 0) {
>  		spin_lock_irqsave(&mp->lock, flags);
> -
> -		/* tx_desc_count might have changed before acquiring the lock */
> -		if (mp->tx_desc_count <= 0) {
> -			spin_unlock_irqrestore(&mp->lock, flags);
> -			return released;
> -		}
> -

The code you remove above is needed to protect against a nasty bug.
It was added in January and needs to stay in.

> @@ -367,7 +360,7 @@
>  
>  		if (!force && (cmd_sts & ETH_BUFFER_OWNED_BY_DMA)) {
>  			spin_unlock_irqrestore(&mp->lock, flags);
> -			return released;
> +			continue;
>  		}

No.  Continuing at that point results in the driver spinning, wasting
cpu while polling for the hardware to release the DMA descriptor.

> @@ -1488,7 +1481,19 @@
>  	BUG_ON(netif_queue_stopped(dev));
>  	BUG_ON(skb == NULL);
>  
> -	if (mp->tx_ring_size - mp->tx_desc_count < MAX_DESCS_PER_SKB) {
> +	if (mp->tx_ring_size - mp->tx_desc_count <= MAX_DESCS_PER_SKB) {
> +		/*
> +		 * We are completely out of TX descriptors, however,
> +		 * if 'tx_used_desc_q' is zero, most likely the port
> +		 * has been configured and is up, but there is no link.
> +		 * We attempt to free a single descriptor to keep the
> +		 * 'sendto' call and rest of the stack happy. If this
> +		 * check is taken out, expect an error and kernel panic
> +		 * from 'kernel/softirq.c:141' inside 'local_bh_enable'.
> +		 */
> +		if (mp->tx_used_desc_q == 0)
> +			mv643xx_eth_free_all_tx_descs(dev);

I don't understand the bug, but regardless, we need a better fix.
Freeing all the descriptors when the HW xmit queue is full is not
a solution.

-Dale
-
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