[PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ed L. Cashin]

Signed-off-by: "Ed L. Cashin" <ecashin@coraid.com>

Explicitly set the minimum packet length to ETH_ZLEN.

Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
===================================================================
--- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:20:34.000000000 -0400
+++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
@@ -20,6 +20,9 @@
 {
 	struct sk_buff *skb;
 
+	if (len < ETH_ZLEN)
+		len = ETH_ZLEN;
+
 	skb = alloc_skb(len, GFP_ATOMIC);
 	if (skb) {
 		skb->nh.raw = skb->mac.raw = skb->data;


-- 
  Ed L. Cashin <ecashin@coraid.com>

[PATCH 2.6.14-rc2] aoe [2/2]: use get_unaligned for possibly unaligned accesses in ATA id buffer

[Ed L. Cashin]

Signed-off-by: "Ed L. Cashin" <ecashin@coraid.com>

Use get_unaligned for possibly-unaligned multi-byte accesses to the
ATA device identify response buffer.

Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
===================================================================
--- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
+++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
@@ -8,6 +8,7 @@
 #include <linux/blkdev.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
+#include <asm/unaligned.h>
 #include "aoe.h"
 
 #define TIMERTICK (HZ / 10)
@@ -314,16 +315,16 @@
 	u16 n;
 
 	/* word 83: command set supported */
-	n = le16_to_cpup((__le16 *) &id[83<<1]);
+	n = le16_to_cpu(get_unaligned((__le16 *) &id[83<<1]));
 
 	/* word 86: command set/feature enabled */
-	n |= le16_to_cpup((__le16 *) &id[86<<1]);
+	n |= le16_to_cpu(get_unaligned((__le16 *) &id[86<<1]));
 
 	if (n & (1<<10)) {	/* bit 10: LBA 48 */
 		d->flags |= DEVFL_EXT;
 
 		/* word 100: number lba48 sectors */
-		ssize = le64_to_cpup((__le64 *) &id[100<<1]);
+		ssize = le64_to_cpu(get_unaligned((__le64 *) &id[100<<1]));
 
 		/* set as in ide-disk.c:init_idedisk_capacity */
 		d->geo.cylinders = ssize;
@@ -334,12 +335,12 @@
 		d->flags &= ~DEVFL_EXT;
 
 		/* number lba28 sectors */
-		ssize = le32_to_cpup((__le32 *) &id[60<<1]);
+		ssize = le32_to_cpu(get_unaligned((__le32 *) &id[60<<1]));
 
 		/* NOTE: obsolete in ATA 6 */
-		d->geo.cylinders = le16_to_cpup((__le16 *) &id[54<<1]);
-		d->geo.heads = le16_to_cpup((__le16 *) &id[55<<1]);
-		d->geo.sectors = le16_to_cpup((__le16 *) &id[56<<1]);
+		d->geo.cylinders = le16_to_cpu(get_unaligned((__le16 *) &id[54<<1]));
+		d->geo.heads = le16_to_cpu(get_unaligned((__le16 *) &id[55<<1]));
+		d->geo.sectors = le16_to_cpu(get_unaligned((__le16 *) &id[56<<1]));
 	}
 	d->ssize = ssize;
 	d->geo.start = 0;


-- 
  Ed L. Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.14-rc2] aoe [2/2]: use get_unaligned for possibly unaligned accesses in ATA id buffer

[David S. Miller]

From: "Ed L. Cashin" <ecashin@coraid.com>
Date: Mon, 26 Sep 2005 12:45:34 -0400

> Signed-off-by: "Ed L. Cashin" <ecashin@coraid.com>
> 
> Use get_unaligned for possibly-unaligned multi-byte accesses to the
> ATA device identify response buffer.

Thanks for following up on this Ed.

Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ed L Cashin]

"Ed L. Cashin" <ecashin@coraid.com> writes:

