[PATCH] sis900: Allocate rx replacement buffer before rx operation

[PATCH] sis900: Allocate rx replacement buffer before rx operation

[Neil Horman]

Hey there-
	The sis900 driver appears to have a bug in which the receive routine
passes the skbuff holding the received frame to the network stack before
refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, the
driver simply leaves a hole in the rx ring, which causes the driver to stop
receiving frames and become non-recoverable without an rmmod/insmod according to
reporters.  This patch reverses that order, attempting to allocate a replacement
buffer first, and receiving the new frame only if one can be allocated.  If no
skbuff can be allocated, the current skbuf in the rx ring is recycled, dropping
the current frame, but keeping the NIC operational.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 sis900.c |   44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)


commit b11e33ae6b479f2e7f83b9dc148f0f24d47530bd
Author: Neil Horman <nhorman@redhat.com>
Date:   Mon Feb 12 09:47:45 2007 -0500

    fixing buffer refill problem on sis900

diff --git a/drivers/net/sis900.c b/drivers/net/sis900.c
index fb2b530..a6a0f09 100644
--- a/drivers/net/sis900.c
+++ b/drivers/net/sis900.c
@@ -1755,6 +1755,24 @@ static int sis900_rx(struct net_device *net_dev)
 		} else {
 			struct sk_buff * skb;
 
+			pci_unmap_single(sis_priv->pci_dev,
+				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE,
+				PCI_DMA_FROMDEVICE);
+
+			/* refill the Rx buffer, what if there is not enought
+			 * memory for new socket buffer ?? */
+			if ((skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
+				/*
+				 * Not enough memory to refill the buffer
+				 * so we need to recycle the old one so
+				 * as to avoid creating a memory hole
+				 * in the rx ring
+				 */
+				skb = sis_priv->rx_skbuff[entry];
+				sis_priv->stats.rx_dropped++;
+				goto refill_rx_ring;
+			}	
+
 			/* This situation should never happen, but due to
 			   some unknow bugs, it is possible that
 			   we are working on NULL sk_buff :-( */
@@ -1768,9 +1786,6 @@ static int sis900_rx(struct net_device *net_dev)
 				break;
 			}
 
-			pci_unmap_single(sis_priv->pci_dev,
-				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE,
-				PCI_DMA_FROMDEVICE);
 			/* give the socket buffer to upper layers */
 			skb = sis_priv->rx_skbuff[entry];
 			skb_put(skb, rx_size);
@@ -1783,33 +1798,14 @@ static int sis900_rx(struct net_device *net_dev)
 			net_dev->last_rx = jiffies;
 			sis_priv->stats.rx_bytes += rx_size;
 			sis_priv->stats.rx_packets++;
-
-			/* refill the Rx buffer, what if there is not enought
-			 * memory for new socket buffer ?? */
-			if ((skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
-				/* not enough memory for skbuff, this makes a
-				 * "hole" on the buffer ring, it is not clear
-				 * how the hardware will react to this kind
-				 * of degenerated buffer */
-				if (netif_msg_rx_status(sis_priv))
-					printk(KERN_INFO "%s: Memory squeeze,"
-						"deferring packet.\n",
-						net_dev->name);
-				sis_priv->rx_skbuff[entry] = NULL;
-				/* reset buffer descriptor state */
-				sis_priv->rx_ring[entry].cmdsts = 0;
-				sis_priv->rx_ring[entry].bufptr = 0;
-				sis_priv->stats.rx_dropped++;
-				sis_priv->cur_rx++;
-				break;
-			}
+			sis_priv->dirty_rx++;
+refill_rx_ring:
 			skb->dev = net_dev;
 			sis_priv->rx_skbuff[entry] = skb;
 			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
                 	sis_priv->rx_ring[entry].bufptr =
 				pci_map_single(sis_priv->pci_dev, skb->data,
 					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
-			sis_priv->dirty_rx++;
 		}
 		sis_priv->cur_rx++;
 		entry = sis_priv->cur_rx % NUM_RX_DESC;

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Jeff Garzik]

Neil Horman wrote:
> Hey there-
> 	The sis900 driver appears to have a bug in which the receive routine
> passes the skbuff holding the received frame to the network stack before
> refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, the
> driver simply leaves a hole in the rx ring, which causes the driver to stop
> receiving frames and become non-recoverable without an rmmod/insmod according to
> reporters.  This patch reverses that order, attempting to allocate a replacement
> buffer first, and receiving the new frame only if one can be allocated.  If no
> skbuff can be allocated, the current skbuf in the rx ring is recycled, dropping
> the current frame, but keeping the NIC operational.
> 
> Thanks & Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Please CC the net driver maintainer ;-)

