Possible leaks in network drivers

Possible leaks in network drivers

[Eric Sesterhenn]

hi,

Coverity complains about several pretty similar resource leaks
inside the net drivers, and i am not sure if those are real

name				coverity #id

drivers/net/8390.c		623
drivers/net/pcmcia/xirc2ps_cs.c	627
drivers/net/sis190.c		628
drivers/net/wireless/wavelan.c	634
drivers/net/wireless/orinoco.c	661
drivers/net/depca.c		1246
drivers/net/hp100.c		1247
drivers/net/smc9194.c		1248
drivers/net/skge.c		1249

Its always in the hard_start_xmit() function
of the driver. Where we call skb=skb_padto(skb, ETH_ZLEN),
and dont free the skb later when something goes wrong.

Here is the output from the sis190.c case:

------------snip--8<-------------
1158 		if (unlikely(skb->len < ETH_ZLEN)) {

Event alloc_fn: Called allocation function "skb_padto" [model]
Event var_assign: Assigned variable "skb" to storage returned from "skb_padto"
Also see events: [var_assign][leaked_storage]

1159 			skb = skb_padto(skb, ETH_ZLEN);

At conditional (1): "skb == 0" taking false path

1160 			if (!skb) {
1161 				tp->stats.tx_dropped++;
1162 				goto out;
1163 			}
1164 			len = ETH_ZLEN;
1165 		} else {
1166 			len = skb->len;
1167 		}
1168 	
1169 		entry = tp->cur_tx % NUM_TX_DESC;
1170 		desc = tp->TxDescRing + entry;
1171 	

At conditional (2): "(desc)->status & 2147483648 != 0" taking true path

1172 		if (unlikely(le32_to_cpu(desc->status) & OWNbit)) {
1173 			netif_stop_queue(dev);

At conditional (3): "(tp)->msg_enable & 128 != 0" taking true path

1174 			net_tx_err(tp, KERN_ERR PFX
1175 				   "%s: BUG! Tx Ring full when queue awake!\n",
1176 				   dev->name);

Event leaked_storage: Returned without freeing storage "skb"
Also see events: [alloc_fn][var_assign]

1177 			return NETDEV_TX_BUSY;
1178 		}

------------snip--8<-------------

As far as i can see, skb_put() might return a fresh allocated skb, 
so adding a kfree_skb() here should fix these, or am i missing
something?

Thanks Eric

Re: Possible leaks in network drivers

[Randy.Dunlap]

On Wed, 21 Jun 2006 18:28:37 +0200 Eric Sesterhenn wrote:

> hi,
> 
> Coverity complains about several pretty similar resource leaks
> inside the net drivers, and i am not sure if those are real

Linux network driver development is done on the netdev
mailing list (netdev@vger.kernel.org).
You'll find a larger pool of the right people there...


> name				coverity #id
> 
> drivers/net/8390.c		623
> drivers/net/pcmcia/xirc2ps_cs.c	627
> drivers/net/sis190.c		628
> drivers/net/wireless/wavelan.c	634
> drivers/net/wireless/orinoco.c	661
> drivers/net/depca.c		1246
> drivers/net/hp100.c		1247
> drivers/net/smc9194.c		1248
> drivers/net/skge.c		1249
> 
> Its always in the hard_start_xmit() function
> of the driver. Where we call skb=skb_padto(skb, ETH_ZLEN),
> and dont free the skb later when something goes wrong.
> 
> Here is the output from the sis190.c case:
> 
> ------------snip--8<-------------
> 1158 		if (unlikely(skb->len < ETH_ZLEN)) {
> 
> Event alloc_fn: Called allocation function "skb_padto" [model]
> Event var_assign: Assigned variable "skb" to storage returned from "skb_padto"
> Also see events: [var_assign][leaked_storage]
> 
> 1159 			skb = skb_padto(skb, ETH_ZLEN);
> 
> At conditional (1): "skb == 0" taking false path
> 
> 1160 			if (!skb) {
> 1161 				tp->stats.tx_dropped++;
> 1162 				goto out;
> 1163 			}
> 1164 			len = ETH_ZLEN;
> 1165 		} else {
> 1166 			len = skb->len;
> 1167 		}
> 1168 	
> 1169 		entry = tp->cur_tx % NUM_TX_DESC;
> 1170 		desc = tp->TxDescRing + entry;
> 1171 	
> 
> At conditional (2): "(desc)->status & 2147483648 != 0" taking true path
> 
> 1172 		if (unlikely(le32_to_cpu(desc->status) & OWNbit)) {
> 1173 			netif_stop_queue(dev);
> 
> At conditional (3): "(tp)->msg_enable & 128 != 0" taking true path
> 
> 1174 			net_tx_err(tp, KERN_ERR PFX
> 1175 				   "%s: BUG! Tx Ring full when queue awake!\n",
> 1176 				   dev->name);
> 
> Event leaked_storage: Returned without freeing storage "skb"
> Also see events: [alloc_fn][var_assign]
> 
> 1177 			return NETDEV_TX_BUSY;
> 1178 		}
> 
> ------------snip--8<-------------
> 
> As far as i can see, skb_put() might return a fresh allocated skb, 
> so adding a kfree_skb() here should fix these, or am i missing
> something?
> 
> Thanks Eric

---
~Randy

Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Alan Cox]

Ar Mer, 2006-06-21 am 18:28 +0200, ysgrifennodd Eric Sesterhenn:
> of the driver. Where we call skb=skb_padto(skb, ETH_ZLEN),
> and dont free the skb later when something goes wrong.

skb_padto() returns either a new buffer or the existing one depending
upon the space situation. If it returns a new buffer then it frees the
old one.

The sequence is

dev_queue_xmit(skb)
	->hard_start_xmit(dev, skb)
	if (0)
		free skb
	return

Which I think means that the error path for a short packet would double
free the skb buffer and leak nskb.


So these drivers should indeed be checking their status before they
clone the buffer. At the point they do an skb_padto they must not fail
if the skb_padto succeeds.

In the case of 8390.c this was broken in 2.6.9 when the efficient 8390
padding code was replaced by something slower and it turns out broken -
although nobody realised that last bit until the checker came along.

See: http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-02/4480.html

Although reverting the change was proposed it was not actually done.


The following should do the trick:

Signed-off-by: Alan Cox <alan@redhat.com>

--- drivers/net/8390.c~	2006-06-21 17:41:12.006145536 +0100
+++ drivers/net/8390.c	2006-06-21 17:41:12.007145384 +0100
@@ -275,12 +275,14 @@
 	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
 	int send_length = skb->len, output_page;
 	unsigned long flags;
+	char buf[64];
+	char *data = skb->data;
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
+		memcpy(buf, data, ETH_ZLEN);
 		send_length = ETH_ZLEN;
+		data = buf;
 	}
 
 	/* Mask interrupts from the ethercard. 
@@ -347,7 +349,7 @@
 	 * trigger the send later, upon receiving a Tx done interrupt.
 	 */
 	 
