[PATCH] 8139too: harden against TX ring overflow

[PATCH] 8139too: harden against TX ring overflow

[Sergei Shtylyov]

This driver's 4-packet deep TX queue is too sensible to the "careless" callers
ignoring its state (like netpoll in trapped mode), so add "queue full" check at
the start of the hard_start_xmit() method (only under #ifndef RTL8139_NDEBUG,
otherwise the queue will get stuck once dirty pointer gets out of sync); switch
to using appropriate mnemonics for the return values while at it.
Also, the out-of-sync dirty pointer check is misplaced in rtl8139_tx_interrupt()
which causes TX descriptors to be inspected more than once in case the pointer
really gets out-of-sync (and incrementing the dirty pointer always by 4 is just
not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the queue) --
place it before the loop and limit the loop to only look through 4 descriptors
at most, so that already overwritten descriptors are just not counted.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---

 drivers/net/8139too.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

Index: linux-2.6/drivers/net/8139too.c
===================================================================
--- linux-2.6.orig/drivers/net/8139too.c
+++ linux-2.6/drivers/net/8139too.c
@@ -90,7 +90,7 @@
 */
 
 #define DRV_NAME	"8139too"
-#define DRV_VERSION	"0.9.28"
+#define DRV_VERSION	"0.9.31"
 
 
 #include <linux/module.h>
