BUG in pcnet32.c?

BUG in pcnet32.c?

[Brian Murphy]

In pcnet32.c where the driver writer sets up her receive buffers there 
is this line

lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, 
rx_skbuff->len, PCI_DMA_FROMDEVICE);

the length value turns out to be 0 and crashes the running process,ifconfig.
Is making a map for a buffer of length 0 valid at all? If not what the 
hell is going on here.

I feel this should say PKT_BUF_SZ instead of rx_skbuff->len which is the 
length of skbuff which has been
allocated at this point in the code, this is line 986 in todays checkout.

Something is wrong in any case, any pointers?

/Brian

Re: BUG in pcnet32.c?

[Steven J. Hill]

Brian Murphy wrote:
> In pcnet32.c where the driver writer sets up her receive buffers there 
> is this line
> 
> lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, 
> rx_skbuff->len, PCI_DMA_FROMDEVICE);
> 
> the length value turns out to be 0 and crashes the running 
> process,ifconfig.
> Is making a map for a buffer of length 0 valid at all? If not what the 
> hell is going on here.
> 
> I feel this should say PKT_BUF_SZ instead of rx_skbuff->len which is the 
> length of skbuff which has been
> allocated at this point in the code, this is line 986 in todays checkout.
> 
> Something is wrong in any case, any pointers?
> 
Excellent. So my new BUG code detected another bad network driver. Your network
driver is broken and it needs fixed. I will refer you to these posts between
Jeff Garzik and myself when I found a similar issue on the 'natsemi.c' driver.
Mapping a PCI address with length zero is a BUG, period. You length should
be the maximum RX buffer length + 2. You will see from the patches in the
messages below that this is for IP header alignment. Good luck and please let
use know how it turns out.

-Steve

http://lkml.org/lkml/2004/3/16/218
http://lkml.org/lkml/2004/3/16/244

Re: BUG in pcnet32.c?

[Kevin D. Kissell]

> Excellent. So my new BUG code detected another bad network driver. Your network
> driver is broken and it needs fixed. I will refer you to these posts between
> Jeff Garzik and myself when I found a similar issue on the 'natsemi.c' driver.
> Mapping a PCI address with length zero is a BUG, period. You length should
> be the maximum RX buffer length + 2. You will see from the patches in the
> messages below that this is for IP header alignment. Good luck and please let
> use know how it turns out.

Which reminds me of something I've been meaning to mention for a while.
Back in the dark days of Linux 2.2 on MIPS, I discovered that a number
of network drivers were subtly broken for MIPS because they allocated
enough extra space for IP header alignment, but not for cache line alignment.
Particularly on CPUs with write-back caches, it can be a Bad Thing if a cache 
line straddles two packet buffers, as the flush of one can cause the other to be
clobbered.  I had to redefine the alignment constant for MIPS to be a function
of the line size to have 100% solid operation of the Tulip and pcnet32 drivers.

The whole network driver cache management paradigm was redone for 2.4,
and I've often wondered whether the same potential problem exists, but never
had the time to go in and check.

There, I've mentioned it.  My conscience is clear.  ;o)

Re: BUG in pcnet32.c?

[Kevin D. Kissell]

> Excellent. So my new BUG code detected another bad network driver. Your network
> driver is broken and it needs fixed. I will refer you to these posts between
> Jeff Garzik and myself when I found a similar issue on the 'natsemi.c' driver.
> Mapping a PCI address with length zero is a BUG, period. You length should
> be the maximum RX buffer length + 2. You will see from the patches in the
> messages below that this is for IP header alignment. Good luck and please let
> use know how it turns out.

Which reminds me of something I've been meaning to mention for a while.
Back in the dark days of Linux 2.2 on MIPS, I discovered that a number
of network drivers were subtly broken for MIPS because they allocated
enough extra space for IP header alignment, but not for cache line alignment.
Particularly on CPUs with write-back caches, it can be a Bad Thing if a cache 
line straddles two packet buffers, as the flush of one can cause the other to be
clobbered.  I had to redefine the alignment constant for MIPS to be a function
of the line size to have 100% solid operation of the Tulip and pcnet32 drivers.

The whole network driver cache management paradigm was redone for 2.4,
and I've often wondered whether the same potential problem exists, but never
had the time to go in and check.

