[v2] net: mvneta: fix refilling for Rx DMA buffers

Commit Message

Simon Guinot July 19, 2015, 11 a.m. UTC

With the actual code, if a memory allocation error happens while
refilling a Rx descriptor, then the original Rx buffer is both passed
to the networking stack (in a SKB) and let in the Rx ring. This leads
to various kernel oops and crashes.

As a fix, this patch moves Rx descriptor refilling ahead of building
SKB with the associated Rx buffer. In case of a memory allocation
failure, data is dropped and the original DMA buffer is put back into
the Rx ring.

Signed-off-by: Simon Guinot <[e-mail]>
Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP network unit")
Cc: <[e-mail]> # v3.8+
Tested-by: Yoann Sculo <[e-mail]>
---
 drivers/net/ethernet/marvell/mvneta.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Changes for v2:
- Update commit message.

Comments

David Miller July 21, 2015, 7:30 a.m. UTC | #1

From: Simon Guinot <[e-mail]>
Date: Sun, 19 Jul 2015 13:00:53 +0200

> With the actual code, if a memory allocation error happens while
> refilling a Rx descriptor, then the original Rx buffer is both passed
> to the networking stack (in a SKB) and let in the Rx ring. This leads
> to various kernel oops and crashes.
> 
> As a fix, this patch moves Rx descriptor refilling ahead of building
> SKB with the associated Rx buffer. In case of a memory allocation
> failure, data is dropped and the original DMA buffer is put back into
> the Rx ring.
> 
> Signed-off-by: Simon Guinot <[e-mail]>
> Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP network unit")
> Cc: <[e-mail]> # v3.8+
> Tested-by: Yoann Sculo <[e-mail]>

Applied, thanks.

Simon Guinot Sept. 14, 2015, 10:13 p.m. UTC | #2

Hi Oren,

On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
> I had to undo this change on my Amada 370 based board.  It was causing
> corrupt data to make it through on large downloads.  I'm using wget to get
> the same 30MB file many times and the SHA would occasionally be different.

During your tests, can you see some "Linux processing - Can't refill"
messages along with the data corruptions ?

> I tracked it down to this commit.  In it, I would find on the order of a
> few hundred bytes to simply be wrong data.

I am little bit surprised here. For me, this patch is very simple and
does the exact opposite. It does fix kernel crashes and data corruptions
in case of refilling errors. This can happen for example if you run
large data transfers with jumbo frames enabled...

But anyway, I'll try to reproduce the issue tomorrow. I only have to
wget the same file (size 30MB) in a loop and to check its md5sum ?
That's it ? And how long should I wait for the error ?

Thanks,

Simon

> 
> Thanks,
> 
> Oren
> 
> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <[e-mail]> wrote:
> 
> > From: Simon Guinot <[e-mail]>
> > Date: Sun, 19 Jul 2015 13:00:53 +0200
> >
> > > With the actual code, if a memory allocation error happens while
> > > refilling a Rx descriptor, then the original Rx buffer is both passed
> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
> > > to various kernel oops and crashes.
> > >
> > > As a fix, this patch moves Rx descriptor refilling ahead of building
> > > SKB with the associated Rx buffer. In case of a memory allocation
> > > failure, data is dropped and the original DMA buffer is put back into
> > > the Rx ring.
> > >
> > > Signed-off-by: Simon Guinot <[e-mail]>
> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
> > network unit")
> > > Cc: <[e-mail]> # v3.8+
> > > Tested-by: Yoann Sculo <[e-mail]>
> >
> > Applied, thanks.
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >

Oren Laskin Sept. 14, 2015, 10:16 p.m. UTC | #3

I would hit this error on my Armada 370 board about 20% of the time
after downloading a 30MB file to /tmp.  We're running a 1 Gb SGMII
link.  I would hit this in less than a minute before removing this
commit from my tree.  I've now been running this test in a loop for a
few hours with no problems.

It was somewhat hard to diagnose since files I used scp didn't see the
issues (or at least as quickly).  I set up an http program to serve a
file and replicated the problem with wget and found it.

Oren