...
> Explicitly set the minimum packet length to ETH_ZLEN.
>
> Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
> ===================================================================
> --- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:20:34.000000000 -0400
> +++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
> @@ -20,6 +20,9 @@
>  {
>  	struct sk_buff *skb;
>  
> +	if (len < ETH_ZLEN)
> +		len = ETH_ZLEN;
> +
>  	skb = alloc_skb(len, GFP_ATOMIC);

This change fixes some strange problems observed on a system that was
using the e1000 network driver.  Is the network driver supposed to
ensure that ethernet packets are up to spec, at least 60 bytes long?

-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On Mon, 26 Sep 2005 12:50:28 EDT, Ed L Cashin said:
> "Ed L. Cashin" <ecashin@coraid.com> writes:
> 
> ...
> > Explicitly set the minimum packet length to ETH_ZLEN.
> >
> > Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
> > ===================================================================
> > --- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:20:34.000000000 -0400
> > +++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
> > @@ -20,6 +20,9 @@
> >  {
> >  	struct sk_buff *skb;
> >  
> > +	if (len < ETH_ZLEN)
> > +		len = ETH_ZLEN;
> > +
> >  	skb = alloc_skb(len, GFP_ATOMIC);
> 
> This change fixes some strange problems observed on a system that was
> using the e1000 network driver.  Is the network driver supposed to
> ensure that ethernet packets are up to spec, at least 60 bytes long?

I haven't chased through the code in detail - will this change ensure that
all ETH_ZLEN bytes are initialized?  We had a bunch of drivers a few years
ago that set the length to the legal min, but then only copied some smaller
number of bytes in, resulting in leakage of kernel memory contents....


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Randy.Dunlap]

On Mon, 26 Sep 2005 Valdis.Kletnieks@vt.edu wrote:
> On Mon, 26 Sep 2005 12:50:28 EDT, Ed L Cashin said:
> > "Ed L. Cashin" <ecashin@coraid.com> writes:
> >
> > ...
> > > Explicitly set the minimum packet length to ETH_ZLEN.
> > >
> > > Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
> > > ===================================================================
> > > --- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:20:34.000000000 -0400
> > > +++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
> > > @@ -20,6 +20,9 @@
> > >  {
> > >  	struct sk_buff *skb;
> > >
> > > +	if (len < ETH_ZLEN)
> > > +		len = ETH_ZLEN;
> > > +
> > >  	skb = alloc_skb(len, GFP_ATOMIC);
> >
> > This change fixes some strange problems observed on a system that was
> > using the e1000 network driver.  Is the network driver supposed to
> > ensure that ethernet packets are up to spec, at least 60 bytes long?
>
> I haven't chased through the code in detail - will this change ensure that
> all ETH_ZLEN bytes are initialized?  We had a bunch of drivers a few years
> ago that set the length to the legal min, but then only copied some smaller
> number of bytes in, resulting in leakage of kernel memory contents....

Good point.

On Ed's other question (which I have already trashed -- sorry),
I am familiar with some NICs that automatically pad packets to
ensure min packet size packets, and they know to do so with
a safe data pattern.  However, it's been a few years since I
worked on NIC drivers, so I don't know the current state of
affairs.

-- 
~Randy

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ed L Cashin]

Valdis.Kletnieks@vt.edu writes:

...
> I haven't chased through the code in detail - will this change ensure that
> all ETH_ZLEN bytes are initialized?  We had a bunch of drivers a few years
> ago that set the length to the legal min, but then only copied some smaller
> number of bytes in, resulting in leakage of kernel memory contents....

No, it looks like alloc_skb just kmallocs the data, so I'd need to
follow up with something like this:

diff -rN -u old-aoe-2.6-stand/linux/drivers/block/aoe/aoecmd.c new-aoe-2.6-stand/linux/drivers/block/aoe/aoecmd.c
--- old-aoe-2.6-stand/linux/drivers/block/aoe/aoecmd.c	2005-09-26 18:25:19.000000000 -0400
+++ new-aoe-2.6-stand/linux/drivers/block/aoe/aoecmd.c	2005-09-26 17:08:21.000000000 -0400
@@ -26,6 +26,7 @@
 
 	skb = alloc_skb(len, GFP_ATOMIC);
 	if (skb) {
+		memset(skb->head, 0, skb->end - skb->head);
 		skb->nh.raw = skb->mac.raw = skb->data;
 		skb->dev = if_dev;
 		skb->protocol = __constant_htons(ETH_P_AOE);



-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[David S. Miller]

From: Ed L Cashin <ecashin@coraid.com>
Date: Mon, 26 Sep 2005 18:28:39 -0400

> No, it looks like alloc_skb just kmallocs the data, so I'd need to
> follow up with something like this:

You should explicitly initialize the data areas of the SKB as you
"push" and "put" to allocate space in the data buffer, not right
after alloc_skb() and before you've allocate any space.

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ed L Cashin]

"David S. Miller" <davem@davemloft.net> writes:

> From: Ed L Cashin <ecashin@coraid.com>
> Date: Mon, 26 Sep 2005 18:28:39 -0400
>
>> No, it looks like alloc_skb just kmallocs the data, so I'd need to
>> follow up with something like this:
>
> You should explicitly initialize the data areas of the SKB as you
> "push" and "put" to allocate space in the data buffer, not right
> after alloc_skb() and before you've allocate any space.

Sure, we can do that.  I will resend these patches.

-- 
  Ed L Cashin <ecashin@coraid.com>

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ben Dooks]

