forcedeth: fix random memory scribbling bug

Two critical bugs were found in forcedeth 0.47:
- TSO doesn't work.
- pci_map_single() for the rx buffers is called with size==0. This bug is critical, it causes random memory corruptions on systems with an iommu.

Below is a minimal fix for both bugs, for inclusion into 2.6.15. TSO will be fixed properly in the next version.
Tested on x86-64.

@@ -10,7 +10,7 @@
  * trademarks of NVIDIA Corporation in the United States and other
  * countries.
  *
- * Copyright (C) 2003,4 Manfred Spraul
+ * Copyright (C) 2003,4,5 Manfred Spraul
  * Copyright (C) 2004 Andrew de Quincey (wol support)
  * Copyright (C) 2004 Carl-Daniel Hailfinger (invalid MAC handling, insane
  *		IRQ rate fixes, bigendian fixes, cleanups, verification)

@@ -100,6 +100,7 @@
  *	0.45: 18 Sep 2005: Remove nv_stop/start_rx from every link check
  *	0.46: 20 Oct 2005: Add irq optimization modes.
  *	0.47: 26 Oct 2005: Add phyaddr 0 in phy scan.
+ *	0.48: 24 Dec 2005: Disable TSO, bugfix for pci_map_single
  *
  * Known bugs:
  * We suspect that on some hardware no TX done interrupts are generated.

@@ -111,7 +112,7 @@
  * DEV_NEED_TIMERIRQ will not harm you on sane hardware, only generating a few
  * superfluous timer interrupts from the nic.
  */
-#define FORCEDETH_VERSION		"0.47"
+#define FORCEDETH_VERSION		"0.48"
 #define DRV_NAME			"forcedeth"
 
 #include <linux/module.h>

@@ -871,8 +872,8 @@
 		} else {
 			skb = np->rx_skbuff[nr];
 		}
-		np->rx_dma[nr] = pci_map_single(np->pci_dev, skb->data, skb->len,
-						PCI_DMA_FROMDEVICE);
+		np->rx_dma[nr] = pci_map_single(np->pci_dev, skb->data,
+					skb->end-skb->data, PCI_DMA_FROMDEVICE);
 		if (np->desc_ver == DESC_VER_1 || np->desc_ver == DESC_VER_2) {
 			np->rx_ring.orig[nr].PacketBuffer = cpu_to_le32(np->rx_dma[nr]);
 			wmb();

@@ -999,7 +1000,7 @@
 		wmb();
 		if (np->rx_skbuff[i]) {
 			pci_unmap_single(np->pci_dev, np->rx_dma[i],
-						np->rx_skbuff[i]->len,
+						np->rx_skbuff[i]->end-np->rx_skbuff[i]->data,
 						PCI_DMA_FROMDEVICE);
 			dev_kfree_skb(np->rx_skbuff[i]);
 			np->rx_skbuff[i] = NULL;

@@ -1334,7 +1335,7 @@
 		 * the performance.
 		 */
 		pci_unmap_single(np->pci_dev, np->rx_dma[i],
-				np->rx_skbuff[i]->len,
+				np->rx_skbuff[i]->end-np->rx_skbuff[i]->data,
 				PCI_DMA_FROMDEVICE);
 
 		{

@@ -2455,7 +2456,7 @@
 		np->txrxctl_bits |= NVREG_TXRXCTL_RXCHECK;
 		dev->features |= NETIF_F_HW_CSUM | NETIF_F_SG;
 #ifdef NETIF_F_TSO
-		dev->features |= NETIF_F_TSO;
+		/* disabled dev->features |= NETIF_F_TSO; */
 #endif
  	}
 

1) Why does forcedeth require a non-standard calculation for each pci_map_single() call?

2) I have requested multiple times that you avoid MIME...

3) Why disable TSO completely? It sounds like it should default to off, then permit enabling via ethtool.

Jeff

- skb->len is the wrong thing (tm), since it's 0 until skb_put().
- I have not found a field that contains the actual size of the data area of an skb.
- the results must be identical for map and unmap.
- I could recalculate the size of the allocation from np->rx_buf_sz, but I don't like that. Right now it would work, but it's too subtile that changing rx_buf_sz while there are outstanding rx buffers results in a iommu memory leak. Therefore I decided to calculate the mapping size with "skb->end - skb->data": The size of the mapping for an skb is calculated by looking at fields in the skb, no knowledge about driver fields.

It's the first time that you complain about Content-Transfer-Encoding: 7bit attachments.

The bugfix is in 0.49 - it's just a bit larger, I would consider it for 2.5.16.

