VIA velocity skb leak.

VIA velocity skb leak.

[Dave Jones]

I think I've found a bug in the via-velocity driver, but I'm not
sure how to fix it.
We've been carrying some debug patches in the Fedora kernel that
dwmw2 wrote, which do some sanity checks on dma allocations.
Velocity triggered this trace..

[  324.777540] ------------[ cut here ]------------
[  324.777559] WARNING: at lib/dma-debug.c:470 check_unmap+0x196/0x3e4() (Not tainted)
[  324.777575] Hardware name:  
[  324.777591] via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x000000001a4c8ca2] [map size=60 bytes] [unmap size=54 bytes]
[  324.777614] Modules linked in: ipt_MASQUERADE iptable_nat nf_nat ipt_LOG xt_limit sunrpc nf_conntrack_netbios_ns ip6t_REJECT ip6table_filter ip6_tables ipv6 cpufreq_ondemand serio_raw usb_storage pcspkr i2c_viapro 3c59x mii i2c_core firewire_ohci via_velocity firewire_core crc_ccitt crc_itu_t ata_generic pata_acpi pata_via ext2 [last unloaded: scsi_wait_scan]
[  324.777792] Pid: 0, comm: swapper Not tainted 2.6.29-0.179.rc6.git5.fc11.i686.PAE #1
[  324.777807] Call Trace:
[  324.777839]  [<c0437a9f>] warn_slowpath+0x7c/0xa7
[  324.777870]  [<c0701800>] ? _spin_unlock_irq+0x1c/0x34
[  324.777899]  [<c042ae55>] ? account_group_exec_runtime+0x4d/0x54
[  324.777929]  [<c045783c>] ? mark_lock+0x1e/0x30b
[  324.777952]  [<c045783c>] ? mark_lock+0x1e/0x30b
[  324.777973]  [<c0554765>] ? get_hash_bucket+0x26/0x2f
[  324.777995]  [<c0554fc3>] check_unmap+0x196/0x3e4
[  324.778021]  [<c040de4d>] ? sched_clock+0x9/0xd
[  324.778042]  [<c05553bf>] debug_dma_unmap_page+0x68/0x70
[  324.778064]  [<c05553bf>] ? debug_dma_unmap_page+0x68/0x70
[  324.778111]  [<dcfc251c>] pci_unmap_single+0x65/0x70 [via_velocity]
[  324.778148]  [<dcfc25f5>] velocity_tx_srv+0xce/0x183 [via_velocity]
[  324.778188]  [<dcfc39a7>] velocity_intr+0x52f/0x594 [via_velocity]
[  324.778211]  [<c0457023>] ? trace_hardirqs_off+0xb/0xd
[  324.778232]  [<c067d15d>] ? netif_rx+0x9d/0x11a
[  324.778277]  [<dc861f8e>] ? boomerang_interrupt+0x324/0x331 [3c59x]
[  324.778300]  [<c045783c>] ? mark_lock+0x1e/0x30b
[  324.778330]  [<c047a6a4>] ? handle_fasteoi_irq+0x76/0xb7
[  324.778351]  [<c04566d8>] ? lock_release_holdtime+0x2b/0x123
[  324.778374]  [<c0479284>] handle_IRQ_event+0x22/0x58
[  324.778394]  [<c0479284>] ? handle_IRQ_event+0x22/0x58
[  324.778417]  [<c047a6ae>] handle_fasteoi_irq+0x80/0xb7
[  324.778437]  [<c047a62e>] ? handle_fasteoi_irq+0x0/0xb7
[  324.778450]  <IRQ>  [<c0409cac>] ? common_interrupt+0x2c/0x34
[  324.778490]  [<c0457d78>] ? trace_hardirqs_on+0xb/0xd
[  324.778501]  [<c059fcca>] ? acpi_idle_enter_simple+0x13a/0x179
[  324.778501]  [<c059f9b4>] ? acpi_idle_enter_bm+0xd1/0x2ad
[  324.778501]  [<c0660105>] ? menu_select+0x3d/0xa4
[  324.778501]  [<c065f670>] ? cpuidle_idle_call+0x65/0x9d
[  324.778501]  [<c065f670>] ? cpuidle_idle_call+0x65/0x9d
[  324.778501]  [<c040871e>] ? cpu_idle+0x72/0x90
[  324.778501]  [<c06efa03>] ? rest_init+0x53/0x55
[  324.778501] ---[ end trace 57ea7e11ea46a364 ]---


