[PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Anton Blanchard]

There were a few issues in the DMA mapping error handling in e1000_tx_map
which I found via fault injection.

If we fail to map the first descriptor count will end up as -1 but the
check of (count >= 0) will still be true since count is unsigned. Instead
of changing count to be signed, just simplify the logic.

Secondly, when we wrap the tx ring we rely on i to go negative, but it
was unsigned.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux.trees.git/drivers/net/e1000/e1000_main.c
===================================================================
--- linux.trees.git.orig/drivers/net/e1000/e1000_main.c	2010-01-21 11:10:00.000000000 +1100
+++ linux.trees.git/drivers/net/e1000/e1000_main.c	2010-01-21 11:12:52.000000000 +1100
@@ -2693,8 +2693,9 @@ static int e1000_tx_map(struct e1000_ada
 	struct pci_dev *pdev = adapter->pdev;
 	struct e1000_buffer *buffer_info;
 	unsigned int len = skb_headlen(skb);
-	unsigned int offset = 0, size, count = 0, i;
+	unsigned int offset = 0, size, count = 0;
 	unsigned int f;
+	int i;
 
 	i = tx_ring->next_to_use;
 
@@ -2802,10 +2803,8 @@ static int e1000_tx_map(struct e1000_ada
 dma_error:
 	dev_err(&pdev->dev, "TX DMA map failed\n");
 	buffer_info->dma = 0;
-	count--;
 
-	while (count >= 0) {
-		count--;
+	while (count--) {
 		i--;
 		if (i < 0)
 			i += tx_ring->count;

[PATCH 2/2] e1000: Fix DMA mapping error handling on RX

[Anton Blanchard]

Check for error return from pci_map_single/pci_map_page and clean up.

With this and the previous patch the driver was able to handle a significant
percentage of errors (I set the fault injection rate to 10% and could still
download large files at a reasonable speed).

Signed-off-by: Anton Blanchard <anton@samba.org>
---

I wasn't able to stress the jumbo frame path, so that part could do with some
more eyes.

Index: linux.trees.git/drivers/net/e1000/e1000_main.c
===================================================================
--- linux.trees.git.orig/drivers/net/e1000/e1000_main.c	2010-01-21 11:13:10.000000000 +1100
+++ linux.trees.git/drivers/net/e1000/e1000_main.c	2010-01-21 11:15:54.000000000 +1100
@@ -4014,11 +4014,21 @@ check_page:
 			}
 		}
 
-		if (!buffer_info->dma)
+		if (!buffer_info->dma) {
 			buffer_info->dma = pci_map_page(pdev,
 			                                buffer_info->page, 0,
 			                                buffer_info->length,
 			                                PCI_DMA_FROMDEVICE);
+			if (pci_dma_mapping_error(pdev, buffer_info->dma)) {
+				put_page(buffer_info->page);
+				dev_kfree_skb(skb);
+				buffer_info->page = NULL;
+				buffer_info->skb = NULL;
+				buffer_info->dma = 0;
+				adapter->alloc_rx_buff_failed++;
+				break; /* while !buffer_info->skb */
+			}
+		}
 
 		rx_desc = E1000_RX_DESC(*rx_ring, i);
 		rx_desc->buffer_addr = cpu_to_le64(buffer_info->dma);
@@ -4109,6 +4119,13 @@ map_skb:
 						  skb->data,
 						  buffer_info->length,
 						  PCI_DMA_FROMDEVICE);
+		if (pci_dma_mapping_error(pdev, buffer_info->dma)) {
+			dev_kfree_skb(skb);
+			buffer_info->skb = NULL;
+			buffer_info->dma = 0;
+			adapter->alloc_rx_buff_failed++;
+			break; /* while !buffer_info->skb */
+		}
 
 		/*
 		 * XXX if it was allocated cleanly it will never map to a

Re: [PATCH 2/2] e1000: Fix DMA mapping error handling on RX

[Jeff Kirsher]

On Thu, Jan 21, 2010 at 03:44, Anton Blanchard <anton@samba.org> wrote:
>
> Check for error return from pci_map_single/pci_map_page and clean up.
>
> With this and the previous patch the driver was able to handle a significant
> percentage of errors (I set the fault injection rate to 10% and could still
> download large files at a reasonable speed).
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>
> I wasn't able to stress the jumbo frame path, so that part could do with some
> more eyes.
>
> Index: linux.trees.git/drivers/net/e1000/e1000_main.c
> ===================================================================
> --- linux.trees.git.orig/drivers/net/e1000/e1000_main.c 2010-01-21 11:13:10.000000000 +1100
> +++ linux.trees.git/drivers/net/e1000/e1000_main.c      2010-01-21 11:15:54.000000000 +1100
> @@ -4014,11 +4014,21 @@ check_page:
>                        }
>                }
>
> -               if (!buffer_info->dma)
> +               if (!buffer_info->dma) {
>                        buffer_info->dma = pci_map_page(pdev,
>                                                        buffer_info->page, 0,
>                                                        buffer_info->length,
>                                                        PCI_DMA_FROMDEVICE);
> +                       if (pci_dma_mapping_error(pdev, buffer_info->dma)) {
> +                               put_page(buffer_info->page);
> +                               dev_kfree_skb(skb);
> +                               buffer_info->page = NULL;
> +                               buffer_info->skb = NULL;
> +                               buffer_info->dma = 0;
> +                               adapter->alloc_rx_buff_failed++;
> +                               break; /* while !buffer_info->skb */
> +                       }
> +               }
>
>                rx_desc = E1000_RX_DESC(*rx_ring, i);
>                rx_desc->buffer_addr = cpu_to_le64(buffer_info->dma);
> @@ -4109,6 +4119,13 @@ map_skb:
>                                                  skb->data,
>                                                  buffer_info->length,
>                                                  PCI_DMA_FROMDEVICE);
> +               if (pci_dma_mapping_error(pdev, buffer_info->dma)) {
> +                       dev_kfree_skb(skb);
> +                       buffer_info->skb = NULL;
> +                       buffer_info->dma = 0;
> +                       adapter->alloc_rx_buff_failed++;
> +                       break; /* while !buffer_info->skb */
> +               }
>
>                /*
>                 * XXX if it was allocated cleanly it will never map to a

I have added this patch to my queue of patches for review and testing.  Thanks.

-- 
Cheers,
Jeff

Re: [PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Jeff Kirsher]

On Thu, Jan 21, 2010 at 03:42, Anton Blanchard <anton@samba.org> wrote:
>
> There were a few issues in the DMA mapping error handling in e1000_tx_map
> which I found via fault injection.
>
> If we fail to map the first descriptor count will end up as -1 but the
> check of (count >= 0) will still be true since count is unsigned. Instead
> of changing count to be signed, just simplify the logic.
>
> Secondly, when we wrap the tx ring we rely on i to go negative, but it
> was unsigned.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>
> Index: linux.trees.git/drivers/net/e1000/e1000_main.c
> ===================================================================
> --- linux.trees.git.orig/drivers/net/e1000/e1000_main.c 2010-01-21 11:10:00.000000000 +1100
> +++ linux.trees.git/drivers/net/e1000/e1000_main.c      2010-01-21 11:12:52.000000000 +1100
> @@ -2693,8 +2693,9 @@ static int e1000_tx_map(struct e1000_ada
>        struct pci_dev *pdev = adapter->pdev;
>        struct e1000_buffer *buffer_info;
>        unsigned int len = skb_headlen(skb);
> -       unsigned int offset = 0, size, count = 0, i;
> +       unsigned int offset = 0, size, count = 0;
>        unsigned int f;
> +       int i;
>
>        i = tx_ring->next_to_use;
>
> @@ -2802,10 +2803,8 @@ static int e1000_tx_map(struct e1000_ada
>  dma_error:
>        dev_err(&pdev->dev, "TX DMA map failed\n");
>        buffer_info->dma = 0;
> -       count--;
>
> -       while (count >= 0) {
> -               count--;
> +       while (count--) {
>                i--;
>                if (i < 0)
>                        i += tx_ring->count;

This patch does not apply to the current e1000 driver in net-2.6, much
of this patch has already been corrected (applied) by Roel Kluin
recent patch.

-- 
Cheers,
Jeff

Re: [PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Anton Blanchard]

 
Hi,

> This patch does not apply to the current e1000 driver in net-2.6, much
> of this patch has already been corrected (applied) by Roel Kluin
> recent patch.

Sorry I was basing off net-next. I just compared it to my fix and looks like
the patch in net-2.6 has an off by one error doesn't it?

Anton

--

Subject: [PATCH] e1000: Fix DMA mapping error handling on TX

e1000_tx_map has an off by one error in the dma mapping error cleanup path.
We decrement count and never clean the first successfully mapped descriptor.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index d29bb53..427cedd 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2790,8 +2790,6 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 dma_error:
 	dev_err(&pdev->dev, "TX DMA map failed\n");
 	buffer_info->dma = 0;
-	if (count)
-		count--;
 
 	while (count--) {
 		if (i==0)

------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the
world's best and brightest in the field, creating opportunities for Conference
attendees to learn about information security's most important issues through
interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Jeff Kirsher]

On Sat, Jan 23, 2010 at 16:47, Anton Blanchard <anton@samba.org> wrote:
>
> Hi,
>
>> This patch does not apply to the current e1000 driver in net-2.6, much
>> of this patch has already been corrected (applied) by Roel Kluin
>> recent patch.
>
> Sorry I was basing off net-next. I just compared it to my fix and looks like
> the patch in net-2.6 has an off by one error doesn't it?
>
> Anton
>
> --
>

This was discussed during our code review of Roel's patch, and it was
found that there was not an issue.  But I will review the code again
to ensure that there is not "an off by one error".  Thanks for looking
at this.

-- 
Cheers,
Jeff

------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the
world's best and brightest in the field, creating opportunities for Conference
attendees to learn about information security's most important issues through
interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Roel Kluin]

>>> This patch does not apply to the current e1000 driver in net-2.6, much
>>> of this patch has already been corrected (applied) by Roel Kluin
>>> recent patch.
>>
>> Sorry I was basing off net-next. I just compared it to my fix and looks like
>> the patch in net-2.6 has an off by one error doesn't it?
> 
> This was discussed during our code review of Roel's patch, and it was
> found that there was not an issue.  But I will review the code again
> to ensure that there is not "an off by one error".  Thanks for looking
> at this.

He is right, as also reported by Juha Leppanen:

> Before your patch I suppose the logic disregarding the signed/unsigned error was :
> 1) if count==0, no unmapping/freeing inside while loop
> 2) if count>0, do 'count' loops unmapping/freeing
> 
> After your patch the logic is :
> 1) if count==0, no unmapping/freeing inside while loop
> 1) if count==1, no unmapping/freeing inside while loop
> 2) if count>1, do 'count-1' loops unmapping/freeing

> Can tx_ring->count be zero? I hope not.

His suggested fix works:

> dma_error:
> 	dev_err(&pdev->dev, "TX DMA map failed\n");
> 	buffer_info->dma = 0;
> -	if (count)
> -		count--;
> 
> 	while (count--) {
> 		if (i==0)
> -			i += tx_ring->count;
> +			i = tx_ring->count;
> 		i--;
> 		buffer_info = &tx_ring->buffer_info[i];
> 		e1000_unmap_and_free_tx_resource(adapter, buffer_info);
> 	}
> 
> 	return 0;
> }

This affects the patches:
[PATCH] e1000: Fix tests of unsigned in e1000_tx_map()
and the other patch in the same thread.

Do you want me to send a delta patch?

Roel

------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH 1/2] e1000: Fix DMA mapping error handling on TX

[Roel Kluin]

These functions have off by one errors in their dma mapping error cleanup
paths. We decrement count and never clean the first successfully mapped
descriptor.

Reported-by: "Anton Blanchard" <anton@samba.org>
Reported-by: "Juha Leppanen" <juha_motorsportcom@luukku.com>
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 drivers/net/e1000e/netdev.c    |    4 +---
 drivers/net/igbvf/netdev.c     |    4 +---
 drivers/net/ixgb/ixgb_main.c   |    4 +---
 drivers/net/ixgbe/ixgbe_main.c |    4 +---
 4 files changed, 4 insertions(+), 12 deletions(-)

My previous fix inadvertently introduced this change.

The e1000_tx_map() function in drivers/net/e1000/e1000_main.c is already
fixed by the patch sent by Anton Blanchard that can be found here:

http://www.mail-archive.com/e1000-devel@lists.sourceforge.net/msg02313.html

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 57f149b..57c3d44 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3967,12 +3967,10 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 dma_error:
 	dev_err(&pdev->dev, "TX DMA map failed\n");
 	buffer_info->dma = 0;
-	if (count)
-		count--;
 
 	while (count--) {
 		if (i==0)
-			i += tx_ring->count;
+			i = tx_ring->count;
 		i--;
 		buffer_info = &tx_ring->buffer_info[i];
 		e1000_put_txbuf(adapter, buffer_info);;
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index 2aa71a7..3b12603 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -2164,13 +2164,11 @@ dma_error:
 	buffer_info->length = 0;
 	buffer_info->next_to_watch = 0;
 	buffer_info->mapped_as_page = false;
-	if (count)
-		count--;
 
 	/* clear timestamp and dma mappings for remaining portion of packet */
 	while (count--) {
 		if (i==0)
-			i += tx_ring->count;
+			i = tx_ring->count;
 		i--;
 		buffer_info = &tx_ring->buffer_info[i];
 		igbvf_put_txbuf(adapter, buffer_info);
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 593d1a4..de9b36c 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -1363,12 +1363,10 @@ ixgb_tx_map(struct ixgb_adapter *adapter, struct sk_buff *skb,
 dma_error:
 	dev_err(&pdev->dev, "TX DMA map failed\n");
 	buffer_info->dma = 0;
-	if (count)
-		count--;
 
 	while (count--) {
 		if (i==0)
-			i += tx_ring->count;
+			i = tx_ring->count;
 		i--;
 		buffer_info = &tx_ring->buffer_info[i];
 		ixgb_unmap_and_free_tx_resource(adapter, buffer_info);
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index b5f64ad..1713258 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5167,13 +5167,11 @@ dma_error:
 	tx_buffer_info->dma = 0;
 	tx_buffer_info->time_stamp = 0;
 	tx_buffer_info->next_to_watch = 0;
-	if (count)
-		count--;
 
 	/* clear timestamp and dma mappings for remaining portion of packet */
 	while (count--) {
 		if (i==0)
-			i += tx_ring->count;
+			i = tx_ring->count;
 		i--;
 		tx_buffer_info = &tx_ring->tx_buffer_info[i];
 		ixgbe_unmap_and_free_tx_resource(adapter, tx_buffer_info);