RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

[Brandeburg, Jesse]

First, off, added netdev..., someone else can add lkml if they so
choose.

akpm@linux-foundation.org wrote:
> The patch titled
>      e1000e: prevent corruption of EEPROM/NVM
> has been added to the -mm tree.  Its filename is
>      e1000e-prevent-corruption-of-eeprom-nvm.patch

This is disturbing, where did this come from? Personal email?  I now
have a spate of *FIVE* reports similar to this, but I haven't see any
other messages on this issue from this user besides andrew's commit.  I
think it may be a real problem, it is same problem as:
http://bugzilla.kernel.org/show_bug.cgi?id=11382

<snip>
> Subject: e1000e: prevent corruption of EEPROM/NVM
> From: Christopher Li <chrisl@vmware.com>
> 
> Andrey reports e1000e corruption, and that a patch in vmware's ESX
> fixed 
> it.
> 
> The EEPROM corruption is triggered by concurrent access of the EEPROM
> read/write. Putting a lock around it solve the problem.
> 
<snip>  I'd like to know how this actually solves a problem outside
vmware.

> +static spinlock_t e1000_eeprom_lock = SPIN_LOCK_UNLOCKED;
> +

note this line changed in andrew's next patch.

>
/***********************************************************************
*******
>   * Set the phy type member in the hw struct.
>   *
> @@ -4904,6 +4908,15 @@ static s32 e1000_spi_eeprom_ready(struct
>  
>
************************************************************************
*****/
>  s32 e1000_read_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
> u16 *data) { +    s32 ret;
> +    spin_lock(&e1000_eeprom_lock);
> +    ret = e1000_do_read_eeprom(hw, offset, words, data);
> +    spin_unlock(&e1000_eeprom_lock);
> +    return ret;
> +}

spacing issues will be fixed when/if we push this to Jeff/Dave.

That is beside the point, why do we need to lock inside something that
is guaranteed to be inside the RTNL lock?

VMWare issues really do not apply to the Linux kernel, and unless the
architecture of ethtool->ioctl->rtnl->e1000e has changed without me
noticing.  In that case every driver that used ethtool would be broken.


