via-velocity dma-debug warnings again. (2.6.35.2)

via-velocity dma-debug warnings again. (2.6.35.2)

[Dave Jones]

I installed the Fedora 14 alpha, which is based on 2.6.35.2, and hit
the following trace..



WARNING: at lib/dma-debug.c:811 check_unmap+0x212/0x59b()
Hardware name:  
via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x00000000194ba27e] [map size=66 bytes] [unmap size=182 bytes]
Modules linked in: ipt_MASQUERADE iptable_nat nf_nat ipt_LOG xt_limit bluetooth rfkill sunrpc cpufreq_ondemand acpi_cpufreq mperf ip6t_REJECT ip6table_filter nf_conntrack_netbios_ns ip6_tables i2c_viapro 3c59x i2c_core mii via_velocity crc_ccitt ipv6 autofs4 ext2 pata_acpi ata_generic firewire_ohci firewire_core pata_via crc_itu_t [last unloaded: scsi_wait_scan]
Pid: 1712, comm: unix_chkpwd Not tainted 2.6.35.2-9.fc14.i686.PAE #1
Call Trace:
 [<c0441ec5>] warn_slowpath_common+0x6a/0x7f
 [<c05dcc7e>] ? check_unmap+0x212/0x59b
 [<c0441f4d>] warn_slowpath_fmt+0x2b/0x2f
 [<c05dcc7e>] check_unmap+0x212/0x59b
 [<c05dd0b5>] debug_dma_unmap_page+0x5a/0x62
 [<dd22fab4>] pci_unmap_single+0x58/0x63 [via_velocity]
 [<dd23187b>] velocity_tx_srv+0x127/0x1c1 [via_velocity]
 [<dd231ab9>] velocity_poll+0x47/0x89 [via_velocity]
 [<c074b8ff>] net_rx_action+0x9f/0x1b6
 [<c04476e4>] __do_softirq+0xc2/0x179
 [<c04477cf>] do_softirq+0x34/0x56
 [<c0447a4c>] irq_exit+0x3d/0x70
 [<c040a3a3>] do_IRQ+0x7d/0x91
 [<c0408fb5>] common_interrupt+0x35/0x3c
 [<c07e11ed>] ? _raw_spin_unlock_irq+0x27/0x2f
 [<c046ef5e>] ? raw_local_irq_enable+0xa/0x10
 [<c07e11f2>] _raw_spin_unlock_irq+0x2c/0x2f
 [<c043668c>] finish_task_switch+0x59/0xa9
 [<c0436633>] ? finish_task_switch+0x0/0xa9
 [<c07df33f>] schedule+0x51d/0x581
 [<c043e2b5>] __cond_resched+0x1b/0x2b
 [<c07df44d>] _cond_resched+0x1a/0x21
 [<c04d3ac2>] remove_vma+0x25/0x62
 [<c04d4b24>] do_munmap+0x1ed/0x205
 [<c04d5487>] mmap_region+0x70/0x381
 [<c04d59e0>] do_mmap_pgoff+0x248/0x28e
 [<c04d5aea>] sys_mmap_pgoff+0xc4/0xee
 [<c07e141c>] syscall_call+0x7/0xb
---[ end trace a7c3d35b93050b2b ]---
Mapped at:
 [<c05dd335>] debug_dma_map_page+0x44/0xff
 [<dd22ff98>] pci_map_single+0x88/0x94 [via_velocity]
 [<dd230fbc>] velocity_xmit+0xe0/0x2aa [via_velocity]
 [<c074d0ce>] dev_hard_start_xmit+0x1f6/0x2ab
 [<c075de3c>] sch_direct_xmit+0x5c/0x120

I haven't confirmed it yet, but I have a feeling this is due to c79992fddee28bbd31b35ac297e1068d32930179
Doing a build with that backed out now to test.

	Dave

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[Dave Jones]

On Mon, Aug 30, 2010 at 09:13:48PM -0400, Dave Jones wrote:
 
 > WARNING: at lib/dma-debug.c:811 check_unmap+0x212/0x59b()
 > ... 
 > I haven't confirmed it yet, but I have a feeling this is due to c79992fddee28bbd31b35ac297e1068d32930179
 > Doing a build with that backed out now to test.

