[PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[Davide Ciminaghi]

If interrupts are disabled long enough to allow for more than
32 frames to accumulate in the MAC's internal buffers, a buffer
overflow occurs. This patch fixes the problem by making the
driver's frame_head_info buffer bigger enough.

Signed-off-by: Davide Ciminaghi <ciminaghi@gnudd.com>
Signed-off-by: Raffaele Recalcati <raffaele.recalcati@bticino.it>
---
 drivers/net/ethernet/micrel/ks8851_mll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
index 2784bc7..a158e89 100644
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ b/drivers/net/ethernet/micrel/ks8851_mll.c
@@ -40,7 +40,7 @@
 #define	DRV_NAME	"ks8851_mll"
 
 static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
-#define MAX_RECV_FRAMES			32
+#define MAX_RECV_FRAMES			256
 #define MAX_BUF_SIZE			2048
 #define TX_BUF_SIZE			2000
 #define RX_BUF_SIZE			2000
-- 
1.7.0.4

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[Eric Dumazet]

Le mardi 27 mars 2012 à 15:01 +0200, Davide Ciminaghi a écrit :
> If interrupts are disabled long enough to allow for more than
> 32 frames to accumulate in the MAC's internal buffers, a buffer
> overflow occurs. This patch fixes the problem by making the
> driver's frame_head_info buffer bigger enough.
> 
> Signed-off-by: Davide Ciminaghi <ciminaghi@gnudd.com>
> Signed-off-by: Raffaele Recalcati <raffaele.recalcati@bticino.it>
> ---
>  drivers/net/ethernet/micrel/ks8851_mll.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
> index 2784bc7..a158e89 100644
> --- a/drivers/net/ethernet/micrel/ks8851_mll.c
> +++ b/drivers/net/ethernet/micrel/ks8851_mll.c
> @@ -40,7 +40,7 @@
>  #define	DRV_NAME	"ks8851_mll"
>  
>  static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
> -#define MAX_RECV_FRAMES			32
> +#define MAX_RECV_FRAMES			256
>  #define MAX_BUF_SIZE			2048
>  #define TX_BUF_SIZE			2000
>  #define RX_BUF_SIZE			2000

How can this fix the problem for good ?

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Mar 2012 07:39:31 -0700

> Le mardi 27 mars 2012 à 15:01 +0200, Davide Ciminaghi a écrit :
>> @@ -40,7 +40,7 @@
>>  #define	DRV_NAME	"ks8851_mll"
>>  
>>  static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
>> -#define MAX_RECV_FRAMES			32
>> +#define MAX_RECV_FRAMES			256
>>  #define MAX_BUF_SIZE			2048
>>  #define TX_BUF_SIZE			2000
>>  #define RX_BUF_SIZE			2000
> 
> How can this fix the problem for good ?

Indeed.

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[Raffaele Recalcati]

Hi Eric,

On 07:39 Tue 27 Mar     , Eric Dumazet wrote:
> Le mardi 27 mars 2012 à 15:01 +0200, Davide Ciminaghi a écrit :
> > If interrupts are disabled long enough to allow for more than
> > 32 frames to accumulate in the MAC's internal buffers, a buffer
> > overflow occurs. This patch fixes the problem by making the
> > driver's frame_head_info buffer bigger enough.
> > 
> > Signed-off-by: Davide Ciminaghi <ciminaghi@gnudd.com>
> > Signed-off-by: Raffaele Recalcati <raffaele.recalcati@bticino.it>
> > ---
> >  drivers/net/ethernet/micrel/ks8851_mll.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
> > index 2784bc7..a158e89 100644
> > --- a/drivers/net/ethernet/micrel/ks8851_mll.c
> > +++ b/drivers/net/ethernet/micrel/ks8851_mll.c
> > @@ -40,7 +40,7 @@
> >  #define	DRV_NAME	"ks8851_mll"
> >  
> >  static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
> > -#define MAX_RECV_FRAMES			32
> > +#define MAX_RECV_FRAMES			256
> >  #define MAX_BUF_SIZE			2048
> >  #define TX_BUF_SIZE			2000
> >  #define RX_BUF_SIZE			2000
> 
> How can this fix the problem for good ?

You can see in ks_rcv function, 
ks->frame_cnt = ks_rdreg16(ks, KS_RXFCTR) >> 8;
so "RXFC RX Frame Count" is a byte, maximum 256 total frames.
but we have the threshold ..

As you can see also in ks_setup the 
/* Setup Receive Frame Threshold - 1 frame (RXFCTFC) */
ks_wrreg16(ks, KS_RXFCTR, 1 & RXFCTR_THRESHOLD_MASK);

In conclusion what happen is that if we stop system interrupts for a while,
when we are back the ks_rcv function starts reading the frames, and 
the 
while (ks->frame_cnt--) {
..
	frame_hdr++;
}
loop goes out of the malloc'ed area.

We experienced so strange bugs in the last weeks that we are so happy that it seems fixed, but I need
some weeks to confirm it is robust enough.
The 'load setup' is like the following:
- nfs rootfs
- host $ sudo ping -f $TARGET_IP_ADDRESS
- host $ scp -r BIG_DIR user@$TARGET_IP_ADDRESS:
But I'm not sure to create the bug in a mathematical way, it seems to depend on packets on the corporate lan.

Surely if I stop with jtag and restart after some seconds I have buffer overflow, with same errors in ram that I
obtain randomically with 'load setup'.

Hoping it helps,

Raffaele

> 
> 
> 

Ce message, ainsi que tous les fichiers joints à ce message,
peuvent contenir des informations sensibles et/ ou confidentielles
ne devant pas être divulguées. Si vous n'êtes pas le destinataire
de ce message (ou que vous recevez ce message par erreur), nous
vous remercions de le notifier immédiatement à son expéditeur, et
de détruire ce message. Toute copie, divulgation, modification,
utilisation ou diffusion, non autorisée, directe ou indirecte, de
tout ou partie de ce message, est strictement interdite.

This e-mail, and any document attached hereby, may contain
confidential and/or privileged information. If you are not the
intended recipient (or have received this e-mail in error) please
notify the sender immediately and destroy this e-mail. Any
unauthorized, direct or indirect, copying, disclosure, distribution
or other use of the material or parts thereof is strictly
forbidden.

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[David Miller]

From: Raffaele Recalcati <raffaele.recalcati@bticino.it>
Date: Wed, 28 Mar 2012 09:05:40 +0200

> This e-mail, and any document attached hereby, may contain
> confidential and/or privileged information.

Please do not include confidentiality notices on a public
mailing list.

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[Eric Dumazet]

On Wed, 2012-03-28 at 09:05 +0200, Raffaele Recalcati wrote:
> Hi Eric,
> 
> On 07:39 Tue 27 Mar     , Eric Dumazet wrote:
> > Le mardi 27 mars 2012 à 15:01 +0200, Davide Ciminaghi a écrit :
> > > If interrupts are disabled long enough to allow for more than
> > > 32 frames to accumulate in the MAC's internal buffers, a buffer
> > > overflow occurs. This patch fixes the problem by making the
> > > driver's frame_head_info buffer bigger enough.
> > > 
> > > Signed-off-by: Davide Ciminaghi <ciminaghi@gnudd.com>
> > > Signed-off-by: Raffaele Recalcati <raffaele.recalcati@bticino.it>
> > > ---
> > >  drivers/net/ethernet/micrel/ks8851_mll.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
> > > index 2784bc7..a158e89 100644
> > > --- a/drivers/net/ethernet/micrel/ks8851_mll.c
> > > +++ b/drivers/net/ethernet/micrel/ks8851_mll.c
> > > @@ -40,7 +40,7 @@
> > >  #define	DRV_NAME	"ks8851_mll"
> > >  
> > >  static u8 KS_DEFAULT_MAC_ADDRESS[] = { 0x00, 0x10, 0xA1, 0x86, 0x95, 0x11 };
> > > -#define MAX_RECV_FRAMES			32
> > > +#define MAX_RECV_FRAMES			256
> > >  #define MAX_BUF_SIZE			2048
> > >  #define TX_BUF_SIZE			2000
> > >  #define RX_BUF_SIZE			2000
> > 
> > How can this fix the problem for good ?
> 
> You can see in ks_rcv function, 
> ks->frame_cnt = ks_rdreg16(ks, KS_RXFCTR) >> 8;
> so "RXFC RX Frame Count" is a byte, maximum 256 total frames.
> but we have the threshold ..
> 
> As you can see also in ks_setup the 
> /* Setup Receive Frame Threshold - 1 frame (RXFCTFC) */
> ks_wrreg16(ks, KS_RXFCTR, 1 & RXFCTR_THRESHOLD_MASK);
> 
> In conclusion what happen is that if we stop system interrupts for a while,
> when we are back the ks_rcv function starts reading the frames, and 
> the 
> while (ks->frame_cnt--) {
> ..
> 	frame_hdr++;
> }
> loop goes out of the malloc'ed area.
> 
> We experienced so strange bugs in the last weeks that we are so happy that it seems fixed, but I need
> some weeks to confirm it is robust enough.
> The 'load setup' is like the following:
> - nfs rootfs
> - host $ sudo ping -f $TARGET_IP_ADDRESS
> - host $ scp -r BIG_DIR user@$TARGET_IP_ADDRESS:
> But I'm not sure to create the bug in a mathematical way, it seems to depend on packets on the corporate lan.
> 
> Surely if I stop with jtag and restart after some seconds I have buffer overflow, with same errors in ram that I
> obtain randomically with 'load setup'.
> 
> Hoping it helps,

Sure it does.

So the limit should be 255, not 256.
(0xFFFF >> 8 -> 0xFF)

ks->frame_cnt = ks_rdreg16(ks, KS_RXFCTR) >> 8;

I cant see how 256 can be stored in a 8bit field ?

You see, explaining things in Changelog can actually help a lot, since
we now understand chip has this 8bit limit for the frame counter.

Thanks

Re: [PATCH] net/ethernet: ks8851_mll fix rx frame buffer overflow

[Davide Ciminaghi]

On Wed, Mar 28, 2012 at 09:39:51AM +0200, Eric Dumazet wrote:
> On Wed, 2012-03-28 at 09:05 +0200, Raffaele Recalcati wrote:
> > Hi Eric,
> > 
> > On 07:39 Tue 27 Mar     , Eric Dumazet wrote:
> > > Le mardi 27 mars 2012 ?? 15:01 +0200, Davide Ciminaghi a ??crit :

...

> 
> So the limit should be 255, not 256.
> (0xFFFF >> 8 -> 0xFF)
> 
> ks->frame_cnt = ks_rdreg16(ks, KS_RXFCTR) >> 8;
> 
> I cant see how 256 can be stored in a 8bit field ?
>
you're right, we'll resend the patch with 255.
Well actually, since the chip appears to have 12K of internal rx buffers
and the shortest ethernet frame should be 64 bytes long, maybe the limit could
be set to 12*1024/64 = 192 frames, but I think 255 is safer.
 

Thanks and regards
Davide