> +
> +static s32 e1000_do_read_eeprom(struct e1000_hw *hw, u16 offset, u16
> words, u16 *data) +{
>      struct e1000_eeprom_info *eeprom = &hw->eeprom;
>      u32 i = 0;
> 
> @@ -5236,6 +5249,16 @@ s32 e1000_update_eeprom_checksum(struct
>  
>
************************************************************************
*****/
>  s32 e1000_write_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
> u16 *data) { +    s32 ret;
> +    spin_lock(&e1000_eeprom_lock);
> +    ret = e1000_do_write_eeprom(hw, offset, words, data);
> +    spin_unlock(&e1000_eeprom_lock);
> +    return ret;
> +}

preventing concurrent writes should be guaranteed by the caller (which
should be calling ioctl interfaces under RTNL)

someone please correct me if I'm wrong.

I believe there may actually be a problem, but I actually doubt it is
with e1000e and would be glad to drop eeprom write capability from the
driver in the kernel to prove it.

All signs currently point to some issue introduced with 2.6.27-rc, but
we have no solid reproduction steps or even a local repro.  All the
users with this problem have a shared non-volatile memory(NVM) for the
system which the e1000e's data is just a part of, so something like the
ME (management engine) could be corrupting the area for the LAN.

We are trying to reproduce locally, because 

BTW, anyone who reads this far, DON'T RUN IBAUTIL.EXE to try to fix
this!  It will (somehow) remove your LAN adapter from pci config space.

RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

[Zachary Amsden]

On Tue, 2008-09-02 at 14:58 -0700, Brandeburg, Jesse wrote:
> First, off, added netdev..., someone else can add lkml if they so
> choose.
> 
> akpm@linux-foundation.org wrote:
> > The patch titled
> >      e1000e: prevent corruption of EEPROM/NVM
> > has been added to the -mm tree.  Its filename is
> >      e1000e-prevent-corruption-of-eeprom-nvm.patch
> 
> This is disturbing, where did this come from? Personal email?  I now
> have a spate of *FIVE* reports similar to this, but I haven't see any
> other messages on this issue from this user besides andrew's commit.  I
> think it may be a real problem, it is same problem as:
> http://bugzilla.kernel.org/show_bug.cgi?id=11382
> 
> <snip>
> > Subject: e1000e: prevent corruption of EEPROM/NVM
> > From: Christopher Li <chrisl@vmware.com>
> >
> > Andrey reports e1000e corruption, and that a patch in vmware's ESX
> > fixed
> > it.
> >
> > The EEPROM corruption is triggered by concurrent access of the EEPROM
> > read/write. Putting a lock around it solve the problem.
> >
> <snip>  I'd like to know how this actually solves a problem outside
> vmware.

Yes.  This was observed on a physical E1000 device.

> VMWare issues really do not apply to the Linux kernel, and unless the
> architecture of ethtool->ioctl->rtnl->e1000e has changed without me
> noticing.  In that case every driver that used ethtool would be broken.

No, any drivers that have a state machine consisting of a base register
that is used to index which datum is read from device EEPROM by the data
register would be broken.

This is not a VMware issue, its a fundamental hardware synchronization
issue caused by the dependence of data read/write on a device register.
Does the net layer protect against e1000 issuing simultaneous EEPROM
read / write?  I have no idea.  It probably should.  Should the driver
take extra precautions of its own to ensure this on non hot-paths, to
avoid corrupting EEPROM memory and resulting in a non-functional NIC?

Absolutely.

>From casual inspection it looks like set_mtu is done under dev_baselock, while ethtool is done under rtnl_lock.

Setting the MTU internally reads the EEPROM, which could contend with and corrupt and eeprom operations.

Zach

RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[Brandeburg, Jesse]

Zachary Amsden wrote:
>>> The EEPROM corruption is triggered by concurrent access of the
>>> EEPROM read/write. Putting a lock around it solve the problem.
>>> 
>> <snip>  I'd like to know how this actually solves a problem outside
>> vmware.
> 
> Yes.  This was observed on a physical E1000 device.

Hi Zach, thanks for the reply, can you give me a reproduction test so
that we can verify this fix internally?  We are extremely interested.

<long explanation follows>
 
>> VMWare issues really do not apply to the Linux kernel, and unless the
>> architecture of ethtool->ioctl->rtnl->e1000e has changed without me
>> noticing.  In that case every driver that used ethtool would be
>> broken. 
> 
> No, any drivers that have a state machine consisting of a base
> register 
> that is used to index which datum is read from device EEPROM by the
> data 
> register would be broken.

we are (supposed to be) protected by the eeprom semaphore in a hardware
register, so as long as we are not acquiring that hardware semaphore at
different priority levels (a classic deadlock which I think we don't
have) I'm still missing the bug.
 
> This is not a VMware issue, its a fundamental hardware synchronization
> issue caused by the dependence of data read/write on a device
> register. 
> Does the net layer protect against e1000 issuing simultaneous EEPROM
> read / write?  I have no idea.  It probably should.  Should the driver
> take extra precautions of its own to ensure this on non hot-paths, to
> avoid corrupting EEPROM memory and resulting in a non-functional NIC?
> 
> Absolutely.

I totally agree, which is why the driver / hardware synchronizes access
to the eeprom with a hardware semaphore ( in this case SWFLAG on
82566/567), because in reality we have multiple consumers accessing the
eeprom:
1) on a dual port (like 82571), both ports share the same eeprom
2) the hardware asynchronously auto-reads the eeprom on hw-reset
(CTRL.RST bit) while the driver continues init.
3) ethtool (as you've noted)
4) the manageability engine firmware (aka the ME) may be doing eeprom
operations as well.