I'll queue this for review.

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Daniele Venzano]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]


Il giorno 20/apr/07, alle ore 15:54, Neil Horman ha scritto:

> Hey there-
> 	The sis900 driver appears to have a bug in which the receive routine
> passes the skbuff holding the received frame to the network stack  
> before
> refilling the buffer in the rx ring.  If a new skbuff cannot be  
> allocated, the
> driver simply leaves a hole in the rx ring, which causes the driver  
> to stop
> receiving frames and become non-recoverable without an rmmod/insmod  
> according to
> reporters.  This patch reverses that order, attempting to allocate  
> a replacement
> buffer first, and receiving the new frame only if one can be  
> allocated.  If no
> skbuff can be allocated, the current skbuf in the rx ring is  
> recycled, dropping
> the current frame, but keeping the NIC operational.
Yes, I had one report of a user actually hitting this, but it is  
quite hard to reproduce and I don't have continuos access anymore to  
my sis900 machine.
The patch seems fine by me.

> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Daniele Venzano <venza@brownhat.org>

--
Daniele Venzano
http://www.brownhat.org


[-- Attachment #2: Questa è un messaggio firmato elettronicamente --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Jeff Garzik]

Neil Horman wrote:
> Hey there-
> 	The sis900 driver appears to have a bug in which the receive routine
> passes the skbuff holding the received frame to the network stack before
> refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, the
> driver simply leaves a hole in the rx ring, which causes the driver to stop
> receiving frames and become non-recoverable without an rmmod/insmod according to
> reporters.  This patch reverses that order, attempting to allocate a replacement
> buffer first, and receiving the new frame only if one can be allocated.  If no
> skbuff can be allocated, the current skbuf in the rx ring is recycled, dropping
> the current frame, but keeping the NIC operational.
> 
> Thanks & Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  sis900.c |   44 ++++++++++++++++++++------------------------
>  1 file changed, 20 insertions(+), 24 deletions(-)

applied

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Neil Horman]

On Tue, Apr 24, 2007 at 12:43:20PM -0400, Jeff Garzik wrote:
> Neil Horman wrote:
> >Hey there-
> >	The sis900 driver appears to have a bug in which the receive routine
> >passes the skbuff holding the received frame to the network stack before
> >refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, 
> >the
> >driver simply leaves a hole in the rx ring, which causes the driver to stop
> >receiving frames and become non-recoverable without an rmmod/insmod 
> >according to
> >reporters.  This patch reverses that order, attempting to allocate a 
> >replacement
> >buffer first, and receiving the new frame only if one can be allocated.  
> >If no
> >skbuff can be allocated, the current skbuf in the rx ring is recycled, 
> >dropping
> >the current frame, but keeping the NIC operational.
> >
> >Thanks & Regards
> >Neil
> 



Just found a hole in my last patch.  It was reported to me that shortly after we
integrated this patch.  The report was of an oops that took place inside of
netif_rx when using the sis900 driver.  Looking at my origional patch I noted
that there was a spot between the new skb_alloc and the refill_rx_ring label
where skb got reassigned to the pointer currently held in the rx_ring for the
purposes of receiveing the frame.  The result of this is however that the buffer
that gets passed to netif_rx (if it is called), then gets placed right back into
the rx_ring.  So if you receive frames fast enough the skb being processed by
the network stack can get corrupted.  The reporter is testing out the fix I've
written for this below (I'm not near my hardware at the moment to test myself),
but I wanted to post it for review ASAP.  I'll post test results when I hear
them, but I think this is a pretty straightforward fix.  It just uses a separate
pointer to do the rx operation, so that we don't improperly reassign the pointer
that we use to refill the rx ring.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 sis900.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)