On Mon, Sep 26, 2005 at 12:50:28PM -0400, Ed L Cashin wrote:
> "Ed L. Cashin" <ecashin@coraid.com> writes:
> 
> ...
> > Explicitly set the minimum packet length to ETH_ZLEN.
> >
> > Index: 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c
> > ===================================================================
> > --- 2.6.14-rc2-aoe.orig/drivers/block/aoe/aoecmd.c	2005-09-26 12:20:34.000000000 -0400
> > +++ 2.6.14-rc2-aoe/drivers/block/aoe/aoecmd.c	2005-09-26 12:27:49.000000000 -0400
> > @@ -20,6 +20,9 @@
> >  {
> >  	struct sk_buff *skb;
> >  
> > +	if (len < ETH_ZLEN)
> > +		len = ETH_ZLEN;
> > +
> >  	skb = alloc_skb(len, GFP_ATOMIC);
> 
> This change fixes some strange problems observed on a system that was
> using the e1000 network driver.  Is the network driver supposed to
> ensure that ethernet packets are up to spec, at least 60 bytes long?

I belive that 802.3 defines that a packet should be
of at least 64 octets. I belive most ethernet controllers
should consider anything smaller as a `runt`, but as
usual, YMMV.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Alan Cox]

On Llu, 2005-09-26 at 12:50 -0400, Ed L Cashin wrote:
> >  	skb = alloc_skb(len, GFP_ATOMIC);
> 
> This change fixes some strange problems observed on a system that was
> using the e1000 network driver.  Is the network driver supposed to
> ensure that ethernet packets are up to spec, at least 60 bytes long?

The network driver is supposed to pad frames if the hardware cannot and
to blank the spare bits. If it isn't occurring please try and trace down
the offender.

Re: [PATCH 2.6.14-rc2] aoe [1/2]: explicitly set minimum packet length to ETH_ZLEN

[Ed L Cashin]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> On Llu, 2005-09-26 at 12:50 -0400, Ed L Cashin wrote:
>> >  	skb = alloc_skb(len, GFP_ATOMIC);
>> 
>> This change fixes some strange problems observed on a system that was
>> using the e1000 network driver.  Is the network driver supposed to
>> ensure that ethernet packets are up to spec, at least 60 bytes long?
>
> The network driver is supposed to pad frames if the hardware cannot and
> to blank the spare bits. 

Ah ha.

> If it isn't occurring please try and trace down
> the offender.

My colleague Sam observed problems with the e1000 driver in the
2.6.11.4-21.9-smp kernel from Suse 9.3 and also the e1000 driver in
2.6.12-1.1398_FC4smp from Fedora Core 4.  

The problems aren't fully characterized, but AoE ATA read packets
appeared to be getting dropped and/or corrupted.

When using the tg3 driver instead of e1000 the problems went away, and
making the aoe driver alloc_skb with a minimum length of ETH_ZLEN also
made the problems go away.

-- 
  Ed L Cashin <ecashin@coraid.com>