--
Manfred

- skb->len is the wrong thing (tm), since it's 0 until skb_put().
- I have not found a field that contains the actual size of the data area of an skb.
- the results must be identical for map and unmap.
- I could recalculate the size of the allocation from np->rx_buf_sz, but I don't like that. Right now it would work, but it's too subtile that changing rx_buf_sz while there are outstanding rx buffers results in a iommu memory leak. Therefore I decided to calculate the mapping size with "skb->end - skb->data": The size of the mapping for an skb is calculated by looking at fields in the skb, no knowledge about driver fields.

It's the first time that you complain about Content-Transfer-Encoding: 7bit attachments.

The bugfix is in 0.49 - it's just a bit larger, I would consider it for 2.5.16.

--
Manfred

Good catch. Btw, should we perhaps disallow (or at least WARN_ON()) pci_map_single() with a length of zero? I think it's always likely a bug..

However, that

	"skb->end - skb->data"

calculation is a bit strange. It correctly maps the whole skb, but wouldn't it make more sense to use the length we actually tell the card to use?

In other words, wouldn't it be a whole lot more sensible and logical to use

	np->rx_buf_sz

instead? That's the value we use for allocation and that's the size we tell the card we have.

Of course, on the alloc path, it seems to add an additional "NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.

Linus

Good catch. Btw, should we perhaps disallow (or at least WARN_ON()) pci_map_single() with a length of zero? I think it's always likely a bug..

However, that

	"skb->end - skb->data"

calculation is a bit strange. It correctly maps the whole skb, but wouldn't it make more sense to use the length we actually tell the card to use?

In other words, wouldn't it be a whole lot more sensible and logical to use

	np->rx_buf_sz

instead? That's the value we use for allocation and that's the size we tell the card we have.

Of course, on the alloc path, it seems to add an additional "NV_RX_ALLOC_PAD" thing, so maybe the "end-data" thing makes sense.

Linus

The problem is the pci_unmap_single() call that happens during nv_close() or the rx interrupt handler. I think it makes more sense to rely on fields in the individual skb instead of reading from np->rx_buf_sz. If np->rx_buf_sz changes inbetween, then we have a memory leak.

--
Manfred

The problem is the pci_unmap_single() call that happens during nv_close() or the rx interrupt handler. I think it makes more sense to rely on fields in the individual skb instead of reading from np->rx_buf_sz. If np->rx_buf_sz changes inbetween, then we have a memory leak.

--
Manfred

These proper text encodings are easy to _apply_, because the raw email is uncorrupted.

However, attachments are still broken for a very fundamental reason:
basically no email client will ever quote them on replies. Which means that if somebody has commentary about some specific part of the patch, the attachement is _totally_ the wrong thing to do.

In other words, there's a reason I encourage people VERY STRONGLY to use in-line patches. If you have a broken mailer that corrupts whitespace, please just fix it.

Linus

These proper text encodings are easy to _apply_, because the raw email is uncorrupted.

However, attachments are still broken for a very fundamental reason:
basically no email client will ever quote them on replies. Which means that if somebody has commentary about some specific part of the patch, the attachement is _totally_ the wrong thing to do.

In other words, there's a reason I encourage people VERY STRONGLY to use in-line patches. If you have a broken mailer that corrupts whitespace, please just fix it.

Linus

nod

That's the sort of thing I prefer.

None of the other ethernet drivers do 'end - data', which is why I hesitated quite a bit on this change.

Jeff

nod

That's the sort of thing I prefer.

None of the other ethernet drivers do 'end - data', which is why I hesitated quite a bit on this change.

Jeff

Fair enough. Patch applied.

Linus

Fair enough. Patch applied.

Linus

Paranoia -- the situation above never occurs. It is coded as are other drivers: np->rx_buf_sz only changes in ->change_mtu(), which (a) is serialized against close and (b) always stops the engine and drains RX skbs before changing the size.

So can we please remove the subtraction code now added to the hot path?
If not now, for 2.6.16?

Jeff

Paranoia -- the situation above never occurs. It is coded as are other drivers: np->rx_buf_sz only changes in ->change_mtu(), which (a) is serialized against close and (b) always stops the engine and drains RX skbs before changing the size.

So can we please remove the subtraction code now added to the hot path?
If not now, for 2.6.16?

Jeff

Jeff Garzik <[email protected]> :
[...]

Yep.

Btw, regarding the "more sense" thing, now:
- pci_{map/unmap}_single() uses skb->foo
- nv_alloc_rx() and friends use np->rx_buf_sz

(thread moved to [email protected])

--
Ueimor