There, I've mentioned it.  My conscience is clear.  ;o)

Re: BUG in pcnet32.c?

[Steven J. Hill]

Kevin D. Kissell wrote:
> 
> The whole network driver cache management paradigm was redone for 2.4,
> and I've often wondered whether the same potential problem exists, but never
> had the time to go in and check.
> 
> There, I've mentioned it.  My conscience is clear.  ;o)
>
*blush* Okaay, I....I need to clear my conscience too. I knew about this
when posting, but forgot to mention it. Brian, you may find that the + 2
for IP alignment does not work precisely for the reason Kevin mentioned.
You may need to cache align your RX buffer. Again, let us know what you
discover. Okay, now MY conscience is clear.

-Steve

Re: BUG in pcnet32.c?

[Ralf Baechle]

On Mon, Mar 29, 2004 at 10:55:52PM +0200, Kevin D. Kissell wrote:

> Which reminds me of something I've been meaning to mention for a while.
> Back in the dark days of Linux 2.2 on MIPS, I discovered that a number
> of network drivers were subtly broken for MIPS because they allocated
> enough extra space for IP header alignment, but not for cache line alignment.
> Particularly on CPUs with write-back caches, it can be a Bad Thing if a cache 
> line straddles two packet buffers, as the flush of one can cause the other
> to be clobbered.  I had to redefine the alignment constant for MIPS to be
> a function of the line size to have 100% solid operation of the Tulip and
> pcnet32 drivers.
> 
> The whole network driver cache management paradigm was redone for 2.4,
> and I've often wondered whether the same potential problem exists, but never
> had the time to go in and check.

The change goes beyond just cache managment; the API also abstracts away
I/O MMUs which so far are quite rare on MIPS systems - but I really hope
they're going to establish themselves asap.

The Documentation/DMA-API.txt also documents how properly deal with cache
alignment when using this API.

Steven, maybe that we should add another assertion to make sure we don't
run into trouble with missaligned cachelines?

> There, I've mentioned it.  My conscience is clear.  ;o)

Ommmmm ;))

  Ralf

Re: BUG in pcnet32.c?

[Steven J. Hill]

Ralf Baechle wrote:
> 
> The change goes beyond just cache managment; the API also abstracts away
> I/O MMUs which so far are quite rare on MIPS systems - but I really hope
> they're going to establish themselves asap.
> 
> The Documentation/DMA-API.txt also documents how properly deal with cache
> alignment when using this API.
> 
> Steven, maybe that we should add another assertion to make sure we don't
> run into trouble with missaligned cachelines?
>
Okay. I will have a look at tomorrow morning.

-Steve

Re: BUG in pcnet32.c?

[Brian Murphy]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

Steven J. Hill wrote:

> Brian Murphy wrote:
>
>> In pcnet32.c where the driver writer sets up her receive buffers 
>> there is this line
>>
>> lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, 
>> rx_skbuff->len, PCI_DMA_FROMDEVICE);
>>
>> the length value turns out to be 0 and crashes the running 
>> process,ifconfig.
>> Is making a map for a buffer of length 0 valid at all? If not what 
>> the hell is going on here.
>>
>> I feel this should say PKT_BUF_SZ instead of rx_skbuff->len which is 
>> the length of skbuff which has been
>> allocated at this point in the code, this is line 986 in todays 
>> checkout.
>>
>> Something is wrong in any case, any pointers?
>>
> Excellent. So my new BUG code detected another bad network driver. 
> Your network

Not sure what you mean. I get the panic "Break instruction in kernel 
code" from do_bp
in traps.c. This seems like a strange "assertion" to me...

Anyway the attached patch fixes my problems, is anyone interested in 
reviewing it?

/Brian

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2180 bytes --]

Index: pcnet32.c
===================================================================
RCS file: /cvs/linux/drivers/net/pcnet32.c,v
retrieving revision 1.33.2.7
diff -u -r1.33.2.7 pcnet32.c
--- pcnet32.c	17 Nov 2003 01:07:38 -0000	1.33.2.7
+++ pcnet32.c	31 Mar 2004 20:31:01 -0000
@@ -983,7 +983,7 @@
 	}
 
 	if (lp->rx_dma_addr[i] == 0) 