It looks like in velocity_xmit we do the skb_padto because the length is below
the minimum frame size (56 bytes) and pci_map the resulting 60 byte skb.
But later, when we free it, we forget that we did that size adjustment.
Does skb_padto adjust the skb->len somewhere, or is this responsibility of the caller?

	Dave

-- 
http://www.codemonkey.org.uk

Re: VIA velocity skb leak.

[David Miller]

velocity_xmit() needs to set 'pktlen = skb->len;' after,
not before, the skb_padto() call.

Re: VIA velocity skb leak.

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 11 Mar 2009 21:17:06 -0700 (PDT)

> 
> velocity_xmit() needs to set 'pktlen = skb->len;' after,
> not before, the skb_padto() call.

Actually that won't work since, as you suggest, skb->len
isn't updated by skb_padto().

So the transmit needs something like:

	pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);

velocity_free_tx_buf() needs to make the same calculation
instead of just plain skb->len

This bug probably exists in every other driver using
skb_padto() :-)

Re: VIA velocity skb leak.

[Dave Jones]

On Wed, Mar 11, 2009 at 09:20:09PM -0700, David Miller wrote:
 > From: David Miller <davem@davemloft.net>
 > Date: Wed, 11 Mar 2009 21:17:06 -0700 (PDT)
 > 
 > > 
 > > velocity_xmit() needs to set 'pktlen = skb->len;' after,
 > > not before, the skb_padto() call.
 > 
 > Actually that won't work since, as you suggest, skb->len
 > isn't updated by skb_padto().
 > 
 > So the transmit needs something like:
 > 
 > 	pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 > 
 > velocity_free_tx_buf() needs to make the same calculation
 > instead of just plain skb->len

