From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Subject: Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow Date: Fri, 25 May 2007 01:29:29 +0400 Message-ID: <465603B9.90401@ru.mvista.com> References: <200704052350.59464.sshtylyov@ru.mvista.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, mhuth@mvista.com, kgdb-bugreport@lists.sourceforge.net To: jgarzik@pobox.com Return-path: Received: from gateway-1237.mvista.com ([63.81.120.155]:3816 "EHLO imap.sh.mvista.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1750999AbXEXV16 (ORCPT ); Thu, 24 May 2007 17:27:58 -0400 In-Reply-To: <200704052350.59464.sshtylyov@ru.mvista.com> Sender: netdev-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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 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 > @@ -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