[RFT] mv643xxx_eth_start_xmit oops

[RFT] mv643xxx_eth_start_xmit oops

[Stephen Hemminger]

On Fri, 10 Nov 2006 12:17:45 -0700
Erik Andersen <andersen@codepoet.org> wrote:

> I have a Pegasos2 powerpc system acting as my home server.  With
> 2.6.16.x it was 100% stable and I had months of uptime, rebooting
> only to periodically apply security updates to the kernel.
> 
> With 2.6.17 and 2.6.18, after an uptime of no more than 2 days,
> and usually much less, I get a kernel panic, with nothing in the
> log.  I finally caught it in the act, and took a picture.
> http://codepoet.org/oops.jpg
> 
> A quick transcription of the Oops in the screenshot follows:
> --------------------------------------------
> 

The code int mv643xx_eth_start_xmit is not safe on SMP it was checking for space outside of lock.
Does the following (untested) fix it?

0. Fix race where space check is outside of lock.
1. Eliminate bogus BUG_ON()'s
2. Use proper transmit routine return values
3. Cleanup potential kernel log overrun if hit with unaligned frags
4. Compare with actual space needed rather than worst case
5. Use correct return code for case of linearize() failure. 

---
 drivers/net/mv643xx_eth.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 9997081..4052bfe 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -1191,25 +1191,23 @@ static int mv643xx_eth_start_xmit(struct
 	struct net_device_stats *stats = &mp->stats;
 	unsigned long flags;
 
-	BUG_ON(netif_queue_stopped(dev));
-	BUG_ON(skb == NULL);
-
-	if (mp->tx_ring_size - mp->tx_desc_count < MAX_DESCS_PER_SKB) {
-		printk(KERN_ERR "%s: transmit with queue full\n", dev->name);
-		netif_stop_queue(dev);
-		return 1;
-	}
-
-	if (has_tiny_unaligned_frags(skb)) {
-		if (__skb_linearize(skb)) {
-			stats->tx_dropped++;
+	if (has_tiny_unaligned_frags(skb) && __skb_linearize(skb)) {
+		stats->tx_dropped++;
+		if (net_ratelimit())
 			printk(KERN_DEBUG "%s: failed to linearize tiny "
-					"unaligned fragment\n", dev->name);
-			return 1;
-		}
+			       "unaligned fragment\n", dev->name);
+		return NETDEV_TX_OK;
 	}
 
 	spin_lock_irqsave(&mp->lock, flags);
+	if (mp->tx_ring_size - mp->tx_desc_count < skb_shinfo(skb)->nr_frags + 1) {
+		if (!netif_queue_stopped(dev)) {
+			printk(KERN_ERR "%s: transmit with queue full\n", dev->name);
+			netif_stop_queue(dev);
+		}
+		spin_unlock_irqrestore(&mp->lock, flags);
+		return NETDEV_TX_BUSY;;
+	}
 
 	eth_tx_submit_descs_for_skb(mp, skb);
 	stats->tx_bytes = skb->len;
@@ -1221,7 +1219,7 @@ static int mv643xx_eth_start_xmit(struct
 
 	spin_unlock_irqrestore(&mp->lock, flags);
 
-	return 0;		/* success */
+	return NETDEV_TX_OK;
 }
 
 /*
-- 
1.4.1






-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [RFT] mv643xxx_eth_start_xmit oops

[Francois Romieu]

Stephen Hemminger <shemminger@osdl.org> :
[...]
> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> index 9997081..4052bfe 100644
> --- a/drivers/net/mv643xx_eth.c
> +++ b/drivers/net/mv643xx_eth.c
> @@ -1191,25 +1191,23 @@ static int mv643xx_eth_start_xmit(struct
>  	struct net_device_stats *stats = &mp->stats;
>  	unsigned long flags;
>  
> -	BUG_ON(netif_queue_stopped(dev));
> -	BUG_ON(skb == NULL);
> -
> -	if (mp->tx_ring_size - mp->tx_desc_count < MAX_DESCS_PER_SKB) {
> -		printk(KERN_ERR "%s: transmit with queue full\n", dev->name);
> -		netif_stop_queue(dev);
> -		return 1;
> -	}
> -
> -	if (has_tiny_unaligned_frags(skb)) {
> -		if (__skb_linearize(skb)) {
> -			stats->tx_dropped++;
> +	if (has_tiny_unaligned_frags(skb) && __skb_linearize(skb)) {
> +		stats->tx_dropped++;
> +		if (net_ratelimit())
>  			printk(KERN_DEBUG "%s: failed to linearize tiny "
> -					"unaligned fragment\n", dev->name);
> -			return 1;
> -		}
> +			       "unaligned fragment\n", dev->name);
> +		return NETDEV_TX_OK;
>  	}

It seems to propagate a leak from the initial codebase.

-- 
Ueimor

Re: [RFT] mv643xxx_eth_start_xmit oops

[Dale Farnsworth]

In article <20061110203009.GA9851@electric-eye.fr.zoreil.com> you write:
Ueimor <romieu@fr.zoreil.com> wrote:
> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> [...]
> 
> It seems to propagate a leak from the initial codebase.

Can you provide more detail about the leak?

-Dale

Re: [RFT] mv643xxx_eth_start_xmit oops

[Francois Romieu]

Dale Farnsworth <dale@farnsworth.org> :
[...]
> Can you provide more detail about the leak?

+       if (has_tiny_unaligned_frags(skb) && __skb_linearize(skb)) {
+               stats->tx_dropped++;
+               if (net_ratelimit())
+                       printk(KERN_DEBUG "%s: failed to linearize tiny "
+                              "unaligned fragment\n", dev->name);
+               return NETDEV_TX_OK;

Missing kfree_skb(skb) before returning NETDEV_TX_OK ?

-- 
Ueimor

Re: [RFT] mv643xxx_eth_start_xmit oops

[Stephen Hemminger]

On Fri, 10 Nov 2006 22:03:43 +0100
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Dale Farnsworth <dale@farnsworth.org> :
> [...]
> > Can you provide more detail about the leak?
> 
> +       if (has_tiny_unaligned_frags(skb) && __skb_linearize(skb)) {
> +               stats->tx_dropped++;
> +               if (net_ratelimit())
> +                       printk(KERN_DEBUG "%s: failed to linearize tiny "
> +                              "unaligned fragment\n", dev->name);
> +               return NETDEV_TX_OK;
> 
> Missing kfree_skb(skb) before returning NETDEV_TX_OK ?
> 

skb_linearize is documented to free skb on failure.





-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [RFT] mv643xxx_eth_start_xmit oops

[Francois Romieu]

Stephen Hemminger <shemminger@osdl.org> :
[...]
> skb_linearize is documented to free skb on failure.

__skb_linearize
-> __pskb_pull_tail
   -> pskb_expand_head
      [...]
        data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
        if (!data)
                goto nodata;
      [...]
nodata:
        return -ENOMEM;

I don't see where the skb is freed on this path.

Btw, the same __skb_linearize() is followed by a kfree_skb() in
drivers/net/via-velocity.c since 364c6badde0dd62a0a38e5ed67f85d87d6665780

I may be wrong but the source code does not seem completely right either.

-- 
Ueimor

Re: [RFT] mv643xxx_eth_start_xmit oops

[Stephen Hemminger]

On Fri, 10 Nov 2006 22:30:43 +0100
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > skb_linearize is documented to free skb on failure.
> 
> __skb_linearize
> -> __pskb_pull_tail
>    -> pskb_expand_head
>       [...]
>         data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
>         if (!data)
>                 goto nodata;
>       [...]
> nodata:
>         return -ENOMEM;
> 
> I don't see where the skb is freed on this path.
> 
> Btw, the same __skb_linearize() is followed by a kfree_skb() in
> drivers/net/via-velocity.c since 364c6badde0dd62a0a38e5ed67f85d87d6665780
> 
> I may be wrong but the source code does not seem completely right either.


Your correct, it does leave the skb alone. so it would be a leak.
Better documentation in skb_linearize would help.

-- 
Stephen Hemminger <shemminger@osdl.org>

[NET] Update documentation of skb_linearize

[Francois Romieu]

The data is not released.

drivers/net/mv643xx_eth.c apart, each current caller issues
kfree_skb() when required.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 85577a4..380e344 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1235,7 +1235,7 @@ static inline int __skb_linearize(struct
  *	@skb: buffer to linarize
  *
  *	If there is no free memory -ENOMEM is returned, otherwise zero
- *	is returned and the old skb data released.
+ *	is returned.
  */
 static inline int skb_linearize(struct sk_buff *skb)
 {
@@ -1247,7 +1247,7 @@ static inline int skb_linearize(struct s
  *	@skb: buffer to process
  *
  *	If there is no free memory -ENOMEM is returned, otherwise zero
- *	is returned and the old skb data released.
+ *	is returned.
  */
 static inline int skb_linearize_cow(struct sk_buff *skb)
 {

Re: [NET] Update documentation of skb_linearize

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Fri, 10 Nov 2006 23:03:55 +0100

> The data is not released.

Yes it is.

If the non-linear SKB has data in pages, we copy that data from the
pages into the new linear skb->data area and release the paged data.

Re: [NET] Update documentation of skb_linearize

[Francois Romieu]

David Miller <davem@davemloft.net> :
[...]
> Yes it is.
> 
> If the non-linear SKB has data in pages, we copy that data from the
> pages into the new linear skb->data area and release the paged data.


Right. The documentation talks about the skb _data_ so there is no
reason to imagine that the function could freed the skb on failure.

I'll leave Stephen reformulate the documentation if he feels the need to.

-- 
Ueimor