bad paste. I meant b6ca430599ea37843632b0eaa231dea5414dec25.
 
	Dave

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[Simon Kagstrom]

On Mon, 30 Aug 2010 21:13:49 -0400
Dave Jones <davej@redhat.com> wrote:

> I installed the Fedora 14 alpha, which is based on 2.6.35.2, and hit
> the following trace..
> 
> WARNING: at lib/dma-debug.c:811 check_unmap+0x212/0x59b()
> Hardware name:  
> via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x00000000194ba27e] [map size=66 bytes] [unmap size=182 bytes]

I can't reproduce it here, but does the patch below help for you?

// Simon

>From 81fe86ef9e4be4be43cc75e8320384a0708cef1a Mon Sep 17 00:00:00 2001
From: Simon Kagstrom <simon.kagstrom@netinsight.net>
Date: Tue, 31 Aug 2010 08:41:26 +0200
Subject: [PATCH] via-velocity: Correct packet length on tx free

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/via-velocity.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index fd69095..305192e 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1721,7 +1721,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr,
 			/* For scatter-gather */
 			if (skb_shinfo(skb)->nr_frags > 0)
 				pktlen = max_t(size_t, pktlen,
-						td->td_buf[i].size & ~TD_QUEUE);
+						skb_headlen(skb));
 
 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i],
 					le16_to_cpu(pktlen), PCI_DMA_TODEVICE);
-- 
1.7.0.4

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[Dave Jones]

On Tue, Aug 31, 2010 at 11:10:52AM +0200, Simon Kagstrom wrote:
 > On Mon, 30 Aug 2010 21:13:49 -0400
 > Dave Jones <davej@redhat.com> wrote:
 > 
 > > I installed the Fedora 14 alpha, which is based on 2.6.35.2, and hit
 > > the following trace..
 > > 
 > > WARNING: at lib/dma-debug.c:811 check_unmap+0x212/0x59b()
 > > Hardware name:  
 > > via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x00000000194ba27e] [map size=66 bytes] [unmap size=182 bytes]
 > 
 > I can't reproduce it here, but does the patch below help for you?

It's looking good so far.  It's been up 30 minutes and hasn't triggered yet
(normally it triggers very shortly after bootup)

	Dave

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[Simon Kagstrom]

On Tue, 31 Aug 2010 13:21:07 -0400
Dave Jones <davej@redhat.com> wrote:

> On Tue, Aug 31, 2010 at 11:10:52AM +0200, Simon Kagstrom wrote:
>  > On Mon, 30 Aug 2010 21:13:49 -0400
>  > Dave Jones <davej@redhat.com> wrote:
>  > 
>  > > I installed the Fedora 14 alpha, which is based on 2.6.35.2, and hit
>  > > the following trace..
>  > > 
>  > > WARNING: at lib/dma-debug.c:811 check_unmap+0x212/0x59b()
>  > > Hardware name:  
>  > > via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x00000000194ba27e] [map size=66 bytes] [unmap size=182 bytes]
>  > 
>  > I can't reproduce it here, but does the patch below help for you?
> 
> It's looking good so far.  It's been up 30 minutes and hasn't triggered yet
> (normally it triggers very shortly after bootup)

Sounds good. I'll think a bit more about this and submit the patch
after that.

I'm not really an expert on the subject, but with skb_headlen(), the size
should be the same for pci_map_single and pci_unmap_single as far as I
can tell.

// Simon

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[David Miller]

From: Simon Kagstrom <simon.kagstrom@netinsight.net>
Date: Wed, 1 Sep 2010 12:20:59 +0200

> I'm not really an expert on the subject, but with skb_headlen(), the size
> should be the same for pci_map_single and pci_unmap_single as far as I
> can tell.

That's not the case.

You'll have to use exactly the same formula for computing the length
as the pci_map_single() call uses, which is:

	pktlen = skb_shinfo(skb)->nr_frags == 0 ?
			max_t(unsigned int, skb->len, ETH_ZLEN) :
				skb_headlen(skb);

