[RFT] mv643xxx_eth_start_xmit oops

On Fri, 10 Nov 2006 12:17:45 -0700
Erik Andersen [off-list ref] 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>

Stephen Hemminger [off-list ref] :
[...]

It seems to propagate a leak from the initial codebase.

-- 
Ueimor

In article [off-list ref] you write:
Ueimor [off-list ref] wrote:

Stephen Hemminger [off-list ref] :
[...]

quoted

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

Dale Farnsworth [off-list ref] :
[...]

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

On Fri, 10 Nov 2006 22:03:43 +0100
Francois Romieu [off-list ref] wrote:

Dale Farnsworth [off-list ref] :
[...]

quoted

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 [off-list ref]

Stephen Hemminger [off-list ref] :
[...]

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

On Fri, 10 Nov 2006 22:30:43 +0100
Francois Romieu [off-list ref] wrote:

Stephen Hemminger [off-list ref] :
[...]

quoted

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 [off-list ref]

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)
 {

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.

David Miller [off-list ref] :
[...]

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