Signed-off-by: "Ed L. Cashin" <[email protected]>
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 <[email protected]>
"Ed L. Cashin" <[email protected]> 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 <[email protected]>
On Mon, 26 Sep 2005 12:50:28 EDT, Ed L Cashin said:
> "Ed L. Cashin" <[email protected]> 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....
On Mon, Sep 26, 2005 at 12:50:28PM -0400, Ed L Cashin wrote:
> "Ed L. Cashin" <[email protected]> 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 ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Mon, 26 Sep 2005 [email protected] wrote:
> On Mon, 26 Sep 2005 12:50:28 EDT, Ed L Cashin said:
> > "Ed L. Cashin" <[email protected]> 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
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.
Alan Cox <[email protected]> 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 <[email protected]>
[email protected] 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 <[email protected]>
From: Ed L Cashin <[email protected]>
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.
"David S. Miller" <[email protected]> writes:
> From: Ed L Cashin <[email protected]>
> 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 <[email protected]>