-		lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, rx_skbuff->len, PCI_DMA_FROMDEVICE);
+		lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 	lp->rx_ring[i].base = (u32)le32_to_cpu(lp->rx_dma_addr[i]);
 	lp->rx_ring[i].buf_length = le16_to_cpu(-PKT_BUF_SZ);
 	lp->rx_ring[i].status = le16_to_cpu(0x8000);
@@ -1319,13 +1319,13 @@
 		    if ((newskb = dev_alloc_skb (PKT_BUF_SZ))) {
 			skb_reserve (newskb, 2);
 			skb = lp->rx_skbuff[entry];
-			pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[entry], skb->len, PCI_DMA_FROMDEVICE);
+			pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[entry], PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 			skb_put (skb, pkt_len);
 			lp->rx_skbuff[entry] = newskb;
 			newskb->dev = dev;
                         lp->rx_dma_addr[entry] = 
 				pci_map_single(lp->pci_dev, newskb->tail,
-					newskb->len, PCI_DMA_FROMDEVICE);
+					PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 			lp->rx_ring[entry].base = le32_to_cpu(lp->rx_dma_addr[entry]);
 			rx_in_place = 1;
 		    } else
@@ -1354,7 +1354,7 @@
 		    skb_put(skb,pkt_len);	/* Make room */
                     pci_dma_sync_single(lp->pci_dev, 
 		                        lp->rx_dma_addr[entry],
-		                        pkt_len,
+		                        PKT_BUF_SZ,
 		                        PCI_DMA_FROMDEVICE);
 		    eth_copy_and_sum(skb,
 				     (unsigned char *)(lp->rx_skbuff[entry]->tail),
@@ -1409,7 +1409,7 @@
     for (i = 0; i < RX_RING_SIZE; i++) {
 	lp->rx_ring[i].status = 0;			    
 	if (lp->rx_skbuff[i]) {
-            pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[i], lp->rx_skbuff[i]->len, PCI_DMA_FROMDEVICE);
+            pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[i], PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 	    dev_kfree_skb(lp->rx_skbuff[i]);
         }
 	lp->rx_skbuff[i] = NULL;

Re: BUG in pcnet32.c?

[Geert Uytterhoeven]

On Wed, 31 Mar 2004, Brian Murphy wrote:
> Anyway the attached patch fixes my problems, is anyone interested in
> reviewing it?

I guess netdev@oss.sgi.com and tsbogend@alpha.franken.de are.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: BUG in pcnet32.c?

[Ralf Baechle]

On Wed, Mar 31, 2004 at 10:48:16PM +0200, Brian Murphy wrote:

> Not sure what you mean. I get the panic "Break instruction in kernel 
> code" from do_bp
> in traps.c. This seems like a strange "assertion" to me...

The more information BUG or BUG_ON provide the bigger the kernel gets.
Using a simple break instruction was simply the smallest thing.  The
previous, just slightly more verbose BUG() implementation did result
in ~ 87k of bloat ...

  Ralf

Re: BUG in pcnet32.c?

[Brian Murphy]

Ralf Baechle wrote:

>On Wed, Mar 31, 2004 at 10:48:16PM +0200, Brian Murphy wrote:
>
>  
>
>>Not sure what you mean. I get the panic "Break instruction in kernel 
>>code" from do_bp
>>in traps.c. This seems like a strange "assertion" to me...
>>    
>>
>
>The more information BUG or BUG_ON provide the bigger the kernel gets.
>Using a simple break instruction was simply the smallest thing.  The
>previous, just slightly more verbose BUG() implementation did result
>in ~ 87k of bloat ...
>  
>
Perhaps you could mention this usage of break explicitly in the message 
in do_bp.

/Brian

Re: BUG in pcnet32.c?

[Ralf Baechle]

On Fri, Apr 02, 2004 at 10:27:56AM +0200, Brian Murphy wrote:

> >The more information BUG or BUG_ON provide the bigger the kernel gets.
> >Using a simple break instruction was simply the smallest thing.  The
> >previous, just slightly more verbose BUG() implementation did result
> >in ~ 87k of bloat ...
> > 
> >
> Perhaps you could mention this usage of break explicitly in the message 
> in do_bp.

That's easy, BUG() and BUG_ON() if the condition was met use a break
instruction.

  Ralf