Otherwise packets smaller than ETH_ZLEN will be unmapped properly
and trigger the same kind of debugging checks Dave is seeing.

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[Dave Jones]

On Wed, Sep 01, 2010 at 01:03:33PM -0700, David Miller wrote:
 > From: Simon Kagstrom <simon.kagstrom@netinsight.net>
 > Date: Wed, 1 Sep 2010 12:20:59 +0200
 > 
 > > I'm not really an expert on the subject, but with skb_headlen(), the size
 > > should be the same for pci_map_single and pci_unmap_single as far as I
 > > can tell.
 > 
 > That's not the case.
 > 
 > You'll have to use exactly the same formula for computing the length
 > as the pci_map_single() call uses, which is:
 > 
 > 	pktlen = skb_shinfo(skb)->nr_frags == 0 ?
 > 			max_t(unsigned int, skb->len, ETH_ZLEN) :
 > 				skb_headlen(skb);
 > 
 > Otherwise packets smaller than ETH_ZLEN will be unmapped properly
 > and trigger the same kind of debugging checks Dave is seeing.

Looks like you're right.

[ 5674.506024] via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x0000000018e555fa] [map size=54 bytes] [unmap size=108 bytes]

	Dave

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Wed, 1 Sep 2010 16:05:55 -0400

> On Wed, Sep 01, 2010 at 01:03:33PM -0700, David Miller wrote:
>  > You'll have to use exactly the same formula for computing the length
>  > as the pci_map_single() call uses, which is:
>  > 
>  > 	pktlen = skb_shinfo(skb)->nr_frags == 0 ?
>  > 			max_t(unsigned int, skb->len, ETH_ZLEN) :
>  > 				skb_headlen(skb);
>  > 
>  > Otherwise packets smaller than ETH_ZLEN will be unmapped properly
>  > and trigger the same kind of debugging checks Dave is seeing.
> 
> Looks like you're right.
> 
> [ 5674.506024] via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x0000000018e555fa] [map size=54 bytes] [unmap size=108 bytes]

Looking more closely at this driver, it is ALL KINDS OF MESSED UP and
is full of mega-lulz wrt. TX dma mapping handling.

It computes a length as an integer, with an override that comes from
the TX descriptor.  Then it unconditionally little-endian converts
the thing.

First, it uses pci_unmap_single() instead of pci_unmap_page() for the
fragments.

Second, for a fragmented SKB it fetches the length from the descriptor
which as we saw can be modified by the chip.

Third, it makes NULL pointer checks that make absolutely no sense.
It checks "td_info->skb_dma" which is an array, that will never be
NULL.  It also checks &(vptr->tx.infos[q][n]) against NULL which
will not be NULL even if vptr->tx.infos is which is maybe what it
meant to check.

Let's try to unravel all of this mess.

Dave can you test out this patch?

Ugh, while writing this I spotted another bug.  It can't do this
ETH_ZLEN thing, it has to use skb_padto().  Otherwise it's just
transmitting arbitrary kernel memory at the end of the SKB
buffer onto the network which is a big no-no.  I'll fix that
with another patch.

via-velocity: Fix TX buffer unmapping.

Fix several bugs in TX buffer DMA unmapping:

1) Use pci_unmap_page() as appropriate.

2) Don't try to fetch the length from the DMA descriptor,
   the chip and modify that value.  Use the correct lengths,
   calculated the same way as is done at map time.

3) Kill meaningless NULL checks (against embedded sized
   arrays which can never be NULL, and against the address
   of the non-zero indexed entry of an array).

Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index fd69095..d5873ac 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1696,6 +1696,13 @@ err_free_dma_rings_0:
 	goto out;
 }
 