On Mon, Sep 14, 2015 at 3:13 PM, Simon Guinot <[e-mail]> wrote:
> Hi Oren,
>
> On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
>> I had to undo this change on my Amada 370 based board.  It was causing
>> corrupt data to make it through on large downloads.  I'm using wget to get
>> the same 30MB file many times and the SHA would occasionally be different.
>
> During your tests, can you see some "Linux processing - Can't refill"
> messages along with the data corruptions ?
>
>> I tracked it down to this commit.  In it, I would find on the order of a
>> few hundred bytes to simply be wrong data.
>
> I am little bit surprised here. For me, this patch is very simple and
> does the exact opposite. It does fix kernel crashes and data corruptions
> in case of refilling errors. This can happen for example if you run
> large data transfers with jumbo frames enabled...
>
> But anyway, I'll try to reproduce the issue tomorrow. I only have to
> wget the same file (size 30MB) in a loop and to check its md5sum ?
> That's it ? And how long should I wait for the error ?
>
> Thanks,
>
> Simon
>
>>
>> Thanks,
>>
>> Oren
>>
>> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <[e-mail]> wrote:
>>
>> > From: Simon Guinot <[e-mail]>
>> > Date: Sun, 19 Jul 2015 13:00:53 +0200
>> >
>> > > With the actual code, if a memory allocation error happens while
>> > > refilling a Rx descriptor, then the original Rx buffer is both passed
>> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
>> > > to various kernel oops and crashes.
>> > >
>> > > As a fix, this patch moves Rx descriptor refilling ahead of building
>> > > SKB with the associated Rx buffer. In case of a memory allocation
>> > > failure, data is dropped and the original DMA buffer is put back into
>> > > the Rx ring.
>> > >
>> > > Signed-off-by: Simon Guinot <[e-mail]>
>> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
>> > network unit")
>> > > Cc: <[e-mail]> # v3.8+
>> > > Tested-by: Yoann Sculo <[e-mail]>
>> >
>> > Applied, thanks.
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > [e-mail]
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> >

Simon Guinot Sept. 14, 2015, 10:46 p.m. UTC | #4

On Mon, Sep 14, 2015 at 03:16:50PM -0700, Oren Laskin wrote:
> I would hit this error on my Armada 370 board about 20% of the time
> after downloading a 30MB file to /tmp.  We're running a 1 Gb SGMII
> link.  I would hit this in less than a minute before removing this
> commit from my tree.  I've now been running this test in a loop for a
> few hours with no problems.

Outch.

At the time I have tested this patch with several runs of 20 wget/md5sum
of a 1GB file (with jumbo frames enabled or not). I have also used this
program: http://git.lacie-nas.org/?p=netsum.git;a=summary. It allows to
detect data corruption over network very quickly.

It is very weird, I should have seen something...

Moreover I understand that you reproduce the issue very quickly and
without any refilling errors. It is also quite weird because the patch
does basically nothing in a such case.

BTW, which hardware are you using exactly ?

Definitively I'll have a closer look at it tomorrow.

Simon

> 
> It was somewhat hard to diagnose since files I used scp didn't see the
> issues (or at least as quickly).  I set up an http program to serve a
> file and replicated the problem with wget and found it.
> 
> Oren
> 
> On Mon, Sep 14, 2015 at 3:13 PM, Simon Guinot <[e-mail]> wrote:
> > Hi Oren,
> >
> > On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
> >> I had to undo this change on my Amada 370 based board.  It was causing
> >> corrupt data to make it through on large downloads.  I'm using wget to get
> >> the same 30MB file many times and the SHA would occasionally be different.
> >
> > During your tests, can you see some "Linux processing - Can't refill"
> > messages along with the data corruptions ?
> >
> >> I tracked it down to this commit.  In it, I would find on the order of a
> >> few hundred bytes to simply be wrong data.
> >
> > I am little bit surprised here. For me, this patch is very simple and
> > does the exact opposite. It does fix kernel crashes and data corruptions
> > in case of refilling errors. This can happen for example if you run
> > large data transfers with jumbo frames enabled...
> >
> > But anyway, I'll try to reproduce the issue tomorrow. I only have to
> > wget the same file (size 30MB) in a loop and to check its md5sum ?
> > That's it ? And how long should I wait for the error ?
> >
> > Thanks,
> >
> > Simon
> >
> >>
> >> Thanks,
> >>
> >> Oren
> >>
> >> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <[e-mail]> wrote:
> >>
> >> > From: Simon Guinot <[e-mail]>
> >> > Date: Sun, 19 Jul 2015 13:00:53 +0200
> >> >
> >> > > With the actual code, if a memory allocation error happens while
> >> > > refilling a Rx descriptor, then the original Rx buffer is both passed
> >> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
> >> > > to various kernel oops and crashes.
> >> > >
> >> > > As a fix, this patch moves Rx descriptor refilling ahead of building
> >> > > SKB with the associated Rx buffer. In case of a memory allocation
> >> > > failure, data is dropped and the original DMA buffer is put back into
> >> > > the Rx ring.
> >> > >
> >> > > Signed-off-by: Simon Guinot <[e-mail]>
> >> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
> >> > network unit")
> >> > > Cc: <[e-mail]> # v3.8+
> >> > > Tested-by: Yoann Sculo <[e-mail]>
> >> >
> >> > Applied, thanks.
> >> >
> >> > _______________________________________________
> >> > linux-arm-kernel mailing list
> >> > [e-mail]
> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >> >