-	ei_block_output(dev, send_length, skb->data, output_page);
+	ei_block_output(dev, send_length, data, output_page);
 		
 	if (! ei_local->txing) 
 	{

Re: Memory corruption in 8390.c ?

[Ben Pfaff]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> --- drivers/net/8390.c~	2006-06-21 17:41:12.006145536 +0100
> +++ drivers/net/8390.c	2006-06-21 17:41:12.007145384 +0100
> @@ -275,12 +275,14 @@
>  	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
>  	int send_length = skb->len, output_page;
>  	unsigned long flags;
> +	char buf[64];
> +	char *data = skb->data;
>  
>  	if (skb->len < ETH_ZLEN) {
> -		skb = skb_padto(skb, ETH_ZLEN);
> -		if (skb == NULL)
> -			return 0;
> +		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
> +		memcpy(buf, data, ETH_ZLEN);

Is this really correct?  It zeros out ETH_ZLEN bytes only to
immediately copy over all of them again.

>  		send_length = ETH_ZLEN;
> +		data = buf;
>  	}
>  
>  	/* Mask interrupts from the ethercard. 
> @@ -347,7 +349,7 @@
>  	 * trigger the send later, upon receiving a Tx done interrupt.
>  	 */
>  	 
> -	ei_block_output(dev, send_length, skb->data, output_page);
> +	ei_block_output(dev, send_length, data, output_page);
>  		
>  	if (! ei_local->txing) 
>  	{


-- 
Ben Pfaff 
email: blp@cs.stanford.edu
web: http://benpfaff.org

Re: Memory corruption in 8390.c ?

[Alan Cox]

Ar Mer, 2006-06-21 am 10:23 -0700, ysgrifennodd Ben Pfaff:
> > +		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
> > +		memcpy(buf, data, ETH_ZLEN);
> 
> Is this really correct?  It zeros out ETH_ZLEN bytes only to
> immediately copy over all of them again.

When I did it originally I tested with rdtsc and its actually quicker to
let it build the static memset the copy data over it than to do the
extra maths and the variable length loop.

Hence the comment

Re: Memory corruption in 8390.c ?

[Ben Pfaff]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Ar Mer, 2006-06-21 am 10:23 -0700, ysgrifennodd Ben Pfaff:
>> > +		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
>> > +		memcpy(buf, data, ETH_ZLEN);
>> 
>> Is this really correct?  It zeros out ETH_ZLEN bytes only to
>> immediately copy over all of them again.
>
> When I did it originally I tested with rdtsc and its actually quicker to
> let it build the static memset the copy data over it than to do the
> extra maths and the variable length loop.
>
> Hence the comment

You are saying that this:
        memset(buf, 0, ETH_ZLEN);
        memcpy(buf, data, ETH_ZLEN);
is faster than this?
        memcpy(buf, data, ETH_ZLEN);

Because as far as I can tell they are equivalent.
-- 
Ben Pfaff 
email: blp@cs.stanford.edu
web: http://benpfaff.org

Re: Memory corruption in 8390.c ?

[Alan Cox]

Ar Mer, 2006-06-21 am 11:03 -0700, ysgrifennodd Ben Pfaff:
> You are saying that this:
>         memset(buf, 0, ETH_ZLEN);
>         memcpy(buf, data, ETH_ZLEN);
> is faster than this?
>         memcpy(buf, data, ETH_ZLEN);
> 

No, you are noticing that the memcpy should be skb->len 8)

Alan

PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....)

[Alan Cox]

Ok this is my first pass at fixing all the reported cases.

8390 reverts to the faster pre 2.6.9 solution
HP100 moves the padto to a point after the fail cases
Ditto xirc2ps
Ditto smc9194
Orinoco was a mess, someone long ago managed to merge both the skb_padto
fix and the proper faster fix. The result worked (minus this newly
noticed problem) but was not neccessary. Removed the padto stuff
Wavelan uses the ETH_ZLEN sized buffer trick

The trickiest one is skge. For that I've grabbed an extra reference to
the original buffer and used that to keep the right things alive. Not
sure if it is the best way or the right way to play with refcounts.
Could do with more review.

Signed-off-by: Alan Cox <alan@redhat.com>


diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/8390.c linux-2.6.17/drivers/net/8390.c
--- linux.vanilla-2.6.17/drivers/net/8390.c	2006-06-19 17:17:32.000000000 +0100
+++ linux-2.6.17/drivers/net/8390.c	2006-06-21 17:41:12.007145384 +0100
@@ -275,12 +275,14 @@
 	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
 	int send_length = skb->len, output_page;
 	unsigned long flags;
+	char buf[ETH_ZLEN];
+	char *data = skb->data;
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
+		memcpy(buf, data, ETH_ZLEN);
 		send_length = ETH_ZLEN;
+		data = buf;
 	}
 
 	/* Mask interrupts from the ethercard. 
@@ -347,7 +349,7 @@
 	 * trigger the send later, upon receiving a Tx done interrupt.
 	 */
 	 
-	ei_block_output(dev, send_length, skb->data, output_page);
+	ei_block_output(dev, send_length, data, output_page);
 		
 	if (! ei_local->txing) 
 	{
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/hp100.c linux-2.6.17/drivers/net/hp100.c
--- linux.vanilla-2.6.17/drivers/net/hp100.c	2006-06-19 17:29:46.000000000 +0100
+++ linux-2.6.17/drivers/net/hp100.c	2006-06-21 17:52:01.211451328 +0100
@@ -1484,14 +1484,6 @@
 		return 0;
 	}
 
-	if (skb->len <= 0)
-		return 0;
-		
-	if (skb->len < ETH_ZLEN && lp->chip == HP100_CHIPID_SHASTA) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
 
 	/* Get Tx ring tail pointer */
 	if (lp->txrtail->next == lp->txrhead) {
@@ -1557,6 +1549,11 @@
 	ringptr->pdl[0] = ((1 << 16) | i);	/* PDH: 1 Fragment & length */
 	if (lp->chip == HP100_CHIPID_SHASTA) {
 		/* TODO:Could someone who has the EISA card please check if this works? */
+		if (skb->len < ETH_ZLEN) {
+			skb = skb_padto(skb, ETH_ZLEN);
+                	if (skb == NULL)
+				return 0;
+		}
 		ringptr->pdl[2] = i;
 	} else {		/* Lassen */
 		/* In the PDL, don't use the padded size but the real packet size: */
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/pcmcia/xirc2ps_cs.c linux-2.6.17/drivers/net/pcmcia/xirc2ps_cs.c
--- linux.vanilla-2.6.17/drivers/net/pcmcia/xirc2ps_cs.c	2006-06-19 17:29:46.000000000 +0100
+++ linux-2.6.17/drivers/net/pcmcia/xirc2ps_cs.c	2006-06-21 18:12:50.291562288 +0100
@@ -1359,27 +1359,11 @@
     kio_addr_t ioaddr = dev->base_addr;
     int okay;
     unsigned freespace;
-    unsigned pktlen = skb? skb->len : 0;
+    unsigned pktlen = max_t(unsigned, skb->len, ETH_ZLEN);
 
     DEBUG(1, "do_start_xmit(skb=%p, dev=%p) len=%u\n",
 	  skb, dev, pktlen);
 
-
-    /* adjust the packet length to min. required
-     * and hope that the buffer is large enough
-     * to provide some random data.
-     * fixme: For Mohawk we can change this by sending
-     * a larger packetlen than we actually have; the chip will
-     * pad this in his buffer with random bytes
-     */
-    if (pktlen < ETH_ZLEN)
-    {
-        skb = skb_padto(skb, ETH_ZLEN);
-        if (skb == NULL)
-        	return 0;
-	pktlen = ETH_ZLEN;
-    }
-
     netif_stop_queue(dev);
     SelectPage(0);
     PutWord(XIRCREG0_TRS, (u_short)pktlen+2);
@@ -1393,6 +1377,19 @@
     if (!okay) { /* not enough space */
 	return 1;  /* upper layer may decide to requeue this packet */
     }
+    /* adjust the packet length to min. required
+     * and hope that the buffer is large enough
+     * to provide some random data.
+     * fixme: For Mohawk we can change this by sending
+     * a larger packetlen than we actually have; the chip will
+     * pad this in his buffer with random bytes
+     */
+    if (skb->len < ETH_ZLEN)
+    {
+        skb = skb_padto(skb, ETH_ZLEN);
+        if (skb == NULL)
+        	return 0;
+    }
     /* send the packet */
     PutWord(XIRCREG_EDP, (u_short)pktlen);
     outsw(ioaddr+XIRCREG_EDP, skb->data, pktlen>>1);
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/sis190.c linux-2.6.17/drivers/net/sis190.c
--- linux.vanilla-2.6.17/drivers/net/sis190.c	2006-06-19 17:29:46.000000000 +0100
+++ linux-2.6.17/drivers/net/sis190.c	2006-06-21 17:46:47.975070496 +0100
@@ -1155,17 +1155,6 @@
 	struct TxDesc *desc;
 	dma_addr_t mapping;
 
-	if (unlikely(skb->len < ETH_ZLEN)) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (!skb) {
-			tp->stats.tx_dropped++;
-			goto out;
-		}
-		len = ETH_ZLEN;
-	} else {
-		len = skb->len;
-	}
-
 	entry = tp->cur_tx % NUM_TX_DESC;
 	desc = tp->TxDescRing + entry;
 
@@ -1177,6 +1166,17 @@
 		return NETDEV_TX_BUSY;
 	}
 