+static inline int tx_skb_headlen(struct sk_buff *skb)
+{
+	return skb_shinfo(skb)->nr_frags == 0 ?
+		max_t(unsigned int, skb->len, ETH_ZLEN) :
+		skb_headlen(skb);
+}
+
 /**
  *	velocity_free_tx_buf	-	free transmit buffer
  *	@vptr: velocity
@@ -1705,28 +1712,21 @@ err_free_dma_rings_0:
  *	recycle it, if not then unmap the buffer.
  */
 static void velocity_free_tx_buf(struct velocity_info *vptr,
-		struct velocity_td_info *tdinfo, struct tx_desc *td)
+		struct velocity_td_info *tdinfo)
 {
 	struct sk_buff *skb = tdinfo->skb;
+	int i;
 
-	/*
-	 *	Don't unmap the pre-allocated tx_bufs
-	 */
-	if (tdinfo->skb_dma) {
-		int i;
-
-		for (i = 0; i < tdinfo->nskb_dma; i++) {
-			size_t pktlen = max_t(size_t, skb->len, ETH_ZLEN);
+	pci_unmap_single(vptr->pdev, tdinfo->skb_dma[0],
+			 tx_skb_headlen(skb), PCI_DMA_TODEVICE);
 
-			/* For scatter-gather */
-			if (skb_shinfo(skb)->nr_frags > 0)
-				pktlen = max_t(size_t, pktlen,
-						td->td_buf[i].size & ~TD_QUEUE);
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
 
-			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i],
-					le16_to_cpu(pktlen), PCI_DMA_TODEVICE);
-		}
+		pci_unmap_page(vptr->pdev, tdinfo->skb_dma[i + 1],
+			       frag->size, PCI_DMA_TODEVICE);
 	}
+
 	dev_kfree_skb_irq(skb);
 	tdinfo->skb = NULL;
 }
@@ -1739,22 +1739,8 @@ static void velocity_free_td_ring_entry(struct velocity_info *vptr,
 							 int q, int n)
 {
 	struct velocity_td_info *td_info = &(vptr->tx.infos[q][n]);
-	int i;
-
-	if (td_info == NULL)
-		return;
 
-	if (td_info->skb) {
-		for (i = 0; i < td_info->nskb_dma; i++) {
-			if (td_info->skb_dma[i]) {
-				pci_unmap_single(vptr->pdev, td_info->skb_dma[i],
-					td_info->skb->len, PCI_DMA_TODEVICE);
-				td_info->skb_dma[i] = 0;
-			}
-		}
-		dev_kfree_skb(td_info->skb);
-		td_info->skb = NULL;
-	}
+	velocity_free_tx_buf(vptr, td_info);
 }
 
 /**
@@ -1925,7 +1911,7 @@ static int velocity_tx_srv(struct velocity_info *vptr)
 				stats->tx_packets++;
 				stats->tx_bytes += tdinfo->skb->len;
 			}
-			velocity_free_tx_buf(vptr, tdinfo, td);
+			velocity_free_tx_buf(vptr, tdinfo);
 			vptr->tx.used[qnum]--;
 		}
 		vptr->tx.tail[qnum] = idx;
@@ -2534,9 +2520,7 @@ static netdev_tx_t velocity_xmit(struct sk_buff *skb,
 		return NETDEV_TX_OK;
 	}
 
-	pktlen = skb_shinfo(skb)->nr_frags == 0 ?
-			max_t(unsigned int, skb->len, ETH_ZLEN) :
-				skb_headlen(skb);
+	pktlen = tx_skb_headlen(skb);
 
 	spin_lock_irqsave(&vptr->lock, flags);
 

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 01 Sep 2010 13:34:14 -0700 (PDT)

> Ugh, while writing this I spotted another bug.  It can't do this
> ETH_ZLEN thing, it has to use skb_padto().  Otherwise it's just
> transmitting arbitrary kernel memory at the end of the SKB
> buffer onto the network which is a big no-no.  I'll fix that
> with another patch.

Actually, these ETH_ZLEN things in the length calculation can
just be deleted. It does in fact use skb_padto() properly earlier
in the xmit function.

Re: via-velocity dma-debug warnings again. (2.6.35.2)

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 01 Sep 2010 13:35:47 -0700 (PDT)

> From: David Miller <davem@davemloft.net>
> Date: Wed, 01 Sep 2010 13:34:14 -0700 (PDT)
> 
>> Ugh, while writing this I spotted another bug.  It can't do this
>> ETH_ZLEN thing, it has to use skb_padto().  Otherwise it's just
>> transmitting arbitrary kernel memory at the end of the SKB
>> buffer onto the network which is a big no-no.  I'll fix that
>> with another patch.
> 
> Actually, these ETH_ZLEN things in the length calculation can
> just be deleted. It does in fact use skb_padto() properly earlier
> in the xmit function.

New patch:

via-velocity: Fix TX buffer unmapping.

Fix several bugs in TX buffer DMA unmapping:

1) Use pci_unmap_page() as appropriate.

2) Don't try to fetch the length from the DMA descriptor,
   the chip and modify that value.  Use the correct lengths,
   calculated the same way as is done at map time.

3) Kill meaningless NULL checks (against embedded sized
   arrays which can never be NULL, and against the address
   of the non-zero indexed entry of an array).

4) max() on ETH_ZLEN is not necessary and just adds
   confusion, since the xmit function does a proper
   skb_padto() very early on.

Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index fd69095..4167e1f 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1705,28 +1705,21 @@ err_free_dma_rings_0:
  *	recycle it, if not then unmap the buffer.
  */
 static void velocity_free_tx_buf(struct velocity_info *vptr,
-		struct velocity_td_info *tdinfo, struct tx_desc *td)
+		struct velocity_td_info *tdinfo)
 {
 	struct sk_buff *skb = tdinfo->skb;
+	int i;
 
-	/*
-	 *	Don't unmap the pre-allocated tx_bufs
-	 */
-	if (tdinfo->skb_dma) {
-		int i;
-
-		for (i = 0; i < tdinfo->nskb_dma; i++) {
-			size_t pktlen = max_t(size_t, skb->len, ETH_ZLEN);
+	pci_unmap_single(vptr->pdev, tdinfo->skb_dma[0],
+			 skb_headlen(skb), PCI_DMA_TODEVICE);
 
-			/* For scatter-gather */
-			if (skb_shinfo(skb)->nr_frags > 0)
-				pktlen = max_t(size_t, pktlen,
-						td->td_buf[i].size & ~TD_QUEUE);
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
 
-			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i],
-					le16_to_cpu(pktlen), PCI_DMA_TODEVICE);
-		}
+		pci_unmap_page(vptr->pdev, tdinfo->skb_dma[i + 1],
+			       frag->size, PCI_DMA_TODEVICE);
 	}