If what you claim is true (and I don't doubt it, I just need help
reproducing it) then we have a lower level problem with the eeprom
semaphore/SWFLAG.
 
>> From casual inspection it looks like set_mtu is done under
>> dev_baselock, while ethtool is done under rtnl_lock. 
> 
> Setting the MTU internally reads the EEPROM, which could contend with
> and corrupt and eeprom operations. 

yes, since we issue a full reset and at least 2) above executes.  The
corruption really shouldn't be writing to the eeprom in any way shape or
form, but if it is we really need to fix the root cause of the bug, not
just lock on either side of one of the cases which appears to cause the
problem.

We are definitely investigating, please help us reproduce it if you can.

Jesse

Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[Christopher Li]

On Tue, Sep 02, 2008 at 05:32:26PM -0700, Brandeburg, Jesse wrote:
> yes, since we issue a full reset and at least 2) above executes.  The
> corruption really shouldn't be writing to the eeprom in any way shape or
> form, but if it is we really need to fix the root cause of the bug, not
> just lock on either side of one of the cases which appears to cause the
> problem.
> 
> We are definitely investigating, please help us reproduce it if you can.

Sorry I am late to the discussion.

Can you try to have one process loop on reading the eeprom and
have another process loop on loading/unloading the e1000 driver?
That is the procedure we use in house to reproduce the bug and
verify the fix. It does not happen to every e1000 NICs. We need to
acquired the e1000 NICs from our customer who experienced the
problem. I am not surprised that there is some thing going on
with the hardware eeprom semaphore.

We just try to help when Andrew ask about this bug. I am not
a network driver developer either. Now you know as much as I
do, good luck with the repro. 

Chris

RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[Zachary Amsden]

On Tue, 2008-09-02 at 17:32 -0700, Brandeburg, Jesse wrote:
> Zachary Amsden wrote:
> >>> The EEPROM corruption is triggered by concurrent access of the
> >>> EEPROM read/write. Putting a lock around it solve the problem.
> >>>
> >> <snip>  I'd like to know how this actually solves a problem outside
> >> vmware.
> >
> > Yes.  This was observed on a physical E1000 device.
> 
> Hi Zach, thanks for the reply, can you give me a reproduction test so
> that we can verify this fix internally?  We are extremely interested.
> 
> <long explanation follows>

This was encountered by a customer and fixed by a developer who no
longer works for us.  We can't give any better explanation than - 

1) It looks like there is a massive catastrophic failure that causes
working hardware to DIE.

2) The E1000 driver is really crazily complicated and has to deal with
many different hardware versions, all with different idiosynchronicities
about this EEPROM locking, and it isn't really clear how the hardware
locking protects multiple racing kernel threads in all of these cases.

3) Casual inspection by several people who are not networking experts
shows several possibly viable paths where potential EEPROM corruption
might reasonably occur.

4) Adding locking around EEPROM read and write paths resulted in no more
problems by our customers.

5) It seems LKML customers are getting catastrophic failures where
EEPROMS are getting corrupted.

Thus, the obvious answer to me is ... LOCK AROUND EEPROM READS AND
WRITES now and figure out if there are deeper issues due to network
layer and E1000 complexities later, but ELIMINATE THE CATASTROPHIC
FAILURES ASAP.

> We are definitely investigating, please help us reproduce it if you can.

I'm sorry, we have no way to reproduce this, don't know anything about
the hardware the customer was using and really have almost zero context
on this bug because the developer who addressed the issue and fixed it
is long gone.  We only have statistical data showing 0% reproduction
since a very similar fix to the one you just NACKED.

I know it's not a very useful thing to say.

But seriously, if people's hardware is being hosed, you must take every
precaution in the world to stop it and worry about 'logical' arguments
regarding invariants in the network and driver layers later.

Zach

Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[David Miller]

