BUG in pcnet32.c?

DISCUSSION Brian Murphy 5 participants 11 messages Mar 29, 2004 – Apr 2, 2004 MIPS

lore

Share & tools

lore

Brian Murphy

author

2004-03-29 20:01 UTC #0 lore raw

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

Steven J. Hill

2004-03-29 20:26 UTC #1 lore raw

Quoted 4 lines · 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);

Quoted 8 lines

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

Kevin D. Kissell

2004-03-29 20:54 UTC #2 lore raw

Quoted 7 lines

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)

Steven J. Hill

2004-03-29 20:59 UTC #3 lore raw

Quoted 7 lines · Kevin D. Kissell

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

Ralf Baechle

2004-03-30 01:24 UTC #4 lore raw

In reply to Kevin D. Kissell

Quoted 10 lines · Kevin D. Kissell

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.

Quoted 3 lines

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?

Quoted 1 line

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

Ommmmm ;))

Ralf

Steven J. Hill

2004-03-30 02:30 UTC #5 lore raw

Quoted 11 lines · Ralf Baechle

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

Brian Murphy

author

2004-03-31 20:48 UTC #6 lore raw

In reply to Steven J. Hill

Quoted 9 lines · Steven J. Hill

Quoted 7 lines · 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.

Quoted 6 lines

Quoted 5 lines
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

Index: pcnet32.c
===================================================================
RCS file: /cvs/linux/drivers/net/pcnet32.c,v
retrieving revision 1.33.2.7

pcnet32.c +5 -5

diff -u -r1.33.2.7 pcnet32.c
@@ -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;

Geert Uytterhoeven

2004-04-01 08:31 UTC #7 lore raw

Quoted 2 lines · Brian Murphy

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

I guess [email protected] and [email protected] are.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Ralf Baechle

2004-04-01 17:31 UTC #8 lore raw

In reply to Brian Murphy

Quoted 3 lines · Brian Murphy

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

Brian Murphy

author

2004-04-02 08:28 UTC #9 lore raw

Quoted 9 lines · Ralf Baechle

Quoted 4 lines · Brian Murphy
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...

Quoted 6 lines

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

Ralf Baechle

2004-04-02 08:39 UTC #10 lore raw

Quoted 8 lines · Brian Murphy

Quoted 6 lines
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