Something like this ?
(It looks like the ZERO_COPY_SUPPORT is never enabled anywhere,
 so I didn't dig into how that works).

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index c5691fd..cd34dda 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1838,6 +1838,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 {
 	struct sk_buff *skb = tdinfo->skb;
 	int i;
+	int pktlen;
 
 	/*
 	 *	Don't unmap the pre-allocated tx_bufs
@@ -1845,10 +1846,11 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 	if (tdinfo->skb_dma) {
 
+		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 		for (i = 0; i < tdinfo->nskb_dma; i++) {
 #ifdef VELOCITY_ZERO_COPY_SUPPORT
 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], le16_to_cpu(td->tdesc1.len), PCI_DMA_TODEVICE);
 #else
-			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], skb->len, PCI_DMA_TODEVICE);
+			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], pktlen, PCI_DMA_TODEVICE);
 #endif
 			tdinfo->skb_dma[i] = 0;
 		}
@@ -2080,17 +2083,14 @@ static int velocity_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct tx_desc *td_ptr;
 	struct velocity_td_info *tdinfo;
 	unsigned long flags;
-	int pktlen = skb->len;
+	int pktlen;
 	__le16 len;
 	int index;
 
 
-
-	if (skb->len < ETH_ZLEN) {
-		if (skb_padto(skb, ETH_ZLEN))
-			goto out;
-		pktlen = ETH_ZLEN;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		goto out;
+	pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 
 	len = cpu_to_le16(pktlen);
 
 

 > This bug probably exists in every other driver using
 > skb_padto() :-)

Once I've tested this (tomorrow), I'll do a sweep through some of the others.
I expect the dma-debug stuff will pick them up eventually when that hits mainline,
so it'd be good to fix up the lower hanging fruit.

The dma-debug patches are kinda neat, this is just one type of bug class
it picks up. I forwarded another type to the e1000 list earlier today.

	Dave

-- 
http://www.codemonkey.org.uk

Re: VIA velocity skb leak.

[Eric Dumazet]

Dave Jones a écrit :
> On Wed, Mar 11, 2009 at 09:20:09PM -0700, David Miller wrote:
>  > From: David Miller <davem@davemloft.net>
>  > Date: Wed, 11 Mar 2009 21:17:06 -0700 (PDT)
>  > 
>  > > 
>  > > velocity_xmit() needs to set 'pktlen = skb->len;' after,
>  > > not before, the skb_padto() call.
>  > 
>  > Actually that won't work since, as you suggest, skb->len
>  > isn't updated by skb_padto().
>  > 
>  > So the transmit needs something like:
>  > 
>  > 	pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
>  > 
>  > velocity_free_tx_buf() needs to make the same calculation
>  > instead of just plain skb->len
> 
> Something like this ?
> (It looks like the ZERO_COPY_SUPPORT is never enabled anywhere,
>  so I didn't dig into how that works).
> 
> diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
> index c5691fd..cd34dda 100644
> --- a/drivers/net/via-velocity.c
> +++ b/drivers/net/via-velocity.c
> @@ -1838,6 +1838,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
>  {
>  	struct sk_buff *skb = tdinfo->skb;
>  	int i;
> +	int pktlen;
>  
>  	/*
>  	 *	Don't unmap the pre-allocated tx_bufs
> @@ -1845,10 +1846,11 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
>  	if (tdinfo->skb_dma) {
>  
> +		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);

I personally find better to use max(skb->len, ETH_ZLEN) macro, but YMMV ;)

It actually can avoid you a bug ;)

Re: VIA velocity skb leak.

[Dave Jones]

On Thu, Mar 12, 2009 at 05:45:57AM +0100, Eric Dumazet wrote:

 > > @@ -1845,10 +1846,11 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 > >  	if (tdinfo->skb_dma) {
 > >  
 > > +		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 > 
 > I personally find better to use max(skb->len, ETH_ZLEN) macro, but YMMV ;)
 > 
 > It actually can avoid you a bug ;)
 
I prefer that too, but it makes a warning.

drivers/net/via-velocity.c:2093: warning: comparison of distinct pointer types lacks a cast

We can fix this by either casting ETH_ZLEN to an unsigned int,
or we could just do the diff below..

Or did I overlook something?

(if this looks ok, perhaps the other defines could use the same treatment?)

	Dave

The minimum frame length is never signed, define it as
such so we don't need excessive casts in comparisons.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 7f3c735..c41183e 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -30,7 +30,7 @@
 
 #define ETH_ALEN	6		/* Octets in one ethernet addr	 */
 #define ETH_HLEN	14		/* Total octets in header.	 */
-#define ETH_ZLEN	60		/* Min. octets in frame sans FCS */
+#define ETH_ZLEN	60U		/* Min. octets in frame sans FCS */
 #define ETH_DATA_LEN	1500		/* Max. octets in payload	 */
 #define ETH_FRAME_LEN	1514		/* Max. octets in frame sans FCS */
 #define ETH_FCS_LEN	4		/* Octets in the FCS		 */

-- 
http://www.codemonkey.org.uk

Re: VIA velocity skb leak.

[Eric Dumazet]

Dave Jones a écrit :
> On Thu, Mar 12, 2009 at 05:45:57AM +0100, Eric Dumazet wrote:
> 
>  > > @@ -1845,10 +1846,11 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
>  > >  	if (tdinfo->skb_dma) {
>  > >  
>  > > +		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
>  > 
>  > I personally find better to use max(skb->len, ETH_ZLEN) macro, but YMMV ;)
>  > 
>  > It actually can avoid you a bug ;)
>  
> I prefer that too, but it makes a warning.
> 
> drivers/net/via-velocity.c:2093: warning: comparison of distinct pointer types lacks a cast
> 
> We can fix this by either casting ETH_ZLEN to an unsigned int,
> or we could just do the diff below..
> 
> Or did I overlook something?
> 
> (if this looks ok, perhaps the other defines could use the same treatment?)
> 
> 	Dave
> 
> The minimum frame length is never signed, define it as
> such so we don't need excessive casts in comparisons.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
> index 7f3c735..c41183e 100644
> --- a/include/linux/if_ether.h
> +++ b/include/linux/if_ether.h
> @@ -30,7 +30,7 @@
>  
>  #define ETH_ALEN	6		/* Octets in one ethernet addr	 */
>  #define ETH_HLEN	14		/* Total octets in header.	 */
> -#define ETH_ZLEN	60		/* Min. octets in frame sans FCS */
> +#define ETH_ZLEN	60U		/* Min. octets in frame sans FCS */
>  #define ETH_DATA_LEN	1500		/* Max. octets in payload	 */
>  #define ETH_FRAME_LEN	1514		/* Max. octets in frame sans FCS */
>  #define ETH_FCS_LEN	4		/* Octets in the FCS		 */
> 

or use max_t(unsigned, skb->len, ETH_ZLEN)

Re: VIA velocity skb leak.

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Thu, 12 Mar 2009 06:14:27 +0100

> or use max_t(unsigned, skb->len, ETH_ZLEN)

Right, I'll add Dave's patch, but using max_t() here.

Thanks everyone.

a2065 skb_padto cleanups

[Dave Jones]

Whilst looking for other instances of the bug that was in velocity,
I noticed a few possible cleanups in a2065.  I don't have this
hardware, so extra review appreciated.

	Dave
--- 

Remove unnecessary check (skb_padto does the same check)
Remove unnecessary duplicate variable
Remove unnecessary clearing of padded part of skb.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/a2065.c b/drivers/net/a2065.c
index 7a60bdd..d0d0c2f 100644
--- a/drivers/net/a2065.c
+++ b/drivers/net/a2065.c
@@ -552,18 +552,13 @@ static int lance_start_xmit (struct sk_buff *skb, struct net_device *dev)
 	struct lance_private *lp = netdev_priv(dev);
 	volatile struct lance_regs *ll = lp->ll;
 	volatile struct lance_init_block *ib = lp->init_block;
-	int entry, skblen, len;
+	int entry, skblen;
 	int status = 0;
 	unsigned long flags;
 
-	skblen = skb->len;
-	len = skblen;
-
-	if (len < ETH_ZLEN) {
-		len = ETH_ZLEN;
-		if (skb_padto(skb, ETH_ZLEN))
-			return 0;
-	}
+	if (skb_padto(skb, ETH_ZLEN))
+		return 0;
+	skblen = max_t(unsigned, skb->len, ETH_ZLEN);
 
 	local_irq_save(flags);
 
@@ -586,15 +581,11 @@ static int lance_start_xmit (struct sk_buff *skb, struct net_device *dev)
 	}
 #endif
 	entry = lp->tx_new & lp->tx_ring_mod_mask;