From: Zachary Amsden <zach@vmware.com>
Date: Wed, 03 Sep 2008 01:17:25 -0700

> Thus, the obvious answer to me is ... LOCK AROUND EEPROM READS AND
> WRITES now and figure out if there are deeper issues due to network
> layer and E1000 complexities later, but ELIMINATE THE CATASTROPHIC
> FAILURES ASAP.

I totally agree with Zach here.

The fact that we are still masterbating about this bug and a fix
isn't in the tree is absolutely rediculious.

Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[Andrey Borzenkov]

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

On Wednesday 03 September 2008, Zachary Amsden wrote:
> I'm sorry, we have no way to reproduce this, don't know anything about
> the hardware the customer was using

According to my information bug affects LAN controllers based on Intel 8254x GBe chipset. Of course, I do not claim it is exhaustive list.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

RE: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

[Allan, Bruce W]

8254x is not supported in e1000e however it is supported in e1000.  It is understandable that such a fix is needed (in e1000) for these older parts that did not have anyway of protecting EEPROM reads/writes as Jesse described in a previous email, but newer parts (supported in e1000e) have alternate methods of protection making the spinlock unnecessary.  There is no evidence the recent NVM/EEPROM corruptions on ICHx-based platforms (in e1000e) are due to the same issue resolved by the proposed fix used in VMware for 8254x parts.

I'm currently investigating this issue and hope to have something soon.

-----Original Message-----
From: Andrey Borzenkov [mailto:arvidjaar@mail.ru]
Sent: Wednesday, September 03, 2008 10:05 AM
To: Zachary Amsden
Cc: Brandeburg, Jesse; Qicheng Christopher Li; akpm@linux-foundation.org; Allan, Bruce W; jeff@garzik.org; Kirsher, Jeffrey T; Ronciak, John; Waskiewicz Jr, Peter P; netdev@vger.kernel.org
Subject: Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mmtree

On Wednesday 03 September 2008, Zachary Amsden wrote:
> I'm sorry, we have no way to reproduce this, don't know anything about
> the hardware the customer was using

According to my information bug affects LAN controllers based on Intel 8254x GBe chipset. Of course, I do not claim it is exhaustive list.

Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

[Ben Hutchings]

Zachary Amsden wrote:
> On Tue, 2008-09-02 at 14:58 -0700, Brandeburg, Jesse wrote:
[...]
> > VMWare issues really do not apply to the Linux kernel, and unless the
> > architecture of ethtool->ioctl->rtnl->e1000e has changed without me
> > noticing.  In that case every driver that used ethtool would be broken.
> 
> No, any drivers that have a state machine consisting of a base register
> that is used to index which datum is read from device EEPROM by the data
> register would be broken.
> 
> This is not a VMware issue, its a fundamental hardware synchronization
> issue caused by the dependence of data read/write on a device register.
> Does the net layer protect against e1000 issuing simultaneous EEPROM
> read / write?

All ethtool commands are serialised by the RTNL as Jesse said above.

> I have no idea.  It probably should.  Should the driver
> take extra precautions of its own to ensure this on non hot-paths, to
> avoid corrupting EEPROM memory and resulting in a non-functional NIC?
> 
> Absolutely.
> 
> From casual inspection it looks like set_mtu is done under dev_baselock, while ethtool is done under rtnl_lock.
> 
> Setting the MTU internally reads the EEPROM, which could contend with and corrupt and eeprom operations.
 
No, MTU changes are done by another ioctl which takes the RTNL.  It's
not hard to find this stuff in net/core/dev.c.

Please do VMware driver developers a favour and pay a little more
attention to such subtleties.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

[Christopher Li]

On Tue, Sep 02, 2008 at 05:44:56PM -0700, Ben Hutchings wrote:
> 
> All ethtool commands are serialised by the RTNL as Jesse said above.
>
What Zach trying to say is that, ioctl is not the only way to
reach the eepro read/write code inside the kernel.

Chris