+
 	dev_kfree_skb_irq(skb);
 	tdinfo->skb = NULL;
 }
@@ -1739,22 +1732,8 @@ static void velocity_free_td_ring_entry(struct velocity_info *vptr,
 							 int q, int n)
 {
 	struct velocity_td_info *td_info = &(vptr->tx.infos[q][n]);
-	int i;
-
-	if (td_info == NULL)
-		return;
 
-	if (td_info->skb) {
-		for (i = 0; i < td_info->nskb_dma; i++) {
-			if (td_info->skb_dma[i]) {
-				pci_unmap_single(vptr->pdev, td_info->skb_dma[i],
-					td_info->skb->len, PCI_DMA_TODEVICE);
-				td_info->skb_dma[i] = 0;
-			}
-		}
-		dev_kfree_skb(td_info->skb);
-		td_info->skb = NULL;
-	}
+	velocity_free_tx_buf(vptr, td_info);
 }
 
 /**
@@ -1925,7 +1904,7 @@ static int velocity_tx_srv(struct velocity_info *vptr)
 				stats->tx_packets++;
 				stats->tx_bytes += tdinfo->skb->len;
 			}
-			velocity_free_tx_buf(vptr, tdinfo, td);
+			velocity_free_tx_buf(vptr, tdinfo);
 			vptr->tx.used[qnum]--;
 		}
 		vptr->tx.tail[qnum] = idx;
@@ -2534,9 +2513,7 @@ static netdev_tx_t velocity_xmit(struct sk_buff *skb,
 		return NETDEV_TX_OK;
 	}
 
-	pktlen = skb_shinfo(skb)->nr_frags == 0 ?
-			max_t(unsigned int, skb->len, ETH_ZLEN) :
-				skb_headlen(skb);
+	pktlen = skb_headlen(skb);
 
 	spin_lock_irqsave(&vptr->lock, flags);