netdev - Re: [Intel-wired-lan] [PATCH] igb: use igb_adapter->io_addr instead of e1000_hw->hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Nov 2016 10:35:49 +0100
From:   Corinna Vinschen <vinschen@...hat.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Hisashi T Fujinaka <htodd@...fifty.com>,
        Cao jin <caoj.fnst@...fujitsu.com>,
        Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Izumi, Taku/泉 拓 
        <izumi.taku@...fujitsu.com>
Subject: Re: [Intel-wired-lan] [PATCH] igb: use igb_adapter->io_addr instead
 of e1000_hw->hw_addr

On Nov  8 11:33, Alexander Duyck wrote:
> On Tue, Nov 8, 2016 at 10:37 AM, Corinna Vinschen <vinschen@...hat.com> wrote:
> > On Nov  8 09:16, Hisashi T Fujinaka wrote:
> >> On Tue, 8 Nov 2016, Corinna Vinschen wrote:
> >> > On Nov  8 15:06, Cao jin wrote:
> >> > > When running as guest, under certain condition, it will oops as following.
> >> > > writel() in igb_configure_tx_ring() results in oops, because hw->hw_addr
> >> > > is NULL. While other register access won't oops kernel because they use
> >> > > wr32/rd32 which have a defense against NULL pointer.
> >> > > [...]
> >> >
> >> > Incidentally we're just looking for a solution to that problem too.
> >> > Do three patches to fix the same problem at rougly the same time already
> >> > qualify as freak accident?
> >> >
> >> > FTR, I attached my current patch, which I was planning to submit after
> >> > some external testing.
> >> >
> >> > However, all three patches have one thing in common:  They workaround
> >> > a somewhat dubious resetting of the hardware address to NULL in case
> >> > reading from a register failed.
> >> >
> >> > That makes me wonder if setting the hardware address to NULL in
> >> > rd32/igb_rd32 is really such a good idea.  It's performed in a function
> >> > which return value is *never* tested for validity in the calling
> >> > functions and leads to subsequent crashes since no tests for hw_addr ==
> >> > NULL are performed.
> >> >
> >> > Maybe commit 22a8b2915 should be reconsidered?  Isn't there some more
> >> > graceful way to handle the "surprise removal"?
> >>
> >> Answering this from my home account because, well, work is Outlook.
> >>
> >> "Reconsidering" would be great. In fact, revert if if you'd like. I'm
> >> uncertain that the surprise removal code actually works the way I
> >> thought previously and I think I took a lot of it out of my local code.
> >>
> >> Unfortuantely I don't have any equipment that I can use to reproduce
> >> surprise removal any longer so that means I wouldn't be able to test
> >> anything. I have to defer to you or Cao Jin.
> >
> > I'm not too keen to rip out a PCIe NIC under power from my locale
> > desktop machine, but I think an actual surprise removal is not the
> > problem.
> >
> > As described in my git log entry, the error condition in igb_rd32 can be
> > triggered during a suspend.  The HW has been put into a sleep state but
> > some register read requests are apparently not guarded against that
> > situation.  Reading a register in this state returns -1, thus a suspend
> > is erroneously triggering the "surprise removal" sequence.
> 
> The question I would have is what is reading the device when it is in
> this state.  The watchdog and any other functions that would read the
> device should be disabled.
> 
> One possibility could be a race between a call to igb_close and the
> igb_suspend function.  We have seen some of those pop up recently on
> ixgbe and it looks like igb has the same bug.  We should probably be
> using the rtnl_lock to guarantee that netif_device_detach and the call
> to __igb_close are completed before igb_close could possibly be called
> by the network stack.

Do you have a pointer to the related ixgbe patch, by any chance?

> > Here's a raw idea:
> >
> > - Note that device is suspended in e1000_hw struct.  Don't trigger
> >   error sequence in igb_rd32 if so (...and return a 0 value???)
> 
> The thing is that a suspended device should not be accessed at all.
> If we are accessing it while it is suspended then that is a bug.  If
> you could throw a WARN_ON call in igb_rd32 to capture where this is
> being triggered that might be useful.
> 
> > - Otherwise assume it's actually a surprise removal.  In theory that
> >   should somehow trigger a device removal sequence, kind of like
> >   calling igb_remove, no?
> 
> Well a read of the MMIO region while suspended is more of a surprise
> read since there shouldn't be anything going on.  We need to isolate
> where that read is coming from and fix it.

That would be ideal, but the problem couldn't be reproduced yet apart
from at a customer's customer site.  It's not clear yet if we can access
the machine for further testing.


Corinna

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)