Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
From: Herbert Xu [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
From: Herbert Xu [EMAIL PROTECTED] 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 :) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
Ar Iau, 2006-06-22 am 01:34 -0700, ysgrifennodd David Miller: From: Herbert Xu [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
Alan Cox [EMAIL PROTECTED] 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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
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 [EMAIL PROTECTED] 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! :-) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
Ar Iau, 2006-06-22 am 21:29 +1000, ysgrifennodd Herbert Xu:
Alan Cox [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
- 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.0
+0100
+++ linux-2.6.17/drivers/net/8390.c 2006-06-21 21:23:12.0 +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.0 +0100
+++ linux-2.6.17/drivers/net/wireless/orinoco.c 2006-06-21 18:19:02.0
+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.0 +0100
+++ linux-2.6.17/drivers/net/wireless/wavelan.c 2006-06-21 18:32:47.0
+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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ?
applied except for orinoco, which failed to apply (rediff?) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
Alan Cox [EMAIL PROTECTED] 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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
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 [EMAIL PROTECTED]
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
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