Vincent Donnefort Sept. 15, 2015, 9:09 a.m. UTC | #5

Hi Oren,

On Mon, Sep 14, 2015 at 03:16:50PM -0700, Oren Laskin wrote:
> I would hit this error on my Armada 370 board about 20% of the time
> after downloading a 30MB file to /tmp.  We're running a 1 Gb SGMII
> link.  I would hit this in less than a minute before removing this
> commit from my tree.  I've now been running this test in a loop for a
> few hours with no problems.

I didn't managed to reproduce the issue on a a370-rd board.

What defconfig file are you using? If not mvebu_v7_defconfig, can you
give a try with it?

What kind of hardware are you using?

Simon Guinot Sept. 15, 2015, 8:36 p.m. UTC | #6

On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
> I had to undo this change on my Amada 370 based board.  It was causing
> corrupt data to make it through on large downloads.  I'm using wget to get
> the same 30MB file many times and the SHA would occasionally be different.
> I tracked it down to this commit.  In it, I would find on the order of a
> few hundred bytes to simply be wrong data.

Hi Oren,

Well, you are right. This patch actually introduces a pretty serious
bug. I still don't understand how I missed that in a first place and
I apologize for the inconvenience.

Anyway, I am about to send a patch fixing the issue. Please, can you
test it ?

Thanks in advance,

Simon

> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <[e-mail]> wrote:
> 
> > From: Simon Guinot <[e-mail]>
> > Date: Sun, 19 Jul 2015 13:00:53 +0200
> >
> > > With the actual code, if a memory allocation error happens while
> > > refilling a Rx descriptor, then the original Rx buffer is both passed
> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
> > > to various kernel oops and crashes.
> > >
> > > As a fix, this patch moves Rx descriptor refilling ahead of building
> > > SKB with the associated Rx buffer. In case of a memory allocation
> > > failure, data is dropped and the original DMA buffer is put back into
> > > the Rx ring.
> > >
> > > Signed-off-by: Simon Guinot <[e-mail]>
> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
> > network unit")
> > > Cc: <[e-mail]> # v3.8+
> > > Tested-by: Yoann Sculo <[e-mail]>
> >
> > Applied, thanks.
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >

Patch

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..ac3da11e63a0 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1455,7 +1455,7 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 		     struct mvneta_rx_queue *rxq)
 {
 	struct net_device *dev = pp->dev;
-	int rx_done, rx_filled;
+	int rx_done;
 	u32 rcvd_pkts = 0;
 	u32 rcvd_bytes = 0;
 
@@ -1466,7 +1466,6 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 		rx_todo = rx_done;
 
 	rx_done = 0;
-	rx_filled = 0;
 
 	/* Fairness NAPI loop */
 	while (rx_done < rx_todo) {
@@ -1477,7 +1476,6 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 		int rx_bytes, err;
 
 		rx_done++;
-		rx_filled++;
 		rx_status = rx_desc->status;
 		rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
 		data = (unsigned char *)rx_desc->buf_cookie;
@@ -1517,6 +1515,14 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 			continue;
 		}
 
+		/* Refill processing */
+		err = mvneta_rx_refill(pp, rx_desc);
+		if (err) {
+			netdev_err(dev, "Linux processing - Can't refill\n");
+			rxq->missed++;
+			goto err_drop_frame;
+		}
+
 		skb = build_skb(data, pp->frag_size > PAGE_SIZE ? 0 : pp->frag_size);
 		if (!skb)
 			goto err_drop_frame;
@@ -1536,14 +1542,6 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 		mvneta_rx_csum(pp, rx_status, skb);
 
 		napi_gro_receive(&pp->napi, skb);
-
-		/* Refill processing */
-		err = mvneta_rx_refill(pp, rx_desc);
-		if (err) {
-			netdev_err(dev, "Linux processing - Can't refill\n");
-			rxq->missed++;
-			rx_filled--;
-		}
 	}
 
 	if (rcvd_pkts) {
@@ -1556,7 +1554,7 @@  static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 	}
 
 	/* Update rxq management counters */
-	mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_filled);
+	mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done);
 
 	return rx_done;
 }