+	if (unlikely(skb->len < ETH_ZLEN)) {
+		skb = skb_padto(skb, ETH_ZLEN);
+		if (!skb) {
+			tp->stats.tx_dropped++;
+			goto out;
+		}
+		len = ETH_ZLEN;
+	} else {
+		len = skb->len;
+	}
+
 	mapping = pci_map_single(tp->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
 
 	tp->Tx_skbuff[entry] = skb;
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/skge.c linux-2.6.17/drivers/net/skge.c
--- linux.vanilla-2.6.17/drivers/net/skge.c	2006-06-19 17:29:47.000000000 +0100
+++ linux-2.6.17/drivers/net/skge.c	2006-06-21 18:20:10.785597016 +0100
@@ -2298,23 +2298,28 @@
 		+ (ring->to_clean - ring->to_use) - 1;
 }
 
-static int skge_xmit_frame(struct sk_buff *skb, struct net_device *dev)
+static int skge_xmit_frame(struct sk_buff *tx_skb, struct net_device *dev)
 {
 	struct skge_port *skge = netdev_priv(dev);
 	struct skge_hw *hw = skge->hw;
 	struct skge_ring *ring = &skge->tx_ring;
 	struct skge_element *e;
 	struct skge_tx_desc *td;
+	struct sk_buff *skb;
 	int i;
 	u32 control, len;
 	u64 map;
 
-	skb = skb_padto(skb, ETH_ZLEN);
-	if (!skb)
+	skb_get(tx_skb);
+	skb = skb_padto(tx_skb, ETH_ZLEN);
+	if (!skb) {
+		dev_kfree_skb(tx_skb);
 		return NETDEV_TX_OK;
+	}
 
 	if (!spin_trylock(&skge->tx_lock)) {
 		/* Collision - tell upper layer to requeue */
+		kfree(skb);
 		return NETDEV_TX_LOCKED;
 	}
 
@@ -2326,6 +2331,7 @@
 			       dev->name);
 		}
 		spin_unlock(&skge->tx_lock);
+		kfree(skb);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -2403,6 +2409,8 @@
 	spin_unlock(&skge->tx_lock);
 
 	dev->trans_start = jiffies;
+	/* Drop the reference, the completion handler will drop the final one */
+	kfree_skb(tx_skb);
 
 	return NETDEV_TX_OK;
 }
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/smc9194.c linux-2.6.17/drivers/net/smc9194.c
--- linux.vanilla-2.6.17/drivers/net/smc9194.c	2006-06-19 17:17:33.000000000 +0100
+++ linux-2.6.17/drivers/net/smc9194.c	2006-06-21 17:57:57.604271384 +0100
@@ -520,17 +520,8 @@
 	}
 	lp->saved_skb = skb;
 
-	length = skb->len;
+	length = max(skb->len, ETH_ZLEN);
 
-	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL) {
-			netif_wake_queue(dev);
-			return 0;
-		}
-		length = ETH_ZLEN;
-	}
-		
 	/*
 	** The MMU wants the number of pages to be the number of 256 bytes
 	** 'pages', minus 1 ( since a packet can't ever have 0 pages :) )
@@ -616,7 +607,7 @@
 	struct smc_local *lp = netdev_priv(dev);
 	byte	 		packet_no;
 	struct sk_buff * 	skb = lp->saved_skb;
-	word			length;
+	word			length, data_length;
 	unsigned int		ioaddr;
 	byte			* buf;
 
@@ -626,7 +617,16 @@
 		PRINTK((CARDNAME": In XMIT with no packet to send \n"));
 		return;
 	}
-	length = ETH_ZLEN < skb->len ? skb->len : ETH_ZLEN;
+
+	if (length < ETH_ZLEN) {
+		skb = skb_padto(skb, ETH_ZLEN);
+		if (skb == NULL) {
+			lp->saved_skb = NULL;
+			netif_wake_queue(dev);
+			return;
+		}
+		length = ETH_ZLEN;
+	}
 	buf = skb->data;
 
 	/* If I get here, I _know_ there is a packet slot waiting for me */
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/wireless/orinoco.c linux-2.6.17/drivers/net/wireless/orinoco.c
--- linux.vanilla-2.6.17/drivers/net/wireless/orinoco.c	2006-06-19 17:29:48.000000000 +0100
+++ linux-2.6.17/drivers/net/wireless/orinoco.c	2006-06-21 18:19:02.849924808 +0100
@@ -491,11 +491,8 @@
 	}
 
 	/* Length of the packet body */
-	/* FIXME: what if the skb is smaller than this? */
+	/* A shorter data_len will be padded by hermes_bap_pwrite_pad */
 	len = max_t(int, ALIGN(skb->len, 2), ETH_ZLEN);
-	skb = skb_padto(skb, len);
-	if (skb == NULL)
-		goto fail;
 	len -= ETH_HLEN;
 
 	eh = (struct ethhdr *)skb->data;
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/wireless/wavelan.c linux-2.6.17/drivers/net/wireless/wavelan.c
--- linux.vanilla-2.6.17/drivers/net/wireless/wavelan.c	2006-06-19 17:29:48.000000000 +0100
+++ linux-2.6.17/drivers/net/wireless/wavelan.c	2006-06-21 18:16:01.599479064 +0100
@@ -2903,6 +2903,7 @@
 {
 	net_local *lp = (net_local *) dev->priv;
 	unsigned long flags;
+	char data[ETH_ZLEN];
 
 #ifdef DEBUG_TX_TRACE
 	printk(KERN_DEBUG "%s: ->wavelan_packet_xmit(0x%X)\n", dev->name,
@@ -2937,15 +2938,16 @@
 	 * able to detect collisions, therefore in theory we don't really
 	 * need to pad. Jean II */
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(data, 0, ETH_ZLEN);
+		memcpy(data, skb->data, skb->len);
+		/* Write packet on the card */
+		if(wv_packet_write(dev, data, ETH_ZLEN))
+			return 1;	/* We failed */
 	}
-
-	/* Write packet on the card */
-	if(wv_packet_write(dev, skb->data, skb->len))
+	else if(wv_packet_write(dev, skb->data, skb->len))
 		return 1;	/* We failed */
 
+
 	dev_kfree_skb(skb);
 
 #ifdef DEBUG_TX_TRACE

Re: PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....)

[Olivier Galibert]