@@ -1708,6 +1708,13 @@ static int rtl8139_start_xmit (struct sk
 	unsigned int len = skb->len;
 	unsigned long flags;
 
+#ifndef RTL8139_NDEBUG
+	if (unlikely((tp->cur_tx - tp->dirty_tx) >= NUM_TX_DESC)) {
+		printk(KERN_ERR "%s: TX queue full!\n", dev->name);
+		return NETDEV_TX_BUSY;
+	}
+#endif
+
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % NUM_TX_DESC;
 
@@ -1720,7 +1727,7 @@ static int rtl8139_start_xmit (struct sk
 	} else {
 		dev_kfree_skb(skb);
 		tp->stats.tx_dropped++;
-		return 0;
+		return NETDEV_TX_OK;
 	}
 
 	spin_lock_irqsave(&tp->lock, flags);
@@ -1740,7 +1747,7 @@ static int rtl8139_start_xmit (struct sk
 		printk (KERN_DEBUG "%s: Queued Tx packet size %u to slot %d.\n",
 			dev->name, len, entry);
 
-	return 0;
+	return NETDEV_TX_OK;
 }
 
 
@@ -1755,6 +1762,16 @@ static void rtl8139_tx_interrupt (struct
 
 	dirty_tx = tp->dirty_tx;
 	tx_left = tp->cur_tx - dirty_tx;
+
+#ifndef RTL8139_NDEBUG
+	if (unlikely(tx_left > NUM_TX_DESC)) {
+		printk(KERN_ERR "%s: Out-of-sync dirty pointer, %ld vs. %ld.\n",
+		       dev->name, dirty_tx, tp->cur_tx);
+		tx_left  = NUM_TX_DESC;
+		dirty_tx = tp->cur_tx - NUM_TX_DESC;
+	}
+#endif /* RTL8139_NDEBUG */
+
 	while (tx_left > 0) {
 		int entry = dirty_tx % NUM_TX_DESC;
 		int txstatus;
@@ -1797,14 +1814,6 @@ static void rtl8139_tx_interrupt (struct
 		tx_left--;
 	}
 
-#ifndef RTL8139_NDEBUG
-	if (tp->cur_tx - dirty_tx > NUM_TX_DESC) {
-		printk (KERN_ERR "%s: Out-of-sync dirty pointer, %ld vs. %ld.\n",
-		        dev->name, dirty_tx, tp->cur_tx);
-		dirty_tx += NUM_TX_DESC;
-	}
-#endif /* RTL8139_NDEBUG */
-
 	/* only wake the queue if we did work, and the queue is stopped */
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;

Re: [PATCH] 8139too: harden against TX ring overflow

[Herbert Xu]

Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> This driver's 4-packet deep TX queue is too sensible to the "careless" callers
> ignoring its state (like netpoll in trapped mode), so add "queue full" check at
> the start of the hard_start_xmit() method (only under #ifndef RTL8139_NDEBUG,
> otherwise the queue will get stuck once dirty pointer gets out of sync); switch
> to using appropriate mnemonics for the return values while at it.

Could you please describe this netpoll scenario in more detail?
More importantly, why wouldn't we fix netpoll instead?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] 8139too: harden against TX ring overflow

[Amit S. Kale]

On Friday 06 April 2007 07:18, Herbert Xu wrote:
> Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > This driver's 4-packet deep TX queue is too sensible to the "careless"
> > callers ignoring its state (like netpoll in trapped mode), so add "queue
> > full" check at the start of the hard_start_xmit() method (only under
> > #ifndef RTL8139_NDEBUG, otherwise the queue will get stuck once dirty
> > pointer gets out of sync); switch to using appropriate mnemonics for the
> > return values while at it.
>
> Could you please describe this netpoll scenario in more detail?
> More importantly, why wouldn't we fix netpoll instead?

We're trying to figure out a way of fixing netpoll. Don't know what the 
solution is yet.

Here is what happens: in KGDB we set netpoll trapped flag. This prevents 
stopping and starting of a netdev queue. Interfaces that have a small ring 
(8139) run into a problem because of this. When the ring goes full, it can't 
stop the queue. This doesn't make sense since in absence of ring descriptors, 
the device can't transmit any more packets. Sergie had posted one more patch 
last week that lets us start and stop queues in trapped state.

This patch fixes the 8139 side behavior in this context.
-Amit

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

[Sergei Shtylyov]

Hello.

Amit S. Kale wrote:

>>>This driver's 4-packet deep TX queue is too sensible to the "careless"
>>>callers ignoring its state (like netpoll in trapped mode), so add "queue
>>>full" check at the start of the hard_start_xmit() method (only under
>>>#ifndef RTL8139_NDEBUG, otherwise the queue will get stuck once dirty
>>>pointer gets out of sync); switch to using appropriate mnemonics for the
>>>return values while at it.

>>Could you please describe this netpoll scenario in more detail?
>>More importantly, why wouldn't we fix netpoll instead?

> We're trying to figure out a way of fixing netpoll. Don't know what the 
> solution is yet.

> Here is what happens: in KGDB we set netpoll trapped flag. This prevents 
> stopping and starting of a netdev queue. Interfaces that have a small ring 
> (8139) run into a problem because of this. When the ring goes full, it can't 
> stop the queue. This doesn't make sense since in absence of ring descriptors, 
> the device can't transmit any more packets. Sergie had posted one more patch 
> last week that lets us start and stop queues in trapped state.
> 
> This patch fixes the 8139 side behavior in this context.

    Not really -- the patch doesn't seem necessary in this context.  It's just 
an attempt to make the driver more robust against out-of-sync dirty pointer 
and posting packets to already stopped queue -- that check constituting the 
second part is still necessary in certain out-of-sync pointer situation where 
the queue may erroneously be woken up, i.e. with the difference between the 
dirty_tx and cur_tx is multiple of 4 by the end of loop (well, I guess that 
has happened only with netpoll in trapped mode so far).  Remember that all 
these checks are done only when the driver debugging is enabled (it *is* 
enabled by default though).

> -Amit

MBR, Sergei

Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

[Sergei Shtylyov]

Hello, I wrote:
> This driver's 4-packet deep TX queue is too sensible to the "careless" callers
> ignoring its state (like netpoll in trapped mode), so add "queue full" check at
> the start of the hard_start_xmit() method (only under #ifndef RTL8139_NDEBUG,
> otherwise the queue will get stuck once dirty pointer gets out of sync); switch
> to using appropriate mnemonics for the return values while at it.
> Also, the out-of-sync dirty pointer check is misplaced in rtl8139_tx_interrupt()
> which causes TX descriptors to be inspected more than once in case the pointer
> really gets out-of-sync (and incrementing the dirty pointer always by 4 is just
> not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the queue) --
> place it before the loop and limit the loop to only look through 4 descriptors
> at most, so that already overwritten descriptors are just not counted.

> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

    Jeff, do you have any opinion on this patch?

> Index: linux-2.6/drivers/net/8139too.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/8139too.c
> +++ linux-2.6/drivers/net/8139too.c
> @@ -90,7 +90,7 @@
>  */
>  
>  #define DRV_NAME	"8139too"
> -#define DRV_VERSION	"0.9.28"
> +#define DRV_VERSION	"0.9.31"
>  
>  
>  #include <linux/module.h>
> @@ -1708,6 +1708,13 @@ static int rtl8139_start_xmit (struct sk
>  	unsigned int len = skb->len;
>  	unsigned long flags;
>  
> +#ifndef RTL8139_NDEBUG
> +	if (unlikely((tp->cur_tx - tp->dirty_tx) >= NUM_TX_DESC)) {
> +		printk(KERN_ERR "%s: TX queue full!\n", dev->name);
> +		return NETDEV_TX_BUSY;
> +	}
> +#endif
> +
>  	/* Calculate the next Tx descriptor entry. */
>  	entry = tp->cur_tx % NUM_TX_DESC;
>  
> @@ -1720,7 +1727,7 @@ static int rtl8139_start_xmit (struct sk
>  	} else {
>  		dev_kfree_skb(skb);
>  		tp->stats.tx_dropped++;
> -		return 0;
> +		return NETDEV_TX_OK;
>  	}
>  
>  	spin_lock_irqsave(&tp->lock, flags);
> @@ -1740,7 +1747,7 @@ static int rtl8139_start_xmit (struct sk
>  		printk (KERN_DEBUG "%s: Queued Tx packet size %u to slot %d.\n",
>  			dev->name, len, entry);
>  
> -	return 0;
> +	return NETDEV_TX_OK;
>  }
>  
>  
> @@ -1755,6 +1762,16 @@ static void rtl8139_tx_interrupt (struct
>  
>  	dirty_tx = tp->dirty_tx;
>  	tx_left = tp->cur_tx - dirty_tx;
> +
> +#ifndef RTL8139_NDEBUG
> +	if (unlikely(tx_left > NUM_TX_DESC)) {
> +		printk(KERN_ERR "%s: Out-of-sync dirty pointer, %ld vs. %ld.\n",
> +		       dev->name, dirty_tx, tp->cur_tx);
> +		tx_left  = NUM_TX_DESC;
> +		dirty_tx = tp->cur_tx - NUM_TX_DESC;
> +	}
> +#endif /* RTL8139_NDEBUG */
> +
>  	while (tx_left > 0) {
>  		int entry = dirty_tx % NUM_TX_DESC;
>  		int txstatus;
> @@ -1797,14 +1814,6 @@ static void rtl8139_tx_interrupt (struct
>  		tx_left--;
>  	}
>  
> -#ifndef RTL8139_NDEBUG
> -	if (tp->cur_tx - dirty_tx > NUM_TX_DESC) {
> -		printk (KERN_ERR "%s: Out-of-sync dirty pointer, %ld vs. %ld.\n",
> -		        dev->name, dirty_tx, tp->cur_tx);
> -		dirty_tx += NUM_TX_DESC;
> -	}
> -#endif /* RTL8139_NDEBUG */
> -
>  	/* only wake the queue if we did work, and the queue is stopped */
>  	if (tp->dirty_tx != dirty_tx) {
>  		tp->dirty_tx = dirty_tx;
> 

WBR, Sergei

Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

[Jeff Garzik]

Sergei Shtylyov wrote:
> Hello, I wrote:
>> This driver's 4-packet deep TX queue is too sensible to the "careless" 
>> callers
>> ignoring its state (like netpoll in trapped mode), so add "queue full" 
>> check at
>> the start of the hard_start_xmit() method (only under #ifndef 
>> RTL8139_NDEBUG,
>> otherwise the queue will get stuck once dirty pointer gets out of 
>> sync); switch
>> to using appropriate mnemonics for the return values while at it.
>> Also, the out-of-sync dirty pointer check is misplaced in 
>> rtl8139_tx_interrupt()
>> which causes TX descriptors to be inspected more than once in case the 
>> pointer
>> really gets out-of-sync (and incrementing the dirty pointer always by 
>> 4 is just
>> not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the 
>> queue) --
>> place it before the loop and limit the loop to only look through 4 
>> descriptors
>> at most, so that already overwritten descriptors are just not counted.
> 
>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>    Jeff, do you have any opinion on this patch?

The out-of-sync dirty pointer check is leftover boilerplate, and not 
really indicative of anything except for some code to be removed.

As for the other stuff, I would say "fix the caller".  We don't need to 
hack every driver for cases where netpoll is being dumb.

	Jeff

Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

[Sergei Shtylyov]

Jeff Garzik wrote:

>>> This driver's 4-packet deep TX queue is too sensible to the 
>>> "careless" callers
>>> ignoring its state (like netpoll in trapped mode), so add "queue 
>>> full" check at
>>> the start of the hard_start_xmit() method (only under #ifndef 
>>> RTL8139_NDEBUG,
>>> otherwise the queue will get stuck once dirty pointer gets out of 
>>> sync); switch
>>> to using appropriate mnemonics for the return values while at it.
>>> Also, the out-of-sync dirty pointer check is misplaced in 
>>> rtl8139_tx_interrupt()
>>> which causes TX descriptors to be inspected more than once in case 
>>> the pointer
>>> really gets out-of-sync (and incrementing the dirty pointer always by 
>>> 4 is just
>>> not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the 
>>> queue) --
>>> place it before the loop and limit the loop to only look through 4 
>>> descriptors
>>> at most, so that already overwritten descriptors are just not counted.

>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>    Jeff, do you have any opinion on this patch?

> The out-of-sync dirty pointer check is leftover boilerplate, and not 
> really indicative of anything except for some code to be removed.

> As for the other stuff, I would say "fix the caller".  We don't need to 
> hack every driver for cases where netpoll is being dumb.

    Caller's been fixed now. So, I read that as reject. :-)

>     Jeff

WBR, Sergei

Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

[Jeff Garzik]

Sergei Shtylyov wrote:
> Jeff Garzik wrote:
> 
>>>> This driver's 4-packet deep TX queue is too sensible to the 
>>>> "careless" callers
>>>> ignoring its state (like netpoll in trapped mode), so add "queue 
>>>> full" check at
>>>> the start of the hard_start_xmit() method (only under #ifndef 
>>>> RTL8139_NDEBUG,
>>>> otherwise the queue will get stuck once dirty pointer gets out of 
>>>> sync); switch
>>>> to using appropriate mnemonics for the return values while at it.
>>>> Also, the out-of-sync dirty pointer check is misplaced in 
>>>> rtl8139_tx_interrupt()
>>>> which causes TX descriptors to be inspected more than once in case 
>>>> the pointer
>>>> really gets out-of-sync (and incrementing the dirty pointer always 
>>>> by 4 is just
>>>> not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the 
>>>> queue) --
>>>> place it before the loop and limit the loop to only look through 4 
>>>> descriptors
>>>> at most, so that already overwritten descriptors are just not counted.
> 
>>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>>>    Jeff, do you have any opinion on this patch?
> 
>> The out-of-sync dirty pointer check is leftover boilerplate, and not 
>> really indicative of anything except for some code to be removed.
> 
>> As for the other stuff, I would say "fix the caller".  We don't need 
>> to hack every driver for cases where netpoll is being dumb.
> 
>    Caller's been fixed now. So, I read that as reject. :-)

Mostly.  You're still welcome to remove the silly boilerplate checks...

And ->hard_start_xmit() return value update(s) were sane.

	Jeff