* Possible leaks in network drivers
@ 2006-06-21 16:28 Eric Sesterhenn
2006-06-21 17:05 ` Randy.Dunlap
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
0 siblings, 2 replies; 28+ messages in thread
From: Eric Sesterhenn @ 2006-06-21 16:28 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Possible leaks in network drivers
2006-06-21 16:28 Possible leaks in network drivers Eric Sesterhenn
@ 2006-06-21 17:05 ` Randy.Dunlap
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
1 sibling, 0 replies; 28+ messages in thread
From: Randy.Dunlap @ 2006-06-21 17:05 UTC (permalink / raw)
To: Eric Sesterhenn; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-21 16:28 Possible leaks in network drivers Eric Sesterhenn
2006-06-21 17:05 ` Randy.Dunlap
@ 2006-06-21 17:13 ` Alan Cox
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
` (2 more replies)
1 sibling, 3 replies; 28+ messages in thread
From: Alan Cox @ 2006-06-21 17:13 UTC (permalink / raw)
To: Eric Sesterhenn; +Cc: linux-kernel, jgarzik
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)
{
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
@ 2006-06-21 17:23 ` Ben Pfaff
2006-06-21 17:54 ` Alan Cox
2006-06-21 17:59 ` PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....) Alan Cox
2006-06-21 17:50 ` Possible leaks in network drivers Eric Sesterhenn
2006-06-22 0:55 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Herbert Xu
2 siblings, 2 replies; 28+ messages in thread
From: Ben Pfaff @ 2006-06-21 17:23 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Possible leaks in network drivers
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
@ 2006-06-21 17:50 ` Eric Sesterhenn
2006-06-22 1:41 ` Herbert Xu
2006-06-22 0:55 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Herbert Xu
2 siblings, 1 reply; 28+ messages in thread
From: Eric Sesterhenn @ 2006-06-21 17:50 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, jgarzik, kmliu
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;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
@ 2006-06-21 17:54 ` Alan Cox
2006-06-21 18:03 ` Ben Pfaff
2006-06-21 17:59 ` PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....) Alan Cox
1 sibling, 1 reply; 28+ messages in thread
From: Alan Cox @ 2006-06-21 17:54 UTC (permalink / raw)
To: blp; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....)
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
2006-06-21 17:54 ` Alan Cox
@ 2006-06-21 17:59 ` Alan Cox
2006-06-21 19:00 ` Olivier Galibert
1 sibling, 1 reply; 28+ messages in thread
From: Alan Cox @ 2006-06-21 17:59 UTC (permalink / raw)
To: linux-kernel; +Cc: jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-21 17:54 ` Alan Cox
@ 2006-06-21 18:03 ` Ben Pfaff
2006-06-21 20:50 ` Alan Cox
0 siblings, 1 reply; 28+ messages in thread
From: Ben Pfaff @ 2006-06-21 18:03 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....)
2006-06-21 17:59 ` PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....) Alan Cox
@ 2006-06-21 19:00 ` Olivier Galibert
0 siblings, 0 replies; 28+ messages in thread
From: Olivier Galibert @ 2006-06-21 19:00 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, jgarzik, netdev
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-21 18:03 ` Ben Pfaff
@ 2006-06-21 20:50 ` Alan Cox
0 siblings, 0 replies; 28+ messages in thread
From: Alan Cox @ 2006-06-21 20:50 UTC (permalink / raw)
To: blp; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
2006-06-21 17:50 ` Possible leaks in network drivers Eric Sesterhenn
@ 2006-06-22 0:55 ` Herbert Xu
2006-06-22 2:30 ` Herbert Xu
2 siblings, 1 reply; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 0:55 UTC (permalink / raw)
To: Alan Cox; +Cc: snakebyte, linux-kernel, jgarzik, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Possible leaks in network drivers
2006-06-21 17:50 ` Possible leaks in network drivers Eric Sesterhenn
@ 2006-06-22 1:41 ` Herbert Xu
0 siblings, 0 replies; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 1:41 UTC (permalink / raw)
To: Eric Sesterhenn; +Cc: alan, linux-kernel, jgarzik, kmliu
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 0:55 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Herbert Xu
@ 2006-06-22 2:30 ` Herbert Xu
2006-06-22 8:22 ` Jeff Garzik
2006-06-22 8:26 ` Memory corruption in 8390.c ? David Miller
0 siblings, 2 replies; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 2:30 UTC (permalink / raw)
To: Alan Cox; +Cc: snakebyte, linux-kernel, jgarzik, netdev, davem
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.
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 2:30 ` Herbert Xu
@ 2006-06-22 8:22 ` Jeff Garzik
2006-06-22 8:29 ` Herbert Xu
2006-06-22 8:26 ` Memory corruption in 8390.c ? David Miller
1 sibling, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-06-22 8:22 UTC (permalink / raw)
To: Herbert Xu; +Cc: Alan Cox, snakebyte, linux-kernel, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 2:30 ` Herbert Xu
2006-06-22 8:22 ` Jeff Garzik
@ 2006-06-22 8:26 ` David Miller
2006-06-22 8:30 ` Herbert Xu
1 sibling, 1 reply; 28+ messages in thread
From: David Miller @ 2006-06-22 8:26 UTC (permalink / raw)
To: herbert; +Cc: alan, snakebyte, linux-kernel, jgarzik, netdev
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?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 8:22 ` Jeff Garzik
@ 2006-06-22 8:29 ` Herbert Xu
2006-06-22 8:57 ` Jeff Garzik
0 siblings, 1 reply; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 8:29 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Alan Cox, snakebyte, linux-kernel, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 8:26 ` Memory corruption in 8390.c ? David Miller
@ 2006-06-22 8:30 ` Herbert Xu
2006-06-22 8:34 ` David Miller
0 siblings, 1 reply; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 8:30 UTC (permalink / raw)
To: David Miller; +Cc: alan, snakebyte, linux-kernel, jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 8:30 ` Herbert Xu
@ 2006-06-22 8:34 ` David Miller
2006-06-22 11:34 ` Alan Cox
0 siblings, 1 reply; 28+ messages in thread
From: David Miller @ 2006-06-22 8:34 UTC (permalink / raw)
To: herbert; +Cc: alan, snakebyte, linux-kernel, jgarzik, netdev
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 :)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 8:29 ` Herbert Xu
@ 2006-06-22 8:57 ` Jeff Garzik
2006-06-22 9:02 ` Herbert Xu
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-06-22 8:57 UTC (permalink / raw)
To: Herbert Xu; +Cc: Alan Cox, snakebyte, linux-kernel, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 8:57 ` Jeff Garzik
@ 2006-06-22 9:02 ` Herbert Xu
2006-06-22 9:12 ` Herbert Xu
0 siblings, 1 reply; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 9:02 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Alan Cox, snakebyte, linux-kernel, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)
2006-06-22 9:02 ` Herbert Xu
@ 2006-06-22 9:12 ` Herbert Xu
0 siblings, 0 replies; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 9:12 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Alan Cox, snakebyte, linux-kernel, netdev, davem
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 11:34 ` Alan Cox
@ 2006-06-22 11:29 ` Herbert Xu
2006-06-22 13:25 ` Alan Cox
2006-06-22 11:33 ` Arjan van de Ven
1 sibling, 1 reply; 28+ messages in thread
From: Herbert Xu @ 2006-06-22 11:29 UTC (permalink / raw)
To: Alan Cox; +Cc: davem, herbert, snakebyte, linux-kernel, jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 11:34 ` Alan Cox
2006-06-22 11:29 ` Herbert Xu
@ 2006-06-22 11:33 ` Arjan van de Ven
2006-06-22 12:00 ` Erik Mouw
2006-06-22 13:10 ` Alan Cox
1 sibling, 2 replies; 28+ messages in thread
From: Arjan van de Ven @ 2006-06-22 11:33 UTC (permalink / raw)
To: Alan Cox; +Cc: David Miller, herbert, snakebyte, linux-kernel, jgarzik, netdev
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! :-)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 8:34 ` David Miller
@ 2006-06-22 11:34 ` Alan Cox
2006-06-22 11:29 ` Herbert Xu
2006-06-22 11:33 ` Arjan van de Ven
0 siblings, 2 replies; 28+ messages in thread
From: Alan Cox @ 2006-06-22 11:34 UTC (permalink / raw)
To: David Miller; +Cc: herbert, snakebyte, linux-kernel, jgarzik, netdev
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 11:33 ` Arjan van de Ven
@ 2006-06-22 12:00 ` Erik Mouw
2006-06-22 13:10 ` Alan Cox
1 sibling, 0 replies; 28+ messages in thread
From: Erik Mouw @ 2006-06-22 12:00 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Alan Cox, David Miller, herbert, snakebyte, linux-kernel,
jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 11:33 ` Arjan van de Ven
2006-06-22 12:00 ` Erik Mouw
@ 2006-06-22 13:10 ` Alan Cox
1 sibling, 0 replies; 28+ messages in thread
From: Alan Cox @ 2006-06-22 13:10 UTC (permalink / raw)
To: Arjan van de Ven
Cc: David Miller, herbert, snakebyte, linux-kernel, jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 11:29 ` Herbert Xu
@ 2006-06-22 13:25 ` Alan Cox
2006-06-23 3:32 ` Jeff Garzik
0 siblings, 1 reply; 28+ messages in thread
From: Alan Cox @ 2006-06-22 13:25 UTC (permalink / raw)
To: Herbert Xu; +Cc: davem, snakebyte, linux-kernel, jgarzik, netdev
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Memory corruption in 8390.c ?
2006-06-22 13:25 ` Alan Cox
@ 2006-06-23 3:32 ` Jeff Garzik
0 siblings, 0 replies; 28+ messages in thread
From: Jeff Garzik @ 2006-06-23 3:32 UTC (permalink / raw)
To: Alan Cox; +Cc: Herbert Xu, davem, snakebyte, linux-kernel, netdev
applied except for orinoco, which failed to apply (rediff?)
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2006-06-23 3:32 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-06-21 16:28 Possible leaks in network drivers Eric Sesterhenn
2006-06-21 17:05 ` Randy.Dunlap
2006-06-21 17:13 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Alan Cox
2006-06-21 17:23 ` Memory corruption in 8390.c ? Ben Pfaff
2006-06-21 17:54 ` Alan Cox
2006-06-21 18:03 ` Ben Pfaff
2006-06-21 20:50 ` Alan Cox
2006-06-21 17:59 ` PATCH: Re: Memory corruption in 8390.c ? (and hp100 xirc2ps smc9194 ....) Alan Cox
2006-06-21 19:00 ` Olivier Galibert
2006-06-21 17:50 ` Possible leaks in network drivers Eric Sesterhenn
2006-06-22 1:41 ` Herbert Xu
2006-06-22 0:55 ` Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers) Herbert Xu
2006-06-22 2:30 ` Herbert Xu
2006-06-22 8:22 ` Jeff Garzik
2006-06-22 8:29 ` Herbert Xu
2006-06-22 8:57 ` Jeff Garzik
2006-06-22 9:02 ` Herbert Xu
2006-06-22 9:12 ` Herbert Xu
2006-06-22 8:26 ` Memory corruption in 8390.c ? David Miller
2006-06-22 8:30 ` Herbert Xu
2006-06-22 8:34 ` David Miller
2006-06-22 11:34 ` Alan Cox
2006-06-22 11:29 ` Herbert Xu
2006-06-22 13:25 ` Alan Cox
2006-06-23 3:32 ` Jeff Garzik
2006-06-22 11:33 ` Arjan van de Ven
2006-06-22 12:00 ` Erik Mouw
2006-06-22 13:10 ` Alan Cox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).