diff --git a/drivers/net/sis900.c b/drivers/net/sis900.c
index a6a0f09..7e44939 100644
--- a/drivers/net/sis900.c
+++ b/drivers/net/sis900.c
@@ -1754,6 +1754,7 @@ static int sis900_rx(struct net_device *net_dev)
 			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
 		} else {
 			struct sk_buff * skb;
+			struct sk_buff * rx_skb;
 
 			pci_unmap_single(sis_priv->pci_dev,
 				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE,
@@ -1787,10 +1788,10 @@ static int sis900_rx(struct net_device *net_dev)
 			}
 
 			/* give the socket buffer to upper layers */
-			skb = sis_priv->rx_skbuff[entry];
-			skb_put(skb, rx_size);
-			skb->protocol = eth_type_trans(skb, net_dev);
-			netif_rx(skb);
+			rx_skb = sis_priv->rx_skbuff[entry];
+			skb_put(rx_skb, rx_size);
+			skb->protocol = eth_type_trans(rx_skb, net_dev);
+			netif_rx(rx_skb);
 
 			/* some network statistics */
 			if ((rx_status & BCAST) == MCAST)

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Neil Horman]

On Thu, Apr 26, 2007 at 01:47:36PM -0400, Neil Horman wrote:
> On Tue, Apr 24, 2007 at 12:43:20PM -0400, Jeff Garzik wrote:
> > Neil Horman wrote:
> > >Hey there-
> > >	The sis900 driver appears to have a bug in which the receive routine
> > >passes the skbuff holding the received frame to the network stack before
> > >refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, 
> > >the
> > >driver simply leaves a hole in the rx ring, which causes the driver to stop
> > >receiving frames and become non-recoverable without an rmmod/insmod 
> > >according to
> > >reporters.  This patch reverses that order, attempting to allocate a 
> > >replacement
> > >buffer first, and receiving the new frame only if one can be allocated.  
> > >If no
> > >skbuff can be allocated, the current skbuf in the rx ring is recycled, 
> > >dropping
> > >the current frame, but keeping the NIC operational.
> > >
> > >Thanks & Regards
> > >Neil
> > 
> 
> 
> 
> Just found a hole in my last patch.  It was reported to me that shortly after we
> integrated this patch.  The report was of an oops that took place inside of
> netif_rx when using the sis900 driver.  Looking at my origional patch I noted
> that there was a spot between the new skb_alloc and the refill_rx_ring label
> where skb got reassigned to the pointer currently held in the rx_ring for the
> purposes of receiveing the frame.  The result of this is however that the buffer
> that gets passed to netif_rx (if it is called), then gets placed right back into
> the rx_ring.  So if you receive frames fast enough the skb being processed by
> the network stack can get corrupted.  The reporter is testing out the fix I've
> written for this below (I'm not near my hardware at the moment to test myself),
> but I wanted to post it for review ASAP.  I'll post test results when I hear
> them, but I think this is a pretty straightforward fix.  It just uses a separate
> pointer to do the rx operation, so that we don't improperly reassign the pointer
> that we use to refill the rx ring.
> 
> Thanks & Regards
> Neil
> 
Test results are back and appear successful.
Neil

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Jeff Garzik]