-	ib->btx_ring [entry].length = (-len) | 0xf000;
+	ib->btx_ring [entry].length = (-skblen) | 0xf000;
 	ib->btx_ring [entry].misc = 0;
 
 	skb_copy_from_linear_data(skb, (void *)&ib->tx_buf [entry][0], skblen);
 
-	/* Clear the slack of the packet, do I need this? */
-	if (len != skblen)
-		memset ((void *) &ib->tx_buf [entry][skblen], 0, len - skblen);
-
 	/* Now, give the packet to the lance */
 	ib->btx_ring [entry].tmd1_bits = (LE_T1_POK|LE_T1_OWN);
 	lp->tx_new = (lp->tx_new+1) & lp->tx_ring_mod_mask;

-- 
http://www.codemonkey.org.uk

r8169 skb leak

[Dave Jones]

r8169 seems to have the same problem that the via-velocity did with skb padding.
Found by code-inspection only, no hardware to test.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index b347340..07b1de1 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3145,7 +3145,9 @@ err_out:
 static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
 				 struct TxDesc *desc)
 {
-	unsigned int len = tx_skb->len;
+	unsigned int len;
+
+	len = max_t(unsigned, tx_skb->len, ETH_ZLEN);
 
 	pci_unmap_single(pdev, le64_to_cpu(desc->addr), len, PCI_DMA_TODEVICE);
 	desc->opts1 = 0x00;
@@ -3364,11 +3366,9 @@ static int rtl8169_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	} else {
 		len = skb->len;
 
-		if (unlikely(len < ETH_ZLEN)) {
-			if (skb_padto(skb, ETH_ZLEN))
-				goto err_update_stats;
-			len = ETH_ZLEN;
-		}
+		if (skb_padto(skb, ETH_ZLEN))
+			goto err_update_stats;
+		len = max_t(unsigned, skb->len, ETH_ZLEN);
 
 		opts1 |= FirstFrag | LastFrag;
 		tp->tx_skb[entry].skb = skb;
-- 
http://www.codemonkey.org.uk

Re: r8169 skb leak

[Francois Romieu]

Dave Jones <davej@redhat.com> :
> r8169 seems to have the same problem that the via-velocity did with skb padding.
> Found by code-inspection only, no hardware to test.

Davem, can you defer this one until now + 24h ?

The hardware is supposed to auto-pad: I'd rather check it does with
a few adapters and remove this code altogether.

-- 
Ueimor

Re: r8169 skb leak

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Fri, 13 Mar 2009 23:26:18 +0100

> Dave Jones <davej@redhat.com> :
> > r8169 seems to have the same problem that the via-velocity did with skb padding.
> > Found by code-inspection only, no hardware to test.
> 
> Davem, can you defer this one until now + 24h ?
> 
> The hardware is supposed to auto-pad: I'd rather check it does with
> a few adapters and remove this code altogether.

Sure.

Re: a2065 skb_padto cleanups

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Fri, 13 Mar 2009 16:52:16 -0400

> Remove unnecessary check (skb_padto does the same check)
> Remove unnecessary duplicate variable
> Remove unnecessary clearing of padded part of skb.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Looks good enough to me, applied to net-next-2.6, thanks!