On Wed, Jun 21, 2006 at 06:59:40PM +0100, Alan Cox wrote:
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/8390.c linux-2.6.17/drivers/net/8390.c
> --- linux.vanilla-2.6.17/drivers/net/8390.c	2006-06-19 17:17:32.000000000 +0100
> +++ linux-2.6.17/drivers/net/8390.c	2006-06-21 17:41:12.007145384 +0100
> @@ -275,12 +275,14 @@
>  	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
>  	int send_length = skb->len, output_page;
>  	unsigned long flags;
> +	char buf[ETH_ZLEN];
> +	char *data = skb->data;
>  
>  	if (skb->len < ETH_ZLEN) {
> -		skb = skb_padto(skb, ETH_ZLEN);
> -		if (skb == NULL)
> -			return 0;
> +		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
> +		memcpy(buf, data, ETH_ZLEN);
                                  ^^^^^^^^
send_length surely?

>  		send_length = ETH_ZLEN;
> +		data = buf;
>  	}
>  
>  	/* Mask interrupts from the ethercard. 

  OG.

Re: Possible leaks in network drivers

[Eric Sesterhenn]

On Wed, 2006-06-21 at 18:13 +0100, Alan Cox wrote:
> Ar Mer, 2006-06-21 am 18:28 +0200, ysgrifennodd Eric Sesterhenn:
> > of the driver. Where we call skb=skb_padto(skb, ETH_ZLEN),
> > and dont free the skb later when something goes wrong.
> 
> skb_padto() returns either a new buffer or the existing one depending
> upon the space situation. If it returns a new buffer then it frees the
> old one.
> 
> The sequence is
> 
> dev_queue_xmit(skb)
> 	->hard_start_xmit(dev, skb)
> 	if (0)
> 		free skb
> 	return
> 
> Which I think means that the error path for a short packet would double
> free the skb buffer and leak nskb.
> 
> 
> So these drivers should indeed be checking their status before they
> clone the buffer. At the point they do an skb_padto they must not fail
> if the skb_padto succeeds.

So something like this would be the correct fix for the example?

Fix skb leak found by coverity checker (id #628), skb_put() might
return a new skb, which gets never freed when we return with
NETDEV_TX_BUSY. This patch moves the check above the skb_put().

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.17-git2/drivers/net/sis190.c.orig	2006-06-21 19:44:18.000000000 +0200
+++ linux-2.6.17-git2/drivers/net/sis190.c	2006-06-21 19:46:06.000000000 +0200
@@ -1155,6 +1155,18 @@ static int sis190_start_xmit(struct sk_b
 	struct TxDesc *desc;
 	dma_addr_t mapping;
 
+
+	entry = tp->cur_tx % NUM_TX_DESC;
+	desc = tp->TxDescRing + entry;
+
+	if (unlikely(le32_to_cpu(desc->status) & OWNbit)) {
+		netif_stop_queue(dev);
+		net_tx_err(tp, KERN_ERR PFX
+			"%s: BUG! Tx Ring full when queue awake!\n",
+			dev->name);
+		return NETDEV_TX_BUSY;
+	}
+
 	if (unlikely(skb->len < ETH_ZLEN)) {
 		skb = skb_padto(skb, ETH_ZLEN);
 		if (!skb) {
@@ -1166,17 +1178,6 @@ static int sis190_start_xmit(struct sk_b
 		len = skb->len;
 	}
 
-	entry = tp->cur_tx % NUM_TX_DESC;
-	desc = tp->TxDescRing + entry;
-
-	if (unlikely(le32_to_cpu(desc->status) & OWNbit)) {
-		netif_stop_queue(dev);
-		net_tx_err(tp, KERN_ERR PFX
-			   "%s: BUG! Tx Ring full when queue awake!\n",
-			   dev->name);
-		return NETDEV_TX_BUSY;
-	}
-
 	mapping = pci_map_single(tp->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
 
 	tp->Tx_skbuff[entry] = skb;

Re: Possible leaks in network drivers

[Herbert Xu]

Eric Sesterhenn <snakebyte@gmx.de> wrote:
> 
> So something like this would be the correct fix for the example?
> 
> Fix skb leak found by coverity checker (id #628), skb_put() might
> return a new skb, which gets never freed when we return with
> NETDEV_TX_BUSY. This patch moves the check above the skb_put().

This is bogus.  NETDEV_TX_BUSY is meant to requeue the skb.
The real problem is that copying the skb is simply wrong.

Cheers,
-- 
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: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Herbert Xu]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> skb_padto() returns either a new buffer or the existing one depending
> upon the space situation. If it returns a new buffer then it frees the
> old one.

I think skb_padto simply shouldn't allocate a new skb.  It only needs
to extend the data area.

Cheers,
-- 
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: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Herbert Xu]

On Thu, Jun 22, 2006 at 10:55:44AM +1000, Herbert Xu wrote:
> 
> I think skb_padto simply shouldn't allocate a new skb.  It only needs
> to extend the data area.

OK, here is a patch to make it do that.

[NET]: Avoid allocating skb in skb_pad

First of all it is unnecessary to allocate a new skb in skb_pad since
the existing one is not shared.  More importantly, our hard_start_xmit
interface does not allow a new skb to be allocated since that breaks
requeueing.

This patch uses pskb_expand_head to expand the existing skb and linearize
it if needed.  Actually, someone should sift through every instance of
skb_pad on a non-linear skb as they do not fit the reasons why this was
originally created.

Incidentally, this fixes a minor bug when the skb is cloned (tcpdump,
TCP, etc.).  As it is skb_pad will simply write over a cloned skb.  Because
of the position of the write it is unlikely to cause problems but still
it's best if we don't do it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
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
--
diff --git a/drivers/net/3c527.c b/drivers/net/3c527.c
index 1b1cb00..157eda5 100644
--- a/drivers/net/3c527.c
+++ b/drivers/net/3c527.c
@@ -1031,8 +1031,7 @@ static int mc32_send_packet(struct sk_bu
 		return 1;
 	}
 
-	skb = skb_padto(skb, ETH_ZLEN);
-	if (skb == NULL) {
+	if (skb_padto(skb, ETH_ZLEN)) {
 		netif_wake_queue(dev);
 		return 0;
 	}
diff --git a/drivers/net/82596.c b/drivers/net/82596.c
index da0c878..8a9f7d6 100644
--- a/drivers/net/82596.c
+++ b/drivers/net/82596.c
@@ -1070,8 +1070,7 @@ static int i596_start_xmit(struct sk_buf
 				skb->len, (unsigned int)skb->data));
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/8390.c b/drivers/net/8390.c
index f870274..00abe83 100644
--- a/drivers/net/8390.c
+++ b/drivers/net/8390.c
@@ -277,8 +277,7 @@ static int ei_start_xmit(struct sk_buff 
 	unsigned long flags;
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		send_length = ETH_ZLEN;
 	}
diff --git a/drivers/net/a2065.c b/drivers/net/a2065.c
index 79bb56b..71165ac 100644
--- a/drivers/net/a2065.c
+++ b/drivers/net/a2065.c
@@ -573,8 +573,7 @@ static int lance_start_xmit (struct sk_b
 	
 	if (len < ETH_ZLEN) {
 		len = ETH_ZLEN;
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 	}
 
diff --git a/drivers/net/ariadne.c b/drivers/net/ariadne.c
index d1b6b1f..a9bb7a4 100644
--- a/drivers/net/ariadne.c
+++ b/drivers/net/ariadne.c
@@ -607,8 +607,7 @@ #endif
     /* FIXME: is the 79C960 new enough to do its own padding right ? */
     if (skb->len < ETH_ZLEN)
     {
-    	skb = skb_padto(skb, ETH_ZLEN);
-    	if (skb == NULL)
+    	if (skb_padto(skb, ETH_ZLEN))
     	    return 0;
     	len = ETH_ZLEN;
     }
diff --git a/drivers/net/arm/ether1.c b/drivers/net/arm/ether1.c
index 36475eb..312955d 100644
--- a/drivers/net/arm/ether1.c
+++ b/drivers/net/arm/ether1.c
@@ -700,8 +700,7 @@ ether1_sendpacket (struct sk_buff *skb, 
 	}
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			goto out;
 	}
 
diff --git a/drivers/net/arm/ether3.c b/drivers/net/arm/ether3.c
index f1d5b10..0810741 100644
--- a/drivers/net/arm/ether3.c
+++ b/drivers/net/arm/ether3.c
@@ -518,8 +518,7 @@ ether3_sendpacket(struct sk_buff *skb, s
 
 	length = (length + 1) & ~1;
 	if (length != skb->len) {
-		skb = skb_padto(skb, length);
-		if (skb == NULL)
+		if (skb_padto(skb, length))
 			goto out;
 	}
 
diff --git a/drivers/net/atarilance.c b/drivers/net/atarilance.c
index 442b2cb..91783a8 100644
--- a/drivers/net/atarilance.c
+++ b/drivers/net/atarilance.c
@@ -804,8 +804,7 @@ static int lance_start_xmit( struct sk_b
 		++len;
 		
 	if (len > skb->len) {
-		skb = skb_padto(skb, len);
-		if (skb == NULL)
+		if (skb_padto(skb, len))
 			return 0;
 	}
 		
diff --git a/drivers/net/cassini.c b/drivers/net/cassini.c
index 39f36aa..565a54f 100644
--- a/drivers/net/cassini.c
+++ b/drivers/net/cassini.c
@@ -2915,8 +2915,7 @@ static int cas_start_xmit(struct sk_buff
 	 */
 	static int ring; 
 
-	skb = skb_padto(skb, cp->min_frame_size);
-	if (!skb)
+	if (skb_padto(skb, cp->min_frame_size))
 		return 0;
 
 	/* XXX: we need some higher-level QoS hooks to steer packets to
diff --git a/drivers/net/declance.c b/drivers/net/declance.c
index f130bda..d3d958e 100644
--- a/drivers/net/declance.c
+++ b/drivers/net/declance.c
@@ -885,8 +885,7 @@ static int lance_start_xmit(struct sk_bu
 	len = skblen;
 	
 	if (len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		len = ETH_ZLEN;
 	}
diff --git a/drivers/net/depca.c b/drivers/net/depca.c
index 0941d40..e946c43 100644
--- a/drivers/net/depca.c
+++ b/drivers/net/depca.c
@@ -938,11 +938,8 @@ static int depca_start_xmit(struct sk_bu
 	if (skb->len < 1)
 		goto out;
 
-	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			goto out;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		goto out;
 	
 	netif_stop_queue(dev);
 
diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c
index a806dfe..e70f172 100644
--- a/drivers/net/eepro.c
+++ b/drivers/net/eepro.c
@@ -1154,8 +1154,7 @@ static int eepro_send_packet(struct sk_b
 		printk(KERN_DEBUG  "%s: entering eepro_send_packet routine.\n", dev->name);
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
index 82bd356..a74b207 100644
--- a/drivers/net/eexpress.c
+++ b/drivers/net/eexpress.c
@@ -677,8 +677,7 @@ #if NET_DEBUG > 6
 #endif
 
 	if (buf->len < ETH_ZLEN) {
-		buf = skb_padto(buf, ETH_ZLEN);
-		if (buf == NULL)
+		if (skb_padto(buf, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/epic100.c b/drivers/net/epic100.c
index 8d680ce..724d7dc 100644
--- a/drivers/net/epic100.c
+++ b/drivers/net/epic100.c
@@ -1027,11 +1027,8 @@ static int epic_start_xmit(struct sk_buf
 	u32 ctrl_word;
 	unsigned long flags;
 
-	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return 0;
 
 	/* Caution: the write order is important here, set the field with the
 	   "ownership" bit last. */
diff --git a/drivers/net/eth16i.c b/drivers/net/eth16i.c
index b67545b..4bf76f8 100644
--- a/drivers/net/eth16i.c
+++ b/drivers/net/eth16i.c
@@ -1064,8 +1064,7 @@ static int eth16i_tx(struct sk_buff *skb
 	unsigned long flags;
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/hp100.c b/drivers/net/hp100.c
index 247c8ca..dd1dc32 100644
--- a/drivers/net/hp100.c
+++ b/drivers/net/hp100.c
@@ -1487,11 +1487,8 @@ #endif
 	if (skb->len <= 0)
 		return 0;
 		
-	if (skb->len < ETH_ZLEN && lp->chip == HP100_CHIPID_SHASTA) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
+	if (lp->chip == HP100_CHIPID_SHASTA && skb_padto(skb, ETH_ZLEN))
+		return 0;
 
 	/* Get Tx ring tail pointer */
 	if (lp->txrtail->next == lp->txrhead) {
diff --git a/drivers/net/lance.c b/drivers/net/lance.c
index bb5ad47..c1c3452 100644
--- a/drivers/net/lance.c
+++ b/drivers/net/lance.c
@@ -968,8 +968,7 @@ static int lance_start_xmit(struct sk_bu
 	/* The old LANCE chips doesn't automatically pad buffers to min. size. */
 	if (chip_table[lp->chip_version].flags & LANCE_MUST_PAD) {
 		if (skb->len < ETH_ZLEN) {
-			skb = skb_padto(skb, ETH_ZLEN);
-			if (skb == NULL)
+			if (skb_padto(skb, ETH_ZLEN))
 				goto out;
 			lp->tx_ring[entry].length = -ETH_ZLEN;
 		}
diff --git a/drivers/net/lasi_82596.c b/drivers/net/lasi_82596.c
index 957888d..1ab0944 100644
--- a/drivers/net/lasi_82596.c
+++ b/drivers/net/lasi_82596.c
@@ -1083,8 +1083,7 @@ static int i596_start_xmit(struct sk_buf
 				skb->len, skb->data));
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/lp486e.c b/drivers/net/lp486e.c
index 94d5ea1..bf3f343 100644
--- a/drivers/net/lp486e.c
+++ b/drivers/net/lp486e.c
@@ -877,8 +877,7 @@ static int i596_start_xmit (struct sk_bu
 	length = skb->len;
 	
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index e1feb58..b245476 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -1939,8 +1939,7 @@ #endif				/*NETIF_F_TSO */
 
 		/* pad frames to at least ETH_ZLEN bytes */
 		if (unlikely(skb->len < ETH_ZLEN)) {
-			skb = skb_padto(skb, ETH_ZLEN);
-			if (skb == NULL) {
+			if (skb_padto(skb, ETH_ZLEN)) {
 				/* The packet is gone, so we must
 				 * return 0 */
 				mgp->stats.tx_dropped += 1;
diff --git a/drivers/net/pcmcia/fmvj18x_cs.c b/drivers/net/pcmcia/fmvj18x_cs.c
index 09b1176..ea93b8f 100644
--- a/drivers/net/pcmcia/fmvj18x_cs.c
+++ b/drivers/net/pcmcia/fmvj18x_cs.c
@@ -831,8 +831,7 @@ static int fjn_start_xmit(struct sk_buff
     
     if (length < ETH_ZLEN)
     {
-    	skb = skb_padto(skb, ETH_ZLEN);
-    	if (skb == NULL)
+    	if (skb_padto(skb, ETH_ZLEN))
     		return 0;
     	length = ETH_ZLEN;
     }
diff --git a/drivers/net/pcmcia/xirc2ps_cs.c b/drivers/net/pcmcia/xirc2ps_cs.c
index 71f4505..54bbfda 100644
--- a/drivers/net/pcmcia/xirc2ps_cs.c
+++ b/drivers/net/pcmcia/xirc2ps_cs.c
@@ -1374,8 +1374,7 @@ do_start_xmit(struct sk_buff *skb, struc
      */
     if (pktlen < ETH_ZLEN)
     {
-        skb = skb_padto(skb, ETH_ZLEN);
-        if (skb == NULL)
+        if (skb_padto(skb, ETH_ZLEN))
         	return 0;
 	pktlen = ETH_ZLEN;
     }
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 9945cc6..985afe0 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2222,8 +2222,7 @@ static int rtl8169_start_xmit(struct sk_
 		len = skb->len;
 
 		if (unlikely(len < ETH_ZLEN)) {
-			skb = skb_padto(skb, ETH_ZLEN);
-			if (!skb)
+			if (skb_padto(skb, ETH_ZLEN))
 				goto err_update_stats;
 			len = ETH_ZLEN;
 		}
diff --git a/drivers/net/seeq8005.c b/drivers/net/seeq8005.c
index bcef03f..efd0f23 100644
--- a/drivers/net/seeq8005.c
+++ b/drivers/net/seeq8005.c
@@ -396,8 +396,7 @@ static int seeq8005_send_packet(struct s
 	unsigned char *buf;
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/sis190.c b/drivers/net/sis190.c
index 31dd3f0..df39f34 100644
--- a/drivers/net/sis190.c
+++ b/drivers/net/sis190.c
@@ -1156,8 +1156,7 @@ static int sis190_start_xmit(struct sk_b
 	dma_addr_t mapping;
 
 	if (unlikely(skb->len < ETH_ZLEN)) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (!skb) {
+		if (skb_padto(skb, ETH_ZLEN)) {
 			tp->stats.tx_dropped++;
 			goto out;
 		}
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index 38a26df..f3efbd1 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -1525,7 +1525,7 @@ #endif
 	** This is to resolve faulty padding by the HW with 0xaa bytes.
 	*/
 	if (BytesSend < C_LEN_ETHERNET_MINSIZE) {
-		if ((pMessage = skb_padto(pMessage, C_LEN_ETHERNET_MINSIZE)) == NULL) {
+		if (skb_padto(pMessage, C_LEN_ETHERNET_MINSIZE)) {
 			spin_unlock_irqrestore(&pTxPort->TxDesRingLock, Flags);
 			return 0;
 		}
diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 536dd1c..19a4a16 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -2310,8 +2310,7 @@ static int skge_xmit_frame(struct sk_buf
 	u64 map;
 	unsigned long flags;
 
-	skb = skb_padto(skb, ETH_ZLEN);
-	if (!skb)
+	if (skb_padto(skb, ETH_ZLEN))
 		return NETDEV_TX_OK;
 
 	if (!spin_trylock_irqsave(&skge->tx_lock, flags))
diff --git a/drivers/net/smc9194.c b/drivers/net/smc9194.c
index 6cf16f3..8b0321f 100644
--- a/drivers/net/smc9194.c
+++ b/drivers/net/smc9194.c
@@ -523,8 +523,7 @@ static int smc_wait_to_send_packet( stru
 	length = skb->len;
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL) {
+		if (skb_padto(skb, ETH_ZLEN)) {
 			netif_wake_queue(dev);
 			return 0;
 		}
diff --git a/drivers/net/sonic.c b/drivers/net/sonic.c
index 90b818a..cab0dd9 100644
--- a/drivers/net/sonic.c
+++ b/drivers/net/sonic.c
@@ -231,8 +231,7 @@ static int sonic_send_packet(struct sk_b
 
 	length = skb->len;
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/drivers/net/starfire.c b/drivers/net/starfire.c
index 9b7805b..c158eed 100644
--- a/drivers/net/starfire.c
+++ b/drivers/net/starfire.c
@@ -1349,8 +1349,7 @@ static int start_tx(struct sk_buff *skb,
 
 #if defined(ZEROCOPY) && defined(HAS_BROKEN_FIRMWARE)
 	if (skb->ip_summed == CHECKSUM_HW) {
-		skb = skb_padto(skb, (skb->len + PADDING_MASK) & ~PADDING_MASK);
-		if (skb == NULL)
+		if (skb_padto(skb, (skb->len + PADDING_MASK) & ~PADDING_MASK))
 			return NETDEV_TX_OK;
 	}
 #endif /* ZEROCOPY && HAS_BROKEN_FIRMWARE */
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index fdc2103..c80a4f1 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -1284,11 +1284,8 @@ static int rhine_start_tx(struct sk_buff
 	/* Calculate the next Tx descriptor entry. */
 	entry = rp->cur_tx % TX_RING_SIZE;
 
-	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return 0;
 
 	rp->tx_skbuff[entry] = skb;
 
diff --git a/drivers/net/wireless/ray_cs.c b/drivers/net/wireless/ray_cs.c
index 879eb42..a915fe6 100644
--- a/drivers/net/wireless/ray_cs.c
+++ b/drivers/net/wireless/ray_cs.c
@@ -924,8 +924,7 @@ static int ray_dev_start_xmit(struct sk_
 
     if (length < ETH_ZLEN)
     {
-    	skb = skb_padto(skb, ETH_ZLEN);
-    	if (skb == NULL)
+    	if (skb_padto(skb, ETH_ZLEN))
     		return 0;
     	length = ETH_ZLEN;
     }
diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
index dade4b9..aba56af 100644
--- a/drivers/net/wireless/wavelan.c
+++ b/drivers/net/wireless/wavelan.c
@@ -2936,11 +2936,8 @@ #endif
 	 * and we don't have the Ethernet specific requirement of beeing
 	 * able to detect collisions, therefore in theory we don't really
 	 * need to pad. Jean II */
-	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return 0;
 
 	/* Write packet on the card */
 	if(wv_packet_write(dev, skb->data, skb->len))
diff --git a/drivers/net/wireless/wavelan_cs.c b/drivers/net/wireless/wavelan_cs.c
index f7724eb..561250f 100644
--- a/drivers/net/wireless/wavelan_cs.c
+++ b/drivers/net/wireless/wavelan_cs.c
@@ -3194,11 +3194,8 @@ #endif
 	 * and we don't have the Ethernet specific requirement of beeing
 	 * able to detect collisions, therefore in theory we don't really
 	 * need to pad. Jean II */
-	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return 0;
 
   wv_packet_write(dev, skb->data, skb->len);
 
diff --git a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c
index fd0f43b..ecec8e5 100644
--- a/drivers/net/yellowfin.c
+++ b/drivers/net/yellowfin.c
@@ -862,13 +862,11 @@ static int yellowfin_start_xmit(struct s
 		/* Fix GX chipset errata. */
 		if (cacheline_end > 24  || cacheline_end == 0) {
 			len = skb->len + 32 - cacheline_end + 1;
-			if (len != skb->len)
-				skb = skb_padto(skb, len);
-		}
-		if (skb == NULL) {
-			yp->tx_skbuff[entry] = NULL;
-			netif_wake_queue(dev);
-			return 0;
+			if (skb_padto(skb, len)) {
+				yp->tx_skbuff[entry] = NULL;
+				netif_wake_queue(dev);
+				return 0;
+			}
 		}
 	}
 	yp->tx_skbuff[entry] = skb;
diff --git a/drivers/net/znet.c b/drivers/net/znet.c
index 3ac047b..a7c089d 100644
--- a/drivers/net/znet.c
+++ b/drivers/net/znet.c
@@ -544,8 +544,7 @@ static int znet_send_packet(struct sk_bu
 		printk(KERN_DEBUG "%s: ZNet_send_packet.\n", dev->name);
 
 	if (length < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
+		if (skb_padto(skb, ETH_ZLEN))
 			return 0;
 		length = ETH_ZLEN;
 	}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 66f8819..f8c7eb7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -345,7 +345,7 @@ extern struct sk_buff *skb_realloc_headr
 extern struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 				       int newheadroom, int newtailroom,
 				       gfp_t priority);
-extern struct sk_buff *		skb_pad(struct sk_buff *skb, int pad);
+extern int	       skb_pad(struct sk_buff *skb, int pad);
 #define dev_kfree_skb(a)	kfree_skb(a)
 extern void	      skb_over_panic(struct sk_buff *skb, int len,
 				     void *here);
@@ -1122,16 +1122,15 @@ static inline int skb_cow(struct sk_buff
  *
  *	Pads up a buffer to ensure the trailing bytes exist and are
  *	blanked. If the buffer already contains sufficient data it
- *	is untouched. Returns the buffer, which may be a replacement
- *	for the original, or NULL for out of memory - in which case
- *	the original buffer is still freed.
+ *	is untouched. Otherwise it is extended. Returns zero on
+ *	success. The skb is freed on error.
  */
  
-static inline struct sk_buff *skb_padto(struct sk_buff *skb, unsigned int len)
+static inline int skb_padto(struct sk_buff *skb, unsigned int len)
 {
 	unsigned int size = skb->len;
 	if (likely(size >= len))
-		return skb;
+		return 0;
 	return skb_pad(skb, len-size);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index bb7210f..fe63d4e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -781,24 +781,40 @@ struct sk_buff *skb_copy_expand(const st
  *	filled. Used by network drivers which may DMA or transfer data
  *	beyond the buffer end onto the wire.
  *
- *	May return NULL in out of memory cases.
+ *	May return error in out of memory cases. The skb is freed on error.
  */
  
-struct sk_buff *skb_pad(struct sk_buff *skb, int pad)
+int skb_pad(struct sk_buff *skb, int pad)
 {
-	struct sk_buff *nskb;
+	int err;
+	int ntail;
 	
 	/* If the skbuff is non linear tailroom is always zero.. */
-	if (skb_tailroom(skb) >= pad) {
+	if (!skb_cloned(skb) && skb_tailroom(skb) >= pad) {
 		memset(skb->data+skb->len, 0, pad);
-		return skb;
+		return 0;
 	}
-	
-	nskb = skb_copy_expand(skb, skb_headroom(skb), skb_tailroom(skb) + pad, GFP_ATOMIC);
+
+	ntail = skb->data_len + pad - (skb->end - skb->tail);
+	if (likely(skb_cloned(skb) || ntail > 0)) {
+		err = pskb_expand_head(skb, 0, ntail, GFP_ATOMIC);
+		if (unlikely(err))
+			goto free_skb;
+	}
+
+	/* FIXME: The use of this function with non-linear skb's really needs
+	 * to be audited.
+	 */
+	err = skb_linearize(skb);
+	if (unlikely(err))
+		goto free_skb;
+
+	memset(skb->data + skb->len, 0, pad);
+	return 0;
+
+free_skb:
 	kfree_skb(skb);
-	if (nskb)
-		memset(nskb->data+nskb->len, 0, pad);
-	return nskb;
+	return err;
 }	
  
 /* Trims skb to length len. It can change skb pointers.

Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Jeff Garzik]

Herbert Xu wrote:
> This patch uses pskb_expand_head to expand the existing skb and linearize

Seems sane to me.


> it if needed.  Actually, someone should sift through every instance of
> skb_pad on a non-linear skb as they do not fit the reasons why this was
> originally created.

Non-linear skbs smaller than ETH_ZLEN seem unlikely.

Overall, the skb_pad() changes were made over a short span of time, 
often to older and under-used drivers, so I would not be surprised to 
find rough edges or the occasional bug.

	Jeff

Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Herbert Xu]

On Thu, Jun 22, 2006 at 04:22:22AM -0400, Jeff Garzik wrote:
> 
> >it if needed.  Actually, someone should sift through every instance of
> >skb_pad on a non-linear skb as they do not fit the reasons why this was
> >originally created.
> 
> Non-linear skbs smaller than ETH_ZLEN seem unlikely.

When I was grepping it seems that a few drivers were using it with
a length other than ETH_ZLEN.  I've just done another grep and here
are the potential suspects:

cassini.c
starfire.c
yellowfin.c

Also, the skb_pad in drivers/s390/net/claw.c didn't check for errors
at all.

Cheers,
-- 
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: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Jeff Garzik]

Herbert Xu wrote:
> On Thu, Jun 22, 2006 at 04:22:22AM -0400, Jeff Garzik wrote:
>>> it if needed.  Actually, someone should sift through every instance of
>>> skb_pad on a non-linear skb as they do not fit the reasons why this was
>>> originally created.
>> Non-linear skbs smaller than ETH_ZLEN seem unlikely.
> 
> When I was grepping it seems that a few drivers were using it with
> a length other than ETH_ZLEN.  I've just done another grep and here
> are the potential suspects:
> 
> cassini.c
> starfire.c
> yellowfin.c

That doesn't really invalidate the point :)  These drivers are still 
only padding very small packets.

	Jeff

Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Herbert Xu]

On Thu, Jun 22, 2006 at 04:57:39AM -0400, Jeff Garzik wrote:
>
> >>Non-linear skbs smaller than ETH_ZLEN seem unlikely.
> >
> >When I was grepping it seems that a few drivers were using it with
> >a length other than ETH_ZLEN.  I've just done another grep and here
> >are the potential suspects:
> >
> >cassini.c
> >starfire.c
> >yellowfin.c
> 
> That doesn't really invalidate the point :)  These drivers are still 
> only padding very small packets.

Hmm, at least cassini pads it to 255 for gigabit...

Cheers,
-- 
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: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Herbert Xu]

On Thu, Jun 22, 2006 at 07:02:27PM +1000, herbert wrote:
>
> > >cassini.c
> > >starfire.c
> > >yellowfin.c
> > 
> > That doesn't really invalidate the point :)  These drivers are still 
> > only padding very small packets.
> 
> Hmm, at least cassini pads it to 255 for gigabit...

The one in starfire looks especially dodgy.  It supports SG and also
requires the whole length to be a multiple of 4 if the firmware is
broken.  The question is do they really intend this or do they want
each fragment to terminate on a 4-byte boundary.

Cheers,
-- 
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: Memory corruption in 8390.c ?

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 22 Jun 2006 12:30:29 +1000

> On Thu, Jun 22, 2006 at 10:55:44AM +1000, Herbert Xu wrote:
> > 
> > I think skb_padto simply shouldn't allocate a new skb.  It only needs
> > to extend the data area.
> 
> OK, here is a patch to make it do that.
> 
> [NET]: Avoid allocating skb in skb_pad

Want me to let this cook in 2.6.18 for a while before sending
it off to -stable?

Re: Memory corruption in 8390.c ?

[Herbert Xu]

On Thu, Jun 22, 2006 at 01:26:09AM -0700, David Miller wrote:
>
> Want me to let this cook in 2.6.18 for a while before sending
> it off to -stable?

You know I'm never one to push anything quickly so absolutely yes :)
-- 
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: Memory corruption in 8390.c ?

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 22 Jun 2006 18:30:37 +1000

> On Thu, Jun 22, 2006 at 01:26:09AM -0700, David Miller wrote:
> >
> > Want me to let this cook in 2.6.18 for a while before sending
> > it off to -stable?
> 
> You know I'm never one to push anything quickly so absolutely yes :)

Ok, applied to net-2.6.18 for now :)

Re: Memory corruption in 8390.c ?

[Alan Cox]

Ar Iau, 2006-06-22 am 01:34 -0700, ysgrifennodd David Miller:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 22 Jun 2006 18:30:37 +1000
> 
> > On Thu, Jun 22, 2006 at 01:26:09AM -0700, David Miller wrote:
> > >
> > > Want me to let this cook in 2.6.18 for a while before sending
> > > it off to -stable?
> > 
> > You know I'm never one to push anything quickly so absolutely yes :)
> 
> Ok, applied to net-2.6.18 for now :)

The 8390 change (corrected version) also makes 8390.c faster so should
be applied anyway, and the orinoco one fixes some code that isn't even
needed and someone forgot to remove long ago. Otherwise the skb_padto
behaviour change with the newer skb style makes a lot more sense I
agree.

Re: Memory corruption in 8390.c ?

[Herbert Xu]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> The 8390 change (corrected version) also makes 8390.c faster so should
> be applied anyway, and the orinoco one fixes some code that isn't even
> needed and someone forgot to remove long ago. Otherwise the skb_padto

Yeah I agree totally.  However, I haven't actually seen the fixed 8390
version being posted yet or at least not to netdev :)

Cheers,
-- 
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: Memory corruption in 8390.c ?

[Alan Cox]

Ar Iau, 2006-06-22 am 21:29 +1000, ysgrifennodd Herbert Xu:
> Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > 
> > The 8390 change (corrected version) also makes 8390.c faster so should
> > be applied anyway, and the orinoco one fixes some code that isn't even
> > needed and someone forgot to remove long ago. Otherwise the skb_padto
> 
> Yeah I agree totally.  However, I haven't actually seen the fixed 8390
> version being posted yet or at least not to netdev :)

Ah the resounding clang of a subtle hint ;)

Signed-off-by: Alan Cox <alan@redhat.com>

- Return 8390.c to the old way of handling short packets (which is also
faster)

- Remove the skb_padto from orinoco. This got left in when the padding bad 
write patch was added and is actually not needed. This is fixing a merge
error way back when.

- Wavelan can also use the stack based buffer trick if you want



diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/8390.c linux-2.6.17/drivers/net/8390.c
--- linux.vanilla-2.6.17/drivers/net/8390.c	2006-06-19 17:17:32.000000000 +0100
+++ linux-2.6.17/drivers/net/8390.c	2006-06-21 21:23:12.000000000 +0100
@@ -275,12 +275,14 @@
 	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
 	int send_length = skb->len, output_page;
 	unsigned long flags;
+	char buf[ETH_ZLEN];
+	char *data = skb->data;
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
+		memcpy(buf, data, skb->len);
 		send_length = ETH_ZLEN;
+		data = buf;
 	}
 
 	/* Mask interrupts from the ethercard. 
@@ -347,7 +349,7 @@
 	 * trigger the send later, upon receiving a Tx done interrupt.
 	 */
 	 
-	ei_block_output(dev, send_length, skb->data, output_page);
+	ei_block_output(dev, send_length, data, output_page);
 		
 	if (! ei_local->txing) 
 	{
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/wireless/orinoco.c linux-2.6.17/drivers/net/wireless/orinoco.c
--- linux.vanilla-2.6.17/drivers/net/wireless/orinoco.c	2006-06-19 17:29:48.000000000 +0100
+++ linux-2.6.17/drivers/net/wireless/orinoco.c	2006-06-21 18:19:02.000000000 +0100
@@ -491,11 +491,8 @@
 	}
 
 	/* Length of the packet body */
-	/* FIXME: what if the skb is smaller than this? */
+	/* A shorter data_len will be padded by hermes_bap_pwrite_pad */
 	len = max_t(int, ALIGN(skb->len, 2), ETH_ZLEN);
-	skb = skb_padto(skb, len);
-	if (skb == NULL)
-		goto fail;
 	len -= ETH_HLEN;
 
 	eh = (struct ethhdr *)skb->data;
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17/drivers/net/wireless/wavelan.c linux-2.6.17/drivers/net/wireless/wavelan.c
--- linux.vanilla-2.6.17/drivers/net/wireless/wavelan.c	2006-06-19 17:29:48.000000000 +0100
+++ linux-2.6.17/drivers/net/wireless/wavelan.c	2006-06-21 18:32:47.000000000 +0100
@@ -2903,6 +2903,7 @@
 {
 	net_local *lp = (net_local *) dev->priv;
 	unsigned long flags;
+	char data[ETH_ZLEN];
 
 #ifdef DEBUG_TX_TRACE
 	printk(KERN_DEBUG "%s: ->wavelan_packet_xmit(0x%X)\n", dev->name,
@@ -2937,15 +2938,16 @@
 	 * able to detect collisions, therefore in theory we don't really
 	 * need to pad. Jean II */
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(data, 0, ETH_ZLEN);
+		memcpy(data, skb->data, skb->len);
+		/* Write packet on the card */
+		if(wv_packet_write(dev, data, ETH_ZLEN))
+			return 1;	/* We failed */
 	}
-
-	/* Write packet on the card */
-	if(wv_packet_write(dev, skb->data, skb->len))
+	else if(wv_packet_write(dev, skb->data, skb->len))
 		return 1;	/* We failed */
 
+
 	dev_kfree_skb(skb);
 
 #ifdef DEBUG_TX_TRACE

Re: Memory corruption in 8390.c ?

[Jeff Garzik]

applied except for orinoco, which failed to apply (rediff?)

Re: Memory corruption in 8390.c ?

[Arjan van de Ven]

On Thu, 2006-06-22 at 12:34 +0100, Alan Cox wrote:
> Ar Iau, 2006-06-22 am 01:34 -0700, ysgrifennodd David Miller:
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > Date: Thu, 22 Jun 2006 18:30:37 +1000
> > 
> > > On Thu, Jun 22, 2006 at 01:26:09AM -0700, David Miller wrote:
> > > >
> > > > Want me to let this cook in 2.6.18 for a while before sending
> > > > it off to -stable?
> > > 
> > > You know I'm never one to push anything quickly so absolutely yes :)
> > 
> > Ok, applied to net-2.6.18 for now :)
> 
> The 8390 change (corrected version) also makes 8390.c faster so should
> be applied anyway, 

8390 is such a race monster that a few cycles matter a lot! :-)

Re: Memory corruption in 8390.c ?

[Erik Mouw]

On Thu, Jun 22, 2006 at 01:33:36PM +0200, Arjan van de Ven wrote:
> On Thu, 2006-06-22 at 12:34 +0100, Alan Cox wrote:
> > The 8390 change (corrected version) also makes 8390.c faster so should
> > be applied anyway, 
> 
> 8390 is such a race monster that a few cycles matter a lot! :-)

It sure is. Back in the old days I could saturate a 10 Mbit ethernet
segment using a Western Digital 8003 (the 8 bit ISA card) in a 386DX40
(running Linux 1.0, 1.2, and 1.3).


Erik

-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands

Re: Memory corruption in 8390.c ?

[Alan Cox]

Ar Iau, 2006-06-22 am 13:33 +0200, ysgrifennodd Arjan van de Ven:
> > The 8390 change (corrected version) also makes 8390.c faster so should
> > be applied anyway, 
> 
> 8390 is such a race monster that a few cycles matter a lot! :-)

There are generic 8390 clones for 100Mbit. I'm not suggesting its a good
idea but people did it.

Alan