Neil Horman wrote:
> On Tue, Apr 24, 2007 at 12:43:20PM -0400, Jeff Garzik wrote:
>> Neil Horman wrote:
>>> Hey there-
>>> 	The sis900 driver appears to have a bug in which the receive routine
>>> passes the skbuff holding the received frame to the network stack before
>>> refilling the buffer in the rx ring.  If a new skbuff cannot be allocated, 
>>> the
>>> driver simply leaves a hole in the rx ring, which causes the driver to stop
>>> receiving frames and become non-recoverable without an rmmod/insmod 
>>> according to
>>> reporters.  This patch reverses that order, attempting to allocate a 
>>> replacement
>>> buffer first, and receiving the new frame only if one can be allocated.  
>>> If no
>>> skbuff can be allocated, the current skbuf in the rx ring is recycled, 
>>> dropping
>>> the current frame, but keeping the NIC operational.
>>>
>>> Thanks & Regards
>>> Neil
> 
> 
> 
> Just found a hole in my last patch.  It was reported to me that shortly after we
> integrated this patch.  The report was of an oops that took place inside of
> netif_rx when using the sis900 driver.  Looking at my origional patch I noted
> that there was a spot between the new skb_alloc and the refill_rx_ring label
> where skb got reassigned to the pointer currently held in the rx_ring for the
> purposes of receiveing the frame.  The result of this is however that the buffer
> that gets passed to netif_rx (if it is called), then gets placed right back into
> the rx_ring.  So if you receive frames fast enough the skb being processed by
> the network stack can get corrupted.  The reporter is testing out the fix I've
> written for this below (I'm not near my hardware at the moment to test myself),
> but I wanted to post it for review ASAP.  I'll post test results when I hear
> them, but I think this is a pretty straightforward fix.  It just uses a separate
> pointer to do the rx operation, so that we don't improperly reassign the pointer
> that we use to refill the rx ring.
> 
> Thanks & Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  sis900.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/drivers/net/sis900.c b/drivers/net/sis900.c
> index a6a0f09..7e44939 100644
> --- a/drivers/net/sis900.c
> +++ b/drivers/net/sis900.c
> @@ -1754,6 +1754,7 @@ static int sis900_rx(struct net_device *net_dev)
>  			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
>  		} else {
>  			struct sk_buff * skb;
> +			struct sk_buff * rx_skb;
>  
>  			pci_unmap_single(sis_priv->pci_dev,
>  				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE,
> @@ -1787,10 +1788,10 @@ static int sis900_rx(struct net_device *net_dev)
>  			}
>  
>  			/* give the socket buffer to upper layers */
> -			skb = sis_priv->rx_skbuff[entry];
> -			skb_put(skb, rx_size);
> -			skb->protocol = eth_type_trans(skb, net_dev);
> -			netif_rx(skb);
> +			rx_skb = sis_priv->rx_skbuff[entry];
> +			skb_put(rx_skb, rx_size);
> +			skb->protocol = eth_type_trans(rx_skb, net_dev);

applied this, and the one-line fix to this

Re: [PATCH] sis900: Allocate rx replacement buffer before rx operation

[Neil Horman]

> >diff --git a/drivers/net/sis900.c b/drivers/net/sis900.c
> >index a6a0f09..7e44939 100644
> >--- a/drivers/net/sis900.c
> >+++ b/drivers/net/sis900.c
> >@@ -1754,6 +1754,7 @@ static int sis900_rx(struct net_device *net_dev)
> > 			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
> > 		} else {
> > 			struct sk_buff * skb;
> >+			struct sk_buff * rx_skb;
> > 
> > 			pci_unmap_single(sis_priv->pci_dev,
> > 				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE,
> >@@ -1787,10 +1788,10 @@ static int sis900_rx(struct net_device *net_dev)
> > 			}
> > 
> > 			/* give the socket buffer to upper layers */
> >-			skb = sis_priv->rx_skbuff[entry];
> >-			skb_put(skb, rx_size);
> >-			skb->protocol = eth_type_trans(skb, net_dev);
> >-			netif_rx(skb);
> >+			rx_skb = sis_priv->rx_skbuff[entry];
> >+			skb_put(rx_skb, rx_size);
> >+			skb->protocol = eth_type_trans(rx_skb, net_dev);
> 
> applied this, and the one-line fix to this
> 
Thanks Jeff, sorry again for the headache.  I'll be more careful in the future
Neil