* [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 7:07 ` Ingo Molnar
2008-09-30 3:19 ` [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote: Jesse Brandeburg
` (10 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Bruce Allan, Jesse Brandeburg, Ingo Molnar
From: Bruce Allan <bruce.w.allan@intel.com>
Export set_memory_ro() and set_memory_rw() calls for use by drivers that need
to have more debug information about who might be writing to memory space.
this was initially developed for use while debugging a memory corruption
problem with e1000e.
Ingo Molnar <mingo@elte.hu> said:
>> +EXPORT_SYMBOL(set_memory_rw);
> that's fine, as long as you make it kernel-internal EXPORT_SYMBOL_GPL()
patch was modified with this suggestion.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Acked-by: Ingo Molnar <mingo@elte.hu>
---
arch/x86/mm/pageattr.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 43e2f84..62c1eef 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -906,11 +906,13 @@ int set_memory_ro(unsigned long addr, int numpages)
{
return change_page_attr_clear(addr, numpages, __pgprot(_PAGE_RW));
}
+EXPORT_SYMBOL_GPL(set_memory_ro);
int set_memory_rw(unsigned long addr, int numpages)
{
return change_page_attr_set(addr, numpages, __pgprot(_PAGE_RW));
}
+EXPORT_SYMBOL_GPL(set_memory_rw);
int set_memory_np(unsigned long addr, int numpages)
{
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw
2008-09-30 3:19 ` [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw Jesse Brandeburg
@ 2008-09-30 7:07 ` Ingo Molnar
0 siblings, 0 replies; 46+ messages in thread
From: Ingo Molnar @ 2008-09-30 7:07 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied
* Jesse Brandeburg <jesse.brandeburg@intel.com> wrote:
> From: Bruce Allan <bruce.w.allan@intel.com>
>
> Export set_memory_ro() and set_memory_rw() calls for use by drivers
> that need to have more debug information about who might be writing to
> memory space. this was initially developed for use while debugging a
> memory corruption problem with e1000e.
applied to tip/x86/exports, thanks Jesse.
Ingo
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-10-02 22:23 ` Jesse Barnes
2008-09-30 3:19 ` [RFC PATCH 03/12] e1000e: reset swflag after resetting hardware Jesse Brandeburg
` (9 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Barnes, Jesse Brandeburg
From: Jesse Barnes <jbarnes@virtuousgeek.org>
> I did some snooping around, and while doing so I noticed that the PCI
> mmap code for x86 doesn't do one bit of range checking on the size, or
> any other aspect of the request, wrt. the MMIO regions actually mapped
> in the BARs of the PCI device.
Here's a patch that adds range checking to the sysfs mappings at
least. This patch should catch the case where X (or some other
process) tries to map beyond the specific BAR it's (supposedly)
trying to access, making things safer in general. FWIW both my
F9 and development versions of X start up fine with this patch
applied.
DaveM, will this work for you on sparc? It looked like your code
was allowing bridge window mappings, but that behavior should be
preserved as long as your bridge devices reflect their window
sizes correctly in their pdev->resources?
If we add similar code to the procfs stuff we wouldn't need to do
any checking in the arches.
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/pci/pci-sysfs.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..4d1aa6e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
+#include <linux/sched.h>
#include <linux/pci.h>
#include <linux/stat.h>
#include <linux/topology.h>
@@ -502,6 +503,8 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
struct resource *res = (struct resource *)attr->private;
enum pci_mmap_state mmap_type;
resource_size_t start, end;
+ unsigned long map_len = vma->vm_end - vma->vm_start;
+ unsigned long map_offset = vma->vm_pgoff << PAGE_SHIFT;
int i;
for (i = 0; i < PCI_ROM_RESOURCE; i++)
@@ -510,6 +513,17 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
if (i >= PCI_ROM_RESOURCE)
return -ENODEV;
+ /*
+ * Make sure the range the user is trying to map falls within
+ * the resource
+ */
+ if (map_offset + map_len > pci_resource_len(pdev, i)) {
+ WARN(1, "process \"%s\" tried to map 0x%08lx-0x%08lx on BAR %d (size 0x%08lx)\n",
+ current->comm, map_offset, map_offset + map_len, i,
+ (unsigned long)pci_resource_len(pdev, i));
+ return -EINVAL;
+ }
+
/* pci_mmap_page_range() expects the same kind of entry as coming
* from /proc/bus/pci/ which is a "user visible" value. If this is
* different from the resource itself, arch will do necessary fixup.
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-09-30 3:19 ` [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote: Jesse Brandeburg
@ 2008-10-02 22:23 ` Jesse Barnes
2008-10-03 20:46 ` David Miller
2008-10-07 23:19 ` David Miller
0 siblings, 2 replies; 46+ messages in thread
From: Jesse Barnes @ 2008-10-02 22:23 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied
Ping DaveM. Does this look ok? What else would we need for you to remove
your range checking from sparc?
Thanks,
Jesse
On Monday, September 29, 2008 8:19 pm Jesse Brandeburg wrote:
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> > I did some snooping around, and while doing so I noticed that the PCI
> > mmap code for x86 doesn't do one bit of range checking on the size, or
> > any other aspect of the request, wrt. the MMIO regions actually mapped
> > in the BARs of the PCI device.
>
> Here's a patch that adds range checking to the sysfs mappings at
> least. This patch should catch the case where X (or some other
> process) tries to map beyond the specific BAR it's (supposedly)
> trying to access, making things safer in general. FWIW both my
> F9 and development versions of X start up fine with this patch
> applied.
>
> DaveM, will this work for you on sparc? It looked like your code
> was allowing bridge window mappings, but that behavior should be
> preserved as long as your bridge devices reflect their window
> sizes correctly in their pdev->resources?
>
> If we add similar code to the procfs stuff we wouldn't need to do
> any checking in the arches.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>
> drivers/pci/pci-sysfs.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9c71858..4d1aa6e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -16,6 +16,7 @@
>
>
> #include <linux/kernel.h>
> +#include <linux/sched.h>
> #include <linux/pci.h>
> #include <linux/stat.h>
> #include <linux/topology.h>
> @@ -502,6 +503,8 @@ pci_mmap_resource(struct kobject *kobj, struct
> bin_attribute *attr, struct resource *res = (struct resource
> *)attr->private;
> enum pci_mmap_state mmap_type;
> resource_size_t start, end;
> + unsigned long map_len = vma->vm_end - vma->vm_start;
> + unsigned long map_offset = vma->vm_pgoff << PAGE_SHIFT;
> int i;
>
> for (i = 0; i < PCI_ROM_RESOURCE; i++)
> @@ -510,6 +513,17 @@ pci_mmap_resource(struct kobject *kobj, struct
> bin_attribute *attr, if (i >= PCI_ROM_RESOURCE)
> return -ENODEV;
>
> + /*
> + * Make sure the range the user is trying to map falls within
> + * the resource
> + */
> + if (map_offset + map_len > pci_resource_len(pdev, i)) {
> + WARN(1, "process \"%s\" tried to map 0x%08lx-0x%08lx on BAR %d (size
> 0x%08lx)\n", + current->comm, map_offset, map_offset + map_len, i,
> + (unsigned long)pci_resource_len(pdev, i));
> + return -EINVAL;
> + }
> +
> /* pci_mmap_page_range() expects the same kind of entry as coming
> * from /proc/bus/pci/ which is a "user visible" value. If this is
> * different from the resource itself, arch will do necessary fixup.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-02 22:23 ` Jesse Barnes
@ 2008-10-03 20:46 ` David Miller
2008-10-03 21:29 ` Jesse Barnes
2008-10-07 23:19 ` David Miller
1 sibling, 1 reply; 46+ messages in thread
From: David Miller @ 2008-10-03 20:46 UTC (permalink / raw)
To: jbarnes
Cc: jesse.brandeburg, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, david.graham, bruce.w.allan, jkosina, john.ronciak, tglx,
chris.jones, tim.gardner, airlied
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Thu, 2 Oct 2008 15:23:43 -0700
> Ping DaveM. Does this look ok? What else would we need for you to remove
> your range checking from sparc?
Sorry, I've been in Paris for more than a week otherwise
I would have looked at this already.
I fly home tomorrow, so I'll try to look at it by next
week.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-03 20:46 ` David Miller
@ 2008-10-03 21:29 ` Jesse Barnes
2008-10-03 21:45 ` Jiri Kosina
0 siblings, 1 reply; 46+ messages in thread
From: Jesse Barnes @ 2008-10-03 21:29 UTC (permalink / raw)
To: David Miller
Cc: jesse.brandeburg, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, david.graham, bruce.w.allan, jkosina, john.ronciak, tglx,
chris.jones, tim.gardner, airlied
On Friday, October 3, 2008 1:46 pm David Miller wrote:
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Thu, 2 Oct 2008 15:23:43 -0700
>
> > Ping DaveM. Does this look ok? What else would we need for you to
> > remove your range checking from sparc?
>
> Sorry, I've been in Paris for more than a week otherwise
> I would have looked at this already.
>
> I fly home tomorrow, so I'll try to look at it by next
> week.
Ok, thanks. You'll have to check Linus' tree for sanity though, he just
merged a variant on my patch for 2.6.27. See
b5ff7df3df9efab511244d5a299fce706c71af48 and yell if it broke something for
you.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-03 21:29 ` Jesse Barnes
@ 2008-10-03 21:45 ` Jiri Kosina
2008-10-03 23:28 ` Jesse Brandeburg
0 siblings, 1 reply; 46+ messages in thread
From: Jiri Kosina @ 2008-10-03 21:45 UTC (permalink / raw)
To: Jesse Barnes
Cc: David Miller, jesse.brandeburg, linux-kernel, linux-netdev,
kkeil, agospoda, arjan, david.graham, bruce.w.allan,
john.ronciak, Thomas Gleixner, chris.jones, tim.gardner, airlied,
Olaf Kirch, Linus Torvalds
On Fri, 3 Oct 2008, Jesse Barnes wrote:
> Ok, thanks. You'll have to check Linus' tree for sanity though, he just
> merged a variant on my patch for 2.6.27. See
> b5ff7df3df9efab511244d5a299fce706c71af48 and yell if it broke something
> for you.
Karsten has been testing kernel with these three patches from the series
applied:
e1000e: reset swflag after resetting hardware
e1000e: fix lockdep issues
e1000e: debug contention on NVM SWFLAG
This was done on a hardware which previously triggered the bug in just a
few test iterations in quite a reliable way. Now, with these patches
applied, the EEPROM corruption didn't happen after several tens of
iterations.
Please note, that the patch that disables the writes to EEPROM on hardware
level was *not* involved in this testing.
Therefore it currently seems that these three patches really address the
race condition issue that was present in the e1000e driver.
It is still not clear why the bug started triggering all of a sudden for
so many people though.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-03 21:45 ` Jiri Kosina
@ 2008-10-03 23:28 ` Jesse Brandeburg
2008-10-03 23:30 ` Jesse Brandeburg
2008-10-04 10:21 ` Jiri Kosina
0 siblings, 2 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-10-03 23:28 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jesse Barnes, David Miller, jesse.brandeburg, linux-kernel,
linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, Thomas Gleixner, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Fri, Oct 3, 2008 at 2:45 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> Karsten has been testing kernel with these three patches from the series
> applied:
>
> e1000e: reset swflag after resetting hardware
> e1000e: fix lockdep issues
> e1000e: debug contention on NVM SWFLAG
>
> This was done on a hardware which previously triggered the bug in just a
> few test iterations in quite a reliable way. Now, with these patches
> applied, the EEPROM corruption didn't happen after several tens of
> iterations.
>
> Please note, that the patch that disables the writes to EEPROM on hardware
> level was *not* involved in this testing.
>
> Therefore it currently seems that these three patches really address the
> race condition issue that was present in the e1000e driver.
Our experience is different. We are also testing with the "protection
patch" reverted.
We see that the problem specifically comes and goes when
removing/adding the use of set_memory_ro/set_memory_rw to the driver.
I'm working to catch the bad element in the act with a hardware
breakpoint or an ITP (we're trying both)
> It is still not clear why the bug started triggering all of a sudden for
> so many people though.
we plan to keep on working on this until we understand what is going on.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-03 23:28 ` Jesse Brandeburg
@ 2008-10-03 23:30 ` Jesse Brandeburg
2008-10-04 10:21 ` Jiri Kosina
1 sibling, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-10-03 23:30 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jesse Barnes, David Miller, jesse.brandeburg, linux-kernel,
netdev, kkeil, agospoda, arjan, david.graham, bruce.w.allan,
john.ronciak, Thomas Gleixner, chris.jones, tim.gardner, airlied,
Olaf Kirch, Linus Torvalds
On Fri, Oct 3, 2008 at 4:28 PM, Jesse Brandeburg
<jesse.brandeburg@gmail.com> wrote:
> On Fri, Oct 3, 2008 at 2:45 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> Karsten has been testing kernel with these three patches from the series
>> applied:
>>
>> e1000e: reset swflag after resetting hardware
>> e1000e: fix lockdep issues
>> e1000e: debug contention on NVM SWFLAG
>>
>> This was done on a hardware which previously triggered the bug in just a
>> few test iterations in quite a reliable way. Now, with these patches
>> applied, the EEPROM corruption didn't happen after several tens of
>> iterations.
>>
>> Please note, that the patch that disables the writes to EEPROM on hardware
>> level was *not* involved in this testing.
>>
>> Therefore it currently seems that these three patches really address the
>> race condition issue that was present in the e1000e driver.
>
> Our experience is different. We are also testing with the "protection
> patch" reverted.
>
> We see that the problem specifically comes and goes when
> removing/adding the use of set_memory_ro/set_memory_rw to the driver.
>
> I'm working to catch the bad element in the act with a hardware
> breakpoint or an ITP (we're trying both)
>
>> It is still not clear why the bug started triggering all of a sudden for
>> so many people though.
>
> we plan to keep on working on this until we understand what is going on.
I removed the bad addresses from the cc: list
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-03 23:28 ` Jesse Brandeburg
2008-10-03 23:30 ` Jesse Brandeburg
@ 2008-10-04 10:21 ` Jiri Kosina
2008-10-04 11:02 ` Thomas Gleixner
1 sibling, 1 reply; 46+ messages in thread
From: Jiri Kosina @ 2008-10-04 10:21 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Jesse Barnes, David Miller, jesse.brandeburg, linux-kernel,
linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, Thomas Gleixner, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Fri, 3 Oct 2008, Jesse Brandeburg wrote:
> Our experience is different. We are also testing with the "protection
> patch" reverted.
> We see that the problem specifically comes and goes when
> removing/adding the use of set_memory_ro/set_memory_rw to the driver.
But if this patch (which is an obvious workaround, compared to the other
patches which fix real bugs, right?) would be catching some malicious
accessess to the mapped EEPROM, there should be stacktraces present in the
kernel log, right?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-04 10:21 ` Jiri Kosina
@ 2008-10-04 11:02 ` Thomas Gleixner
2008-10-05 1:24 ` Jesse Brandeburg
0 siblings, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-04 11:02 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jesse Brandeburg, Jesse Barnes, David Miller, jesse.brandeburg,
linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, chris.jones, tim.gardner, airlied,
Olaf Kirch, Linus Torvalds
On Sat, 4 Oct 2008, Jiri Kosina wrote:
> On Fri, 3 Oct 2008, Jesse Brandeburg wrote:
> > Our experience is different. We are also testing with the "protection
> > patch" reverted.
> > We see that the problem specifically comes and goes when
> > removing/adding the use of set_memory_ro/set_memory_rw to the driver.
>
> But if this patch (which is an obvious workaround, compared to the other
> patches which fix real bugs, right?) would be catching some malicious
> accessess to the mapped EEPROM, there should be stacktraces present in the
> kernel log, right?
Exactly. The access to a ro region results in a fault. I have nowhere
seen that trigger, but I can reproduce the trylock() WARN_ON, which
confirms that there is concurrent access to the NVRAM registers. The
backtrace pattern is similar to the one you have seen.
There are two possible bad results from that concurrent access:
1) Task A issues command A
Task B issues command B
Task A writes data for A
which end up in B
2) Task A acquires the software flag
......
Task B acquires the software flag
Task A releases the software flag
The firmware accesses NVRAM Task B accesses the NVRAM
Both are probably serious enough to result in random NVRAM corruption.
There is no doubt: The missing serialization is a real bug.
Your question why this just happens now, while the bug is there for
ever, is definitely a good one. My opinion on that is that we just
have been lucky or some minor modification somewhere else in the
e1000e code or even in the generic/architecture code removed an
accidental serializing effect.
I was not able to reproduce the trylock warning on Fedora 8, but
Fedora 10-Beta triggers it once in 50 boots. I'm not going to remove
the mutex to verify whether it actually would corrupt the NVRAM :)
In theory we should be able to reproduce the problem with older kernel
versions as well. Maybe not the corruption, but we might see the
mutex_trylock check trigger.
Thanks,
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-04 11:02 ` Thomas Gleixner
@ 2008-10-05 1:24 ` Jesse Brandeburg
2008-10-05 8:51 ` Thomas Gleixner
0 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-10-05 1:24 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jiri Kosina, Jesse Barnes, David Miller, jesse.brandeburg,
linux-kernel, netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, chris.jones, tim.gardner, airlied,
Olaf Kirch, Linus Torvalds
On Sat, Oct 4, 2008 at 4:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 4 Oct 2008, Jiri Kosina wrote:
>> On Fri, 3 Oct 2008, Jesse Brandeburg wrote:
>> > Our experience is different. We are also testing with the "protection
>> > patch" reverted.
>> > We see that the problem specifically comes and goes when
>> > removing/adding the use of set_memory_ro/set_memory_rw to the driver.
>>
>> But if this patch (which is an obvious workaround, compared to the other
>> patches which fix real bugs, right?) would be catching some malicious
>> accessess to the mapped EEPROM, there should be stacktraces present in the
>> kernel log, right?
yes, but I think it is just changing timing, i don't see any backtraces either.
> Exactly. The access to a ro region results in a fault. I have nowhere
> seen that trigger, but I can reproduce the trylock() WARN_ON, which
> confirms that there is concurrent access to the NVRAM registers. The
> backtrace pattern is similar to the one you have seen.
are you still getting WARN_ON *with* all the mutex based fixes already applied?
with the mutex patches in place (without protection patch) we are
still reproducing the issue, until we apply the set_memory_ro patch.
I had no luck on friday setting a hardware breakpoint on memory access
with kgdb to catch the writer with a breakpoint.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 1:24 ` Jesse Brandeburg
@ 2008-10-05 8:51 ` Thomas Gleixner
2008-10-05 15:05 ` Arjan van de Ven
0 siblings, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-05 8:51 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Jiri Kosina, Jesse Barnes, David Miller, jesse.brandeburg,
linux-kernel, netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, chris.jones, tim.gardner, airlied,
Olaf Kirch, Linus Torvalds
On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
> > Exactly. The access to a ro region results in a fault. I have nowhere
> > seen that trigger, but I can reproduce the trylock() WARN_ON, which
> > confirms that there is concurrent access to the NVRAM registers. The
> > backtrace pattern is similar to the one you have seen.
>
> are you still getting WARN_ON *with* all the mutex based fixes already applied?
The WARN_ON triggers with current mainline. Is there any fixlet in
Linus tree missing ?
> with the mutex patches in place (without protection patch) we are
> still reproducing the issue, until we apply the set_memory_ro patch.
That does not make sense to me. If the memory_ro patch is providing
_real_ protection then you _must_ run into an access violation. If not,
then the patch just papers over the real problem in some mysterious
way.
The patch does:
+ set_memory_rw((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
writew(val, hw->flash_address + reg);
+ set_memory_ro((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
This changes massively the timing of the flash access. Could this be
the problem on the machine which needs the set_memory_ro patch to
survive ?
> I had no luck on friday setting a hardware breakpoint on memory access
> with kgdb to catch the writer with a breakpoint.
Well, why should you get a hardware breakpoint when the _ro protection
does not trigger in the first place ?
Granted there could be a _rw alias mapping, but then the problem must
be still visible with the _ro patch applied.
Thanks,
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 8:51 ` Thomas Gleixner
@ 2008-10-05 15:05 ` Arjan van de Ven
2008-10-05 15:55 ` Thomas Gleixner
0 siblings, 1 reply; 46+ messages in thread
From: Arjan van de Ven @ 2008-10-05 15:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jesse Brandeburg, Jiri Kosina, Jesse Barnes, David Miller,
jesse.brandeburg, linux-kernel, netdev, kkeil, agospoda,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
Thomas Gleixner wrote:
> On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
>>> Exactly. The access to a ro region results in a fault. I have nowhere
>>> seen that trigger, but I can reproduce the trylock() WARN_ON, which
>>> confirms that there is concurrent access to the NVRAM registers. The
>>> backtrace pattern is similar to the one you have seen.
>> are you still getting WARN_ON *with* all the mutex based fixes already applied?
>
> The WARN_ON triggers with current mainline. Is there any fixlet in
> Linus tree missing ?
>
>> with the mutex patches in place (without protection patch) we are
>> still reproducing the issue, until we apply the set_memory_ro patch.
>
> That does not make sense to me. If the memory_ro patch is providing
> _real_ protection then you _must_ run into an access violation. If not,
> then the patch just papers over the real problem in some mysterious
> way.
>
not if the bad code is doing copy_to_user .... (or similar)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 15:05 ` Arjan van de Ven
@ 2008-10-05 15:55 ` Thomas Gleixner
2008-10-05 16:02 ` Arjan van de Ven
0 siblings, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-05 15:55 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Jesse Brandeburg, Jiri Kosina, Jesse Barnes, David Miller,
jesse.brandeburg, linux-kernel, netdev, kkeil, agospoda,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> Thomas Gleixner wrote:
> > On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
> > > > Exactly. The access to a ro region results in a fault. I have nowhere
> > > > seen that trigger, but I can reproduce the trylock() WARN_ON, which
> > > > confirms that there is concurrent access to the NVRAM registers. The
> > > > backtrace pattern is similar to the one you have seen.
> > > are you still getting WARN_ON *with* all the mutex based fixes already
> > > applied?
> >
> > The WARN_ON triggers with current mainline. Is there any fixlet in
> > Linus tree missing ?
> >
> > > with the mutex patches in place (without protection patch) we are
> > > still reproducing the issue, until we apply the set_memory_ro patch.
> >
> > That does not make sense to me. If the memory_ro patch is providing
> > _real_ protection then you _must_ run into an access violation. If not,
> > then the patch just papers over the real problem in some mysterious
> > way.
> >
>
> not if the bad code is doing copy_to_user .... (or similar)
You mean: copy_from_user :) This would require that the e1000e
nvram region is writable via copy_from_user by an e1000e user space
interface. A quick grep does not reviel such a horrible interface.
Thanks,
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 15:55 ` Thomas Gleixner
@ 2008-10-05 16:02 ` Arjan van de Ven
2008-10-05 16:16 ` Thomas Gleixner
0 siblings, 1 reply; 46+ messages in thread
From: Arjan van de Ven @ 2008-10-05 16:02 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jesse Brandeburg, Jiri Kosina, Jesse Barnes, David Miller,
jesse.brandeburg, linux-kernel, netdev, kkeil, agospoda,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Sun, 5 Oct 2008 17:55:14 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> > Thomas Gleixner wrote:
> > > On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
> > > > > Exactly. The access to a ro region results in a fault. I have
> > > > > nowhere seen that trigger, but I can reproduce the trylock()
> > > > > WARN_ON, which confirms that there is concurrent access to
> > > > > the NVRAM registers. The backtrace pattern is similar to the
> > > > > one you have seen.
> > > > are you still getting WARN_ON *with* all the mutex based fixes
> > > > already applied?
> > >
> > > The WARN_ON triggers with current mainline. Is there any fixlet in
> > > Linus tree missing ?
> > >
> > > > with the mutex patches in place (without protection patch) we
> > > > are still reproducing the issue, until we apply the
> > > > set_memory_ro patch.
> > >
> > > That does not make sense to me. If the memory_ro patch is
> > > providing _real_ protection then you _must_ run into an access
> > > violation. If not, then the patch just papers over the real
> > > problem in some mysterious way.
> > >
> >
> > not if the bad code is doing copy_to_user .... (or similar)
>
> You mean: copy_from_user :) This would require that the e1000e
> nvram region is writable via copy_from_user by an e1000e user space
> interface. A quick grep does not reviel such a horrible interface.
I meant a "copy_to_user" to a duff pointer, somewhere in the kernel.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 16:02 ` Arjan van de Ven
@ 2008-10-05 16:16 ` Thomas Gleixner
2008-10-05 17:01 ` Arjan van de Ven
0 siblings, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-05 16:16 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Jesse Brandeburg, Jiri Kosina, Jesse Barnes, David Miller,
jesse.brandeburg, linux-kernel, netdev, kkeil, agospoda,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> On Sun, 5 Oct 2008 17:55:14 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> > > Thomas Gleixner wrote:
> > > > On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
> > > > > > Exactly. The access to a ro region results in a fault. I have
> > > > > > nowhere seen that trigger, but I can reproduce the trylock()
> > > > > > WARN_ON, which confirms that there is concurrent access to
> > > > > > the NVRAM registers. The backtrace pattern is similar to the
> > > > > > one you have seen.
> > > > > are you still getting WARN_ON *with* all the mutex based fixes
> > > > > already applied?
> > > >
> > > > The WARN_ON triggers with current mainline. Is there any fixlet in
> > > > Linus tree missing ?
> > > >
> > > > > with the mutex patches in place (without protection patch) we
> > > > > are still reproducing the issue, until we apply the
> > > > > set_memory_ro patch.
> > > >
> > > > That does not make sense to me. If the memory_ro patch is
> > > > providing _real_ protection then you _must_ run into an access
> > > > violation. If not, then the patch just papers over the real
> > > > problem in some mysterious way.
> > > >
> > >
> > > not if the bad code is doing copy_to_user .... (or similar)
> >
> > You mean: copy_from_user :) This would require that the e1000e
> > nvram region is writable via copy_from_user by an e1000e user space
> > interface. A quick grep does not reviel such a horrible interface.
>
> I meant a "copy_to_user" to a duff pointer, somewhere in the kernel.
Hmm, don't we check the *to address on copy_to_user ?
Thanks,
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-05 16:16 ` Thomas Gleixner
@ 2008-10-05 17:01 ` Arjan van de Ven
0 siblings, 0 replies; 46+ messages in thread
From: Arjan van de Ven @ 2008-10-05 17:01 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jesse Brandeburg, Jiri Kosina, Jesse Barnes, David Miller,
jesse.brandeburg, linux-kernel, netdev, kkeil, agospoda,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Olaf Kirch, Linus Torvalds
On Sun, 5 Oct 2008 18:16:29 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> > On Sun, 5 Oct 2008 17:55:14 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > > On Sun, 5 Oct 2008, Arjan van de Ven wrote:
> > > > Thomas Gleixner wrote:
> > > > > On Sat, 4 Oct 2008, Jesse Brandeburg wrote:
> > > > > > > Exactly. The access to a ro region results in a fault. I
> > > > > > > have nowhere seen that trigger, but I can reproduce the
> > > > > > > trylock() WARN_ON, which confirms that there is
> > > > > > > concurrent access to the NVRAM registers. The backtrace
> > > > > > > pattern is similar to the one you have seen.
> > > > > > are you still getting WARN_ON *with* all the mutex based
> > > > > > fixes already applied?
> > > > >
> > > > > The WARN_ON triggers with current mainline. Is there any
> > > > > fixlet in Linus tree missing ?
> > > > >
> > > > > > with the mutex patches in place (without protection patch)
> > > > > > we are still reproducing the issue, until we apply the
> > > > > > set_memory_ro patch.
> > > > >
> > > > > That does not make sense to me. If the memory_ro patch is
> > > > > providing _real_ protection then you _must_ run into an access
> > > > > violation. If not, then the patch just papers over the real
> > > > > problem in some mysterious way.
> > > > >
> > > >
> > > > not if the bad code is doing copy_to_user .... (or similar)
> > >
> > > You mean: copy_from_user :) This would require that the e1000e
> > > nvram region is writable via copy_from_user by an e1000e user
> > > space interface. A quick grep does not reviel such a horrible
> > > interface.
> >
> > I meant a "copy_to_user" to a duff pointer, somewhere in the kernel.
>
> Hmm, don't we check the *to address on copy_to_user ?
>
fair point
and we do exception catching for copy_from_user as well on the source,
just by how it's implemented
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:
2008-10-02 22:23 ` Jesse Barnes
2008-10-03 20:46 ` David Miller
@ 2008-10-07 23:19 ` David Miller
1 sibling, 0 replies; 46+ messages in thread
From: David Miller @ 2008-10-07 23:19 UTC (permalink / raw)
To: jbarnes
Cc: jesse.brandeburg, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, david.graham, bruce.w.allan, jkosina, john.ronciak, tglx,
chris.jones, tim.gardner, airlied
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Thu, 2 Oct 2008 15:23:43 -0700
> Ping DaveM. Does this look ok? What else would we need for you to remove
> your range checking from sparc?
I can't do that until you add similar checking to the other
pci_mmap_page_range() call site in drivers/pci/proc.c
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH 03/12] e1000e: reset swflag after resetting hardware
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote: Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 04/12] e1000e: do not ever sleep in interrupt context Jesse Brandeburg
` (8 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Brandeburg
in the process of debugging things, noticed that the swflag is not reset
by the driver after reset, and the swflag is probably not reset unless
management firmware clears it after 100ms.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/ich8lan.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 9e38452..a076079 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -1720,6 +1720,9 @@ static s32 e1000_reset_hw_ich8lan(struct e1000_hw *hw)
ew32(CTRL, (ctrl | E1000_CTRL_RST));
msleep(20);
+ /* release the swflag because it is not reset by hardware reset */
+ e1000_release_swflag_ich8lan(hw);
+
ret_val = e1000e_get_auto_rd_done(hw);
if (ret_val) {
/*
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 04/12] e1000e: do not ever sleep in interrupt context
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (2 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 03/12] e1000e: reset swflag after resetting hardware Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 05/12] e1000e: fix lockdep issues Jesse Brandeburg
` (7 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Brandeburg, Thomas Gleixner
e1000e was apparently calling two functions that attempted to reserve
the SWFLAG bit for exclusive (to hardware and firmware) access to
the PHY and NVM (aka eeprom). These accesses could possibly call
msleep to wait for the resource which is not allowed from interrupt
context.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
drivers/net/e1000e/e1000.h | 2 ++
drivers/net/e1000e/netdev.c | 31 ++++++++++++++++++++++++++++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index ac4e506..ed9d974 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -284,6 +284,8 @@ struct e1000_adapter {
unsigned long led_status;
unsigned int flags;
+ struct work_struct downshift_task;
+ struct work_struct update_phy_task;
};
struct e1000_info {
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index d266510..f8c6c32 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -1115,6 +1115,14 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter)
writel(0, adapter->hw.hw_addr + rx_ring->tail);
}
+static void e1000e_downshift_workaround(struct work_struct *work)
+{
+ struct e1000_adapter *adapter = container_of(work,
+ struct e1000_adapter, downshift_task);
+
+ e1000e_gig_downshift_workaround_ich8lan(&adapter->hw);
+}
+
/**
* e1000_intr_msi - Interrupt Handler
* @irq: interrupt number
@@ -1139,7 +1147,7 @@ static irqreturn_t e1000_intr_msi(int irq, void *data)
*/
if ((adapter->flags & FLAG_LSC_GIG_SPEED_DROP) &&
(!(er32(STATUS) & E1000_STATUS_LU)))
- e1000e_gig_downshift_workaround_ich8lan(hw);
+ schedule_work(&adapter->downshift_task);
/*
* 80003ES2LAN workaround-- For packet buffer work-around on
@@ -1205,7 +1213,7 @@ static irqreturn_t e1000_intr(int irq, void *data)
*/
if ((adapter->flags & FLAG_LSC_GIG_SPEED_DROP) &&
(!(er32(STATUS) & E1000_STATUS_LU)))
- e1000e_gig_downshift_workaround_ich8lan(hw);
+ schedule_work(&adapter->downshift_task);
/*
* 80003ES2LAN workaround--
@@ -2912,6 +2920,21 @@ static int e1000_set_mac(struct net_device *netdev, void *p)
return 0;
}
+/**
+ * e1000e_update_phy_task - work thread to update phy
+ * @work: pointer to our work struct
+ *
+ * this worker thread exists because we must acquire a
+ * semaphore to read the phy, which we could msleep while
+ * waiting for it, and we can't msleep in a timer.
+ **/
+static void e1000e_update_phy_task(struct work_struct *work)
+{
+ struct e1000_adapter *adapter = container_of(work,
+ struct e1000_adapter, update_phy_task);
+ e1000_get_phy_info(&adapter->hw);
+}
+
/*
* Need to wait a few seconds after link up to get diagnostic information from
* the phy
@@ -2919,7 +2942,7 @@ static int e1000_set_mac(struct net_device *netdev, void *p)
static void e1000_update_phy_info(unsigned long data)
{
struct e1000_adapter *adapter = (struct e1000_adapter *) data;
- e1000_get_phy_info(&adapter->hw);
+ schedule_work(&adapter->update_phy_task);
}
/**
@@ -4572,6 +4595,8 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
INIT_WORK(&adapter->reset_task, e1000_reset_task);
INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task);
+ INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
+ INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
e1000e_check_options(adapter);
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 05/12] e1000e: fix lockdep issues
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (3 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 04/12] e1000e: do not ever sleep in interrupt context Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 06/12] e1000e: drop stats lock Jesse Brandeburg
` (6 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Thomas Gleixner, Jesse Brandeburg
thanks to tglx, we're finding some interesting lockdep issues.
The good news is that this patch fixes all the ones I
could find, without damaging any functionality.
CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/ethtool.c | 6 +++++-
drivers/net/e1000e/netdev.c | 13 -------------
2 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index e21c9e0..f3b49f6 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -432,6 +432,10 @@ static void e1000_get_regs(struct net_device *netdev,
regs_buff[11] = er32(TIDV);
regs_buff[12] = adapter->hw.phy.type; /* PHY type (IGP=1, M88=0) */
+
+ /* ethtool doesn't use anything past this point, so all this
+ * code is likely legacy junk for apps that may or may not
+ * exist */
if (hw->phy.type == e1000_phy_m88) {
e1e_rphy(hw, M88E1000_PHY_SPEC_STATUS, &phy_data);
regs_buff[13] = (u32)phy_data; /* cable length */
@@ -447,7 +451,7 @@ static void e1000_get_regs(struct net_device *netdev,
regs_buff[22] = adapter->phy_stats.receive_errors;
regs_buff[23] = regs_buff[13]; /* mdix mode */
}
- regs_buff[21] = adapter->phy_stats.idle_errors; /* phy idle errors */
+ regs_buff[21] = 0; /* was idle_errors */
e1e_rphy(hw, PHY_1000T_STATUS, &phy_data);
regs_buff[24] = (u32)phy_data; /* phy local receiver status */
regs_buff[25] = regs_buff[24]; /* phy remote receiver status */
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index f8c6c32..44ce120 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2954,9 +2954,6 @@ void e1000e_update_stats(struct e1000_adapter *adapter)
struct e1000_hw *hw = &adapter->hw;
struct pci_dev *pdev = adapter->pdev;
unsigned long irq_flags;
- u16 phy_tmp;
-
-#define PHY_IDLE_ERROR_COUNT_MASK 0x00FF
/*
* Prevent stats update while adapter is being reset, or if the pci
@@ -3045,15 +3042,6 @@ void e1000e_update_stats(struct e1000_adapter *adapter)
/* Tx Dropped needs to be maintained elsewhere */
- /* Phy Stats */
- if (hw->phy.media_type == e1000_media_type_copper) {
- if ((adapter->link_speed == SPEED_1000) &&
- (!e1e_rphy(hw, PHY_1000T_STATUS, &phy_tmp))) {
- phy_tmp &= PHY_IDLE_ERROR_COUNT_MASK;
- adapter->phy_stats.idle_errors += phy_tmp;
- }
- }
-
/* Management Stats */
adapter->stats.mgptc += er32(MGTPTC);
adapter->stats.mgprc += er32(MGTPRC);
@@ -3073,7 +3061,6 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
int ret_val;
unsigned long irq_flags;
-
spin_lock_irqsave(&adapter->stats_lock, irq_flags);
if ((er32(STATUS) & E1000_STATUS_LU) &&
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 06/12] e1000e: drop stats lock
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (4 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 05/12] e1000e: fix lockdep issues Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 3:19 ` [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG Jesse Brandeburg
` (5 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Brandeburg, Thomas Gleixner
the stats lock is left over from e1000, e1000e no longer
has the adjust tbi stats function that required the addition
of the stats lock to begin with.
adding a mutex to acquire_swflag helped catch this one too.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
drivers/net/e1000e/e1000.h | 1 -
drivers/net/e1000e/netdev.c | 18 ------------------
2 files changed, 0 insertions(+), 19 deletions(-)
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index ed9d974..f80e43a 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -257,7 +257,6 @@ struct e1000_adapter {
struct net_device *netdev;
struct pci_dev *pdev;
struct net_device_stats net_stats;
- spinlock_t stats_lock; /* prevent concurrent stats updates */
/* structs defined in e1000_hw.h */
struct e1000_hw hw;
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 44ce120..9aa3c79 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2600,8 +2600,6 @@ static int __devinit e1000_sw_init(struct e1000_adapter *adapter)
/* Explicitly disable IRQ since the NIC can be in any state. */
e1000_irq_disable(adapter);
- spin_lock_init(&adapter->stats_lock);
-
set_bit(__E1000_DOWN, &adapter->state);
return 0;
@@ -2953,7 +2951,6 @@ void e1000e_update_stats(struct e1000_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
struct pci_dev *pdev = adapter->pdev;
- unsigned long irq_flags;
/*
* Prevent stats update while adapter is being reset, or if the pci
@@ -2964,14 +2961,6 @@ void e1000e_update_stats(struct e1000_adapter *adapter)
if (pci_channel_offline(pdev))
return;
- spin_lock_irqsave(&adapter->stats_lock, irq_flags);
-
- /*
- * these counters are modified from e1000_adjust_tbi_stats,
- * called from the interrupt context, so they must only
- * be written while holding adapter->stats_lock
- */
-
adapter->stats.crcerrs += er32(CRCERRS);
adapter->stats.gprc += er32(GPRC);
adapter->stats.gorc += er32(GORCL);
@@ -3046,8 +3035,6 @@ void e1000e_update_stats(struct e1000_adapter *adapter)
adapter->stats.mgptc += er32(MGTPTC);
adapter->stats.mgprc += er32(MGTPRC);
adapter->stats.mgpdc += er32(MGTPDC);
-
- spin_unlock_irqrestore(&adapter->stats_lock, irq_flags);
}
/**
@@ -3059,9 +3046,6 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
struct e1000_hw *hw = &adapter->hw;
struct e1000_phy_regs *phy = &adapter->phy_regs;
int ret_val;
- unsigned long irq_flags;
-
- spin_lock_irqsave(&adapter->stats_lock, irq_flags);
if ((er32(STATUS) & E1000_STATUS_LU) &&
(adapter->hw.phy.media_type == e1000_media_type_copper)) {
@@ -3092,8 +3076,6 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
phy->stat1000 = 0;
phy->estatus = (ESTATUS_1000_TFULL | ESTATUS_1000_THALF);
}
-
- spin_unlock_irqrestore(&adapter->stats_lock, irq_flags);
}
static void e1000_print_link_info(struct e1000_adapter *adapter)
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (5 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 06/12] e1000e: drop stats lock Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-10-02 14:28 ` Jiri Kosina
2008-09-30 3:19 ` [RFC PATCH 08/12] e1000e: allow bad checksum Jesse Brandeburg
` (4 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Thomas Gleixner, Jesse Brandeburg
From: Thomas Gleixner <tglx@linutronix.de>
This patch adds a mutex to the e1000e driver that would help
catch any collisions of two e1000e threads accessing hardware
at the same time.
description and patch updated by Jesse
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/ich8lan.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index a076079..57c6d2f 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -366,6 +366,9 @@ static s32 e1000_get_variants_ich8lan(struct e1000_adapter *adapter)
return 0;
}
+static DEFINE_MUTEX(nvm_mutex);
+static pid_t nvm_owner = -1;
+
/**
* e1000_acquire_swflag_ich8lan - Acquire software control flag
* @hw: pointer to the HW structure
@@ -379,6 +382,15 @@ static s32 e1000_acquire_swflag_ich8lan(struct e1000_hw *hw)
u32 extcnf_ctrl;
u32 timeout = PHY_CFG_TIMEOUT;
+ WARN_ON(preempt_count());
+
+ if (!mutex_trylock(&nvm_mutex)) {
+ WARN(1, KERN_ERR "e1000e mutex contention. Owned by pid %d\n",
+ nvm_owner);
+ mutex_lock(&nvm_mutex);
+ }
+ nvm_owner = current->pid;
+
while (timeout) {
extcnf_ctrl = er32(EXTCNF_CTRL);
extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
@@ -393,6 +405,8 @@ static s32 e1000_acquire_swflag_ich8lan(struct e1000_hw *hw)
if (!timeout) {
hw_dbg(hw, "FW or HW has locked the resource for too long.\n");
+ nvm_owner = -1;
+ mutex_unlock(&nvm_mutex);
return -E1000_ERR_CONFIG;
}
@@ -414,6 +428,9 @@ static void e1000_release_swflag_ich8lan(struct e1000_hw *hw)
extcnf_ctrl = er32(EXTCNF_CTRL);
extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
ew32(EXTCNF_CTRL, extcnf_ctrl);
+
+ nvm_owner = -1;
+ mutex_unlock(&nvm_mutex);
}
/**
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-09-30 3:19 ` [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG Jesse Brandeburg
@ 2008-10-02 14:28 ` Jiri Kosina
2008-10-02 15:03 ` Olaf Kirch
0 siblings, 1 reply; 46+ messages in thread
From: Jiri Kosina @ 2008-10-02 14:28 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, Thomas Gleixner, chris.jones,
tim.gardner, airlied, Thomas Gleixner, Olaf Kirch
On Mon, 29 Sep 2008, Jesse Brandeburg wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> This patch adds a mutex to the e1000e driver that would help
> catch any collisions of two e1000e threads accessing hardware
> at the same time.
>
> description and patch updated by Jesse
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>
> drivers/net/e1000e/ich8lan.c | 17 +++++++++++++++++
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
> index a076079..57c6d2f 100644
> --- a/drivers/net/e1000e/ich8lan.c
> +++ b/drivers/net/e1000e/ich8lan.c
> @@ -366,6 +366,9 @@ static s32 e1000_get_variants_ich8lan(struct e1000_adapter *adapter)
> return 0;
> }
>
> +static DEFINE_MUTEX(nvm_mutex);
> +static pid_t nvm_owner = -1;
> +
> /**
> * e1000_acquire_swflag_ich8lan - Acquire software control flag
> * @hw: pointer to the HW structure
> @@ -379,6 +382,15 @@ static s32 e1000_acquire_swflag_ich8lan(struct e1000_hw *hw)
> u32 extcnf_ctrl;
> u32 timeout = PHY_CFG_TIMEOUT;
>
> + WARN_ON(preempt_count());
> +
> + if (!mutex_trylock(&nvm_mutex)) {
> + WARN(1, KERN_ERR "e1000e mutex contention. Owned by pid %d\n",
> + nvm_owner);
> + mutex_lock(&nvm_mutex);
> + }
> + nvm_owner = current->pid;
> +
> while (timeout) {
> extcnf_ctrl = er32(EXTCNF_CTRL);
> extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
> @@ -393,6 +405,8 @@ static s32 e1000_acquire_swflag_ich8lan(struct e1000_hw *hw)
>
> if (!timeout) {
> hw_dbg(hw, "FW or HW has locked the resource for too long.\n");
> + nvm_owner = -1;
> + mutex_unlock(&nvm_mutex);
> return -E1000_ERR_CONFIG;
> }
>
> @@ -414,6 +428,9 @@ static void e1000_release_swflag_ich8lan(struct e1000_hw *hw)
> extcnf_ctrl = er32(EXTCNF_CTRL);
> extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
> ew32(EXTCNF_CTRL, extcnf_ctrl);
> +
> + nvm_owner = -1;
> + mutex_unlock(&nvm_mutex);
> }
A few minutes ago, I have actually just hit this, while debugging the
issue on a kernel that had this patch included.
I was not successful reproducing it yet though, but still it might be a
pointer into direction where the real bug is.
15:49:07 linux-pr0e dhclient: Listening on LPF/eth1/00:15:58:c6:4a:ff
15:49:07 linux-pr0e dhclient: Sending on LPF/eth1/00:15:58:c6:4a:ff
15:49:07 linux-pr0e dhclient: Sending on Socket/fallback
15:49:07 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 3
15:49:10 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 8
15:49:18 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 9
15:49:27 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 9
15:49:36 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 17
15:49:53 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 12
15:50:05 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 3
15:50:08 linux-pr0e dhclient: No DHCPOFFERS received.
15:50:08 linux-pr0e dhclient: No working leases in persistent database - sleeping.
15:50:52 linux-pr0e kernel: ------------[ cut here ]------------
15:50:52 linux-pr0e kernel: WARNING: at drivers/net/e1000e/ich8lan.c:424 e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]()
15:50:52 linux-pr0e kernel: e1000e mutex contention. Owned by pid 4162
15:50:52 linux-pr0e kernel: Modules linked in: af_packet i915 drm ipv6 snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq microcode fuse loop dm_mod tulip arc4 ecb snd_hda_intel snd_pcm crypto_blkcipher rtc_cmos snd_timer ppdev iwl3945 thinkpad_acpi pcmcia uvcvideo parport_pc rtc_core snd_page_alloc video rfkill i2c_i801 mac80211 iTCO_wdt compat_ioctl32 rtc_lib yenta_socket pcspkr joydev ohci1394 snd_hwdep rsrc_nonstatic output i2c_core btusb parport battery led_class videodev ac ieee1394 v4l1_compat e1000e wmi iTCO_vendor_support pcmcia_core button snd soundcore intel_agp cfg80211 bluetooth sg sr_mod cdrom sd_mod crc_t10dif ehci_hcd uhci_hcd usbcore edd ext3 mbcache jbd fan ide_pci_generic ide_core ata_generic ata_piix ahci pata_acpi libata scsi_mod dock thermal processor
15:50:52 linux-pr0e kernel: Pid: 7, comm: events/0 Tainted: G 2.6.27-rc7-7.10-default #1
15:50:52 linux-pr0e kernel:
15:50:52 linux-pr0e kernel: Call Trace:
15:50:52 linux-pr0e kernel: [<ffffffff8020e41e>] show_trace_log_lvl+0x41/0x58
15:50:52 linux-pr0e kernel: [<ffffffff80493716>] dump_stack+0x69/0x6f
15:50:52 linux-pr0e kernel: [<ffffffff8023ee54>] warn_slowpath+0xb4/0xdc
15:50:52 linux-pr0e kernel: [<ffffffffa022ce2e>] e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]
15:50:52 linux-pr0e kernel: [<ffffffffa02317ba>] e1000e_read_phy_reg_igp+0x19/0x64 [e1000e]
15:50:52 linux-pr0e kernel: [<ffffffffa02319f8>] e1000e_phy_has_link_generic+0x50/0xcc [e1000e]
15:50:52 linux-pr0e kernel: [<ffffffffa02306f9>] e1000e_check_for_copper_link+0x24/0x86 [e1000e]
15:50:52 linux-pr0e kernel: [<ffffffffa0236982>] e1000_watchdog_task+0x5c/0x5eb [e1000e]
15:50:52 linux-pr0e kernel: [<ffffffff8024ecdb>] run_workqueue+0xa4/0x14c
15:50:52 linux-pr0e kernel: [<ffffffff8024ee5b>] worker_thread+0xd8/0xe7
15:50:52 linux-pr0e kernel: [<ffffffff80251fe5>] kthread+0x47/0x73
15:50:52 linux-pr0e kernel: [<ffffffff8020d7a9>] child_rip+0xa/0x11
15:50:52 linux-pr0e kernel:
15:50:52 linux-pr0e kernel: ---[ end trace 6f68a3c748ede326 ]---
15:51:25 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 3
15:51:28 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 8
15:51:36 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 13
15:51:49 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 13
15:52:02 linux-pr0e dhclient: DHCPDISCOVER on eth1 to 255.255.255.255 port 67 interval 18
15:52:15 linux-pr0e kernel: Machine check events logged
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 14:28 ` Jiri Kosina
@ 2008-10-02 15:03 ` Olaf Kirch
2008-10-02 16:27 ` Brandeburg, Jesse
2008-10-02 23:42 ` [PATCH] e1000e: prevent concurrent access to NVRAM Thomas Gleixner
0 siblings, 2 replies; 46+ messages in thread
From: Olaf Kirch @ 2008-10-02 15:03 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jesse Brandeburg, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, david.graham, bruce.w.allan, john.ronciak,
Thomas Gleixner, chris.jones, tim.gardner, airlied
On Thursday 02 October 2008 16:28:42 Jiri Kosina wrote:
>
> 15:50:52 linux-pr0e kernel: WARNING: at drivers/net/e1000e/ich8lan.c:424 e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]()
> 15:50:52 linux-pr0e kernel: e1000e mutex contention. Owned by pid 4162
> 15:50:52 linux-pr0e kernel: Call Trace:
> 15:50:52 linux-pr0e kernel: [<ffffffff8020e41e>] show_trace_log_lvl+0x41/0x58
> 15:50:52 linux-pr0e kernel: [<ffffffff80493716>] dump_stack+0x69/0x6f
> 15:50:52 linux-pr0e kernel: [<ffffffff8023ee54>] warn_slowpath+0xb4/0xdc
> 15:50:52 linux-pr0e kernel: [<ffffffffa022ce2e>] e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]
> 15:50:52 linux-pr0e kernel: [<ffffffffa02317ba>] e1000e_read_phy_reg_igp+0x19/0x64 [e1000e]
> 15:50:52 linux-pr0e kernel: [<ffffffffa02319f8>] e1000e_phy_has_link_generic+0x50/0xcc [e1000e]
> 15:50:52 linux-pr0e kernel: [<ffffffffa02306f9>] e1000e_check_for_copper_link+0x24/0x86 [e1000e]
> 15:50:52 linux-pr0e kernel: [<ffffffffa0236982>] e1000_watchdog_task+0x5c/0x5eb [e1000e]
> 15:50:52 linux-pr0e kernel: [<ffffffff8024ecdb>] run_workqueue+0xa4/0x14c
> 15:50:52 linux-pr0e kernel: [<ffffffff8024ee5b>] worker_thread+0xd8/0xe7
> 15:50:52 linux-pr0e kernel: [<ffffffff80251fe5>] kthread+0x47/0x73
> 15:50:52 linux-pr0e kernel: [<ffffffff8020d7a9>] child_rip+0xa/0x11
Looks like the e1000 watchdog racing with some dhclient activity (upping the interface).
I just noticed that the driver actually uses register pages. So it looks like it's
possible to have something like this without the mutex:
process A selects page A
process B selects page B
process A writes to register at offset A'
So we may end up writing to the wrong register. I think I heard Vojtech mention
that the e1000e also has a register based interface to erase/rewrite the NVM
programmatically. Do we know at which offsets these registers live?
Olaf
--
Neo didn't bring down the Matrix. SOA did.
--soafacts.com
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 15:03 ` Olaf Kirch
@ 2008-10-02 16:27 ` Brandeburg, Jesse
2008-10-02 17:33 ` Olaf Kirch
2008-10-02 18:02 ` Thomas Gleixner
2008-10-02 23:42 ` [PATCH] e1000e: prevent concurrent access to NVRAM Thomas Gleixner
1 sibling, 2 replies; 46+ messages in thread
From: Brandeburg, Jesse @ 2008-10-02 16:27 UTC (permalink / raw)
To: Olaf Kirch, Jiri Kosina
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, Graham,
David, Allan, Bruce W, Ronciak, John, Thomas Gleixner,
chris.jones, tim.gardner, airlied
Olaf Kirch wrote:
> Looks like the e1000 watchdog racing with some dhclient activity
> (upping the interface).
> I just noticed that the driver actually uses register pages. So it
> looks like it's possible to have something like this without the
> mutex:
>
> process A selects page A
> process B selects page B
> process A writes to register at offset A'
I think that is possible, which is why the mutex patch would be good for
the future. However we have not shown that to be happening as a root
cause, but I don't rule it out.
so, why now? Drivers since before the e1000/e1000e split had this same
code, with no reports of problems. This code has been heavily tested,
and one of the platforms easily reproducing this has been available for
3 years now (ich8), with code that is basically unchanged in the driver.
> So we may end up writing to the wrong register. I think I heard
> Vojtech mention
> that the e1000e also has a register based interface to erase/rewrite
> the NVM programmatically. Do we know at which offsets these registers
> live?
The flash control registers are documented in the ICH documentation, and
are located at physical memory location indicated by BAR1 in the config
space of device 0:19.0
I wonder if we couldn't put a check in to see if the value we end up
reading from the register controlling the operation matches the
operation we were expecting (read vs write vs block erase)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 16:27 ` Brandeburg, Jesse
@ 2008-10-02 17:33 ` Olaf Kirch
2008-10-02 18:58 ` Thomas Gleixner
2008-10-02 18:02 ` Thomas Gleixner
1 sibling, 1 reply; 46+ messages in thread
From: Olaf Kirch @ 2008-10-02 17:33 UTC (permalink / raw)
To: Brandeburg, Jesse
Cc: Jiri Kosina, linux-kernel, linux-netdev, kkeil, agospoda, arjan,
Graham, David, Allan, Bruce W, Ronciak, John, Thomas Gleixner,
chris.jones, tim.gardner, airlied
On Thursday 02 October 2008 18:27:12 Brandeburg, Jesse wrote:
> so, why now? Drivers since before the e1000/e1000e split had this same
> code, with no reports of problems. This code has been heavily tested,
> and one of the platforms easily reproducing this has been available for
> 3 years now (ich8), with code that is basically unchanged in the driver.
Possibly the dhcp client is doing something differently, or at a much higher
frequency. At any rate, it seems we're seeing this now even when we just
use init level 3, without X involvement. Karsten reports NVM corruption
after 34 reboots into init level 3.
> The flash control registers are documented in the ICH documentation, and
> are located at physical memory location indicated by BAR1 in the config
> space of device 0:19.0
>
> I wonder if we couldn't put a check in to see if the value we end up
> reading from the register controlling the operation matches the
> operation we were expecting (read vs write vs block erase)
That may help, yes.
Olaf
--
Neo didn't bring down the Matrix. SOA did.
--soafacts.com
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 17:33 ` Olaf Kirch
@ 2008-10-02 18:58 ` Thomas Gleixner
2008-10-02 19:07 ` Olaf Kirch
0 siblings, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-02 18:58 UTC (permalink / raw)
To: Olaf Kirch
Cc: Brandeburg, Jesse, Jiri Kosina, linux-kernel, linux-netdev,
kkeil, agospoda, arjan, Graham, David, Allan, Bruce W, Ronciak,
John, chris.jones, tim.gardner, airlied
On Thu, 2 Oct 2008, Olaf Kirch wrote:
> On Thursday 02 October 2008 18:27:12 Brandeburg, Jesse wrote:
> > so, why now? Drivers since before the e1000/e1000e split had this same
> > code, with no reports of problems. This code has been heavily tested,
> > and one of the platforms easily reproducing this has been available for
> > 3 years now (ich8), with code that is basically unchanged in the driver.
>
> Possibly the dhcp client is doing something differently, or at a much higher
> frequency. At any rate, it seems we're seeing this now even when we just
> use init level 3, without X involvement. Karsten reports NVM corruption
> after 34 reboots into init level 3.
Had Karsten the mutex patch applied or not ?
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 18:58 ` Thomas Gleixner
@ 2008-10-02 19:07 ` Olaf Kirch
2008-10-02 19:08 ` Olaf Kirch
0 siblings, 1 reply; 46+ messages in thread
From: Olaf Kirch @ 2008-10-02 19:07 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Brandeburg, Jesse, Jiri Kosina, linux-kernel, linux-netdev,
kkeil, agospoda, arjan, Graham, David, Allan, Bruce W, Ronciak,
John, chris.jones, tim.gardner, airlied
On Thursday 02 October 2008 20:58:43 Thomas Gleixner wrote:
> On Thu, 2 Oct 2008, Olaf Kirch wrote:
>
> > On Thursday 02 October 2008 18:27:12 Brandeburg, Jesse wrote:
> > > so, why now? Drivers since before the e1000/e1000e split had this same
> > > code, with no reports of problems. This code has been heavily tested,
> > > and one of the platforms easily reproducing this has been available for
> > > 3 years now (ich8), with code that is basically unchanged in the driver.
> >
> > Possibly the dhcp client is doing something differently, or at a much higher
> > frequency. At any rate, it seems we're seeing this now even when we just
> > use init level 3, without X involvement. Karsten reports NVM corruption
> > after 34 reboots into init level 3.
>
> Had Karsten the mutex patch applied or not ?
That was openSuse 11.1 without any patches
Olaf
--
Neo didn't bring down the Matrix. SOA did.
--soafacts.com
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 19:07 ` Olaf Kirch
@ 2008-10-02 19:08 ` Olaf Kirch
0 siblings, 0 replies; 46+ messages in thread
From: Olaf Kirch @ 2008-10-02 19:08 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Brandeburg, Jesse, Jiri Kosina, linux-kernel, linux-netdev,
kkeil, agospoda, arjan, Graham, David, Allan, Bruce W, Ronciak,
John, chris.jones, tim.gardner, airlied
On Thursday 02 October 2008 21:07:22 Olaf Kirch wrote:
> That was openSuse 11.1 without any patches
I should have said 11.1 beta1.
Olaf
--
Neo didn't bring down the Matrix. SOA did.
--soafacts.com
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG
2008-10-02 16:27 ` Brandeburg, Jesse
2008-10-02 17:33 ` Olaf Kirch
@ 2008-10-02 18:02 ` Thomas Gleixner
1 sibling, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-02 18:02 UTC (permalink / raw)
To: Brandeburg, Jesse
Cc: Olaf Kirch, Jiri Kosina, linux-kernel, linux-netdev, kkeil,
agospoda, arjan, Graham, David, Allan, Bruce W, Ronciak, John,
chris.jones, tim.gardner, airlied
On Thu, 2 Oct 2008, Brandeburg, Jesse wrote:
> Olaf Kirch wrote:
> > Looks like the e1000 watchdog racing with some dhclient activity
> > (upping the interface).
>
> > I just noticed that the driver actually uses register pages. So it
> > looks like it's possible to have something like this without the
> > mutex:
> >
> > process A selects page A
> > process B selects page B
> > process A writes to register at offset A'
>
> I think that is possible, which is why the mutex patch would be good for
> the future. However we have not shown that to be happening as a root
> cause, but I don't rule it out.
Nevertheless I vote strongly for putting that check in _NOW_. It has
proven that there is concurrent access and that's definitely a bug by
all means.
> so, why now? Drivers since before the e1000/e1000e split had this same
> code, with no reports of problems. This code has been heavily tested,
> and one of the platforms easily reproducing this has been available for
> 3 years now (ich8), with code that is basically unchanged in the driver.
Well, timing of events changes slightly over time and we definitely
had some major changes in the last three years which influence timing
(high res timers, dynticks, NAPI ....)
Thanks,
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH] e1000e: prevent concurrent access to NVRAM
2008-10-02 15:03 ` Olaf Kirch
2008-10-02 16:27 ` Brandeburg, Jesse
@ 2008-10-02 23:42 ` Thomas Gleixner
2008-10-03 0:19 ` Jesse Brandeburg
1 sibling, 1 reply; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-02 23:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Olaf Kirch, Jiri Kosina, Jesse Brandeburg, LKML,
linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, chris.jones, tim.gardner, airlied,
Ingo Molnar
On Thu, 2 Oct 2008, Olaf Kirch wrote:
> On Thursday 02 October 2008 16:28:42 Jiri Kosina wrote:
> >
> > 15:50:52 linux-pr0e kernel: WARNING: at drivers/net/e1000e/ich8lan.c:424 e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]()
> > 15:50:52 linux-pr0e kernel: e1000e mutex contention. Owned by pid 4162
> > 15:50:52 linux-pr0e kernel: Call Trace:
> > 15:50:52 linux-pr0e kernel: [<ffffffff8020e41e>] show_trace_log_lvl+0x41/0x58
> > 15:50:52 linux-pr0e kernel: [<ffffffff80493716>] dump_stack+0x69/0x6f
> > 15:50:52 linux-pr0e kernel: [<ffffffff8023ee54>] warn_slowpath+0xb4/0xdc
> > 15:50:52 linux-pr0e kernel: [<ffffffffa022ce2e>] e1000_acquire_swflag_ich8lan+0x5a/0xdc [e1000e]
> > 15:50:52 linux-pr0e kernel: [<ffffffffa02317ba>] e1000e_read_phy_reg_igp+0x19/0x64 [e1000e]
> > 15:50:52 linux-pr0e kernel: [<ffffffffa02319f8>] e1000e_phy_has_link_generic+0x50/0xcc [e1000e]
> > 15:50:52 linux-pr0e kernel: [<ffffffffa02306f9>] e1000e_check_for_copper_link+0x24/0x86 [e1000e]
> > 15:50:52 linux-pr0e kernel: [<ffffffffa0236982>] e1000_watchdog_task+0x5c/0x5eb [e1000e]
> > 15:50:52 linux-pr0e kernel: [<ffffffff8024ecdb>] run_workqueue+0xa4/0x14c
> > 15:50:52 linux-pr0e kernel: [<ffffffff8024ee5b>] worker_thread+0xd8/0xe7
> > 15:50:52 linux-pr0e kernel: [<ffffffff80251fe5>] kthread+0x47/0x73
> > 15:50:52 linux-pr0e kernel: [<ffffffff8020d7a9>] child_rip+0xa/0x11
>
> Looks like the e1000 watchdog racing with some dhclient activity (upping the interface).
>
> I just noticed that the driver actually uses register pages. So it looks like it's
> possible to have something like this without the mutex:
>
> process A selects page A
> process B selects page B
> process A writes to register at offset A'
>
> So we may end up writing to the wrong register. I think I heard Vojtech mention
> that the e1000e also has a register based interface to erase/rewrite the NVM
> programmatically. Do we know at which offsets these registers live?
Linus,
can you please apply the patch below which prevents the concurrent
access to the NVRAM. It triggered the trace which Olaf reported above.
I worked out that patch on Sept 24th and it triggered a couple of
problems in the e1000e code right away. These have been addressed by
Jesse and are part of the patch series he posted in the last days.
Still, all we have in mainline is some "hopefully bug preventing"
patch which does not address the root cause at all.
The confirmed bugs where the nvram acquire code was called
concurrently are still in your tree and the prevention patch along
with the resulting bugfixes are stuck in some obscure intel QA
process.
Please apply at least the bug prevention patch below.
Thanks,
tglx
---
Subject: e1000e prevent concurrent access and debug contention on NVM SWFALG
Date: Wed, 24 Sep 2008 00:45:08 -0700
From: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: jesse.brandeburg@intel.com
---
drivers/net/e1000e/ich8lan.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Index: linux-2.6/drivers/net/e1000e/ich8lan.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000e/ich8lan.c
+++ linux-2.6/drivers/net/e1000e/ich8lan.c
@@ -382,6 +382,9 @@ static s32 e1000_get_variants_ich8lan(st
return 0;
}
+static DEFINE_MUTEX(nvm_mutex);
+static pid_t nvm_owner = -1;
+
/**
* e1000_acquire_swflag_ich8lan - Acquire software control flag
* @hw: pointer to the HW structure
@@ -395,6 +398,15 @@ static s32 e1000_acquire_swflag_ich8lan(
u32 extcnf_ctrl;
u32 timeout = PHY_CFG_TIMEOUT;
+ WARN_ON(preempt_count());
+
+ if (!mutex_trylock(&nvm_mutex)) {
+ WARN(1, KERN_ERR "e1000e mutex contention. Owned by pid %d\n",
+ nvm_owner);
+ mutex_lock(&nvm_mutex);
+ }
+ nvm_owner = current->pid;
+
while (timeout) {
extcnf_ctrl = er32(EXTCNF_CTRL);
extcnf_ctrl |= E1000_EXTCNF_CTRL_SWFLAG;
@@ -409,6 +421,8 @@ static s32 e1000_acquire_swflag_ich8lan(
if (!timeout) {
hw_dbg(hw, "FW or HW has locked the resource for too long.\n");
+ nvm_owner = -1;
+ mutex_unlock(&nvm_mutex);
return -E1000_ERR_CONFIG;
}
@@ -430,6 +444,9 @@ static void e1000_release_swflag_ich8lan
extcnf_ctrl = er32(EXTCNF_CTRL);
extcnf_ctrl &= ~E1000_EXTCNF_CTRL_SWFLAG;
ew32(EXTCNF_CTRL, extcnf_ctrl);
+
+ nvm_owner = -1;
+ mutex_unlock(&nvm_mutex);
}
/**
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] e1000e: prevent concurrent access to NVRAM
2008-10-02 23:42 ` [PATCH] e1000e: prevent concurrent access to NVRAM Thomas Gleixner
@ 2008-10-03 0:19 ` Jesse Brandeburg
2008-10-03 0:28 ` Thomas Gleixner
0 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-10-03 0:19 UTC (permalink / raw)
To: Thomas Gleixner, Linus Torvalds
Cc: Andrew Morton, Olaf Kirch, Jiri Kosina, Jesse Brandeburg, LKML,
linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, john.ronciak, chris.jones, tim.gardner, airlied,
Ingo Molnar
On Thu, Oct 2, 2008 at 4:42 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> The confirmed bugs where the nvram acquire code was called
> concurrently are still in your tree and the prevention patch along
> with the resulting bugfixes are stuck in some obscure intel QA
> process.
>
> Please apply at least the bug prevention patch below.
This is the same patch I posted 7 minutes ago, except that this patch
without the e1000e changes applied before it will cause all sorts of
WARN's to be printed during normal operation. If at all possible I
think they should stay together as a group to prevent un-necessary
noise in the logs.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] e1000e: prevent concurrent access to NVRAM
2008-10-03 0:19 ` Jesse Brandeburg
@ 2008-10-03 0:28 ` Thomas Gleixner
0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2008-10-03 0:28 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Linus Torvalds, Andrew Morton, Olaf Kirch, Jiri Kosina,
Jesse Brandeburg, LKML, linux-netdev, kkeil, agospoda, arjan,
david.graham, bruce.w.allan, john.ronciak, chris.jones,
tim.gardner, airlied, Ingo Molnar
On Thu, 2 Oct 2008, Jesse Brandeburg wrote:
> On Thu, Oct 2, 2008 at 4:42 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > The confirmed bugs where the nvram acquire code was called
> > concurrently are still in your tree and the prevention patch along
> > with the resulting bugfixes are stuck in some obscure intel QA
> > process.
> >
> > Please apply at least the bug prevention patch below.
>
> This is the same patch I posted 7 minutes ago, except that this patch
> without the e1000e changes applied before it will cause all sorts of
> WARN's to be printed during normal operation. If at all possible I
> think they should stay together as a group to prevent un-necessary
> noise in the logs.
Sure, I'm all for the combo of those. I just wanted to get some motion
into this stale process.
tglx
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH 08/12] e1000e: allow bad checksum
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (6 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG Jesse Brandeburg
@ 2008-09-30 3:19 ` Jesse Brandeburg
2008-09-30 8:38 ` Jiri Kosina
2008-09-30 3:20 ` [RFC PATCH 09/12] e1000e: dump eeprom to dmesg for ich8/9 Jesse Brandeburg
` (3 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Brandeburg
currently if the driver notices a bad checksum it will fail to
load. This patch allows the driver load process to continue with
an invalid mac address and could allow the user to use ethtool or
another app to fix the eeprom.
copied from implementation in e1000
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/netdev.c | 81 ++++++++++++++++++++++++++++++++++++-------
1 files changed, 67 insertions(+), 14 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 9aa3c79..48b0ded 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4338,6 +4338,52 @@ static void e1000_eeprom_checks(struct e1000_adapter *adapter)
}
/**
+ * e1000e_dump_eeprom - write the eeprom to kernel log
+ * @adapter: our adapter struct
+ *
+ * Dump the eeprom for users having checksum issues
+ **/
+static void e1000e_dump_eeprom(struct e1000_adapter *adapter)
+{
+ struct net_device *netdev = adapter->netdev;
+ struct ethtool_eeprom eeprom;
+ const struct ethtool_ops *ops = netdev->ethtool_ops;
+ u8 *data;
+ int i;
+ u16 csum_old, csum_new = 0;
+
+ eeprom.len = ops->get_eeprom_len(netdev);
+ eeprom.offset = 0;
+
+ data = kzalloc(eeprom.len, GFP_KERNEL);
+ if (!data) {
+ printk(KERN_ERR "Unable to allocate memory to dump EEPROM"
+ " data\n");
+ return;
+ }
+
+ ops->get_eeprom(netdev, &eeprom, data);
+
+ csum_old = (data[NVM_CHECKSUM_REG * 2]) +
+ (data[NVM_CHECKSUM_REG * 2 + 1] << 8);
+ for (i = 0; i < NVM_CHECKSUM_REG * 2; i += 2)
+ csum_new += data[i] + (data[i + 1] << 8);
+ csum_new = NVM_SUM - csum_new;
+
+ printk(KERN_ERR "/*********************/\n");
+ printk(KERN_ERR "Current EEPROM Checksum : 0x%04x\n", csum_old);
+ printk(KERN_ERR "Calculated : 0x%04x\n", csum_new);
+
+ printk(KERN_ERR "Offset Values\n");
+ printk(KERN_ERR "======== ======\n");
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1, data, 128, 0);
+
+ printk(KERN_ERR "/*********************/\n");
+
+ kfree(data);
+}
+
+/**
* e1000_probe - Device Initialization Routine
* @pdev: PCI device information struct
* @ent: entry in e1000_pci_tbl
@@ -4527,31 +4573,39 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
* attempt. Let's give it a few tries
*/
for (i = 0;; i++) {
- if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
+ if (e1000_validate_nvm_checksum(hw) >= 0) {
+ /* copy the MAC address out of the NVM */
+ if (e1000e_read_mac_addr(&adapter->hw))
+ e_err("NVM Read Error reading MAC address\n");
break;
+ }
if (i == 2) {
e_err("The NVM Checksum Is Not Valid\n");
- err = -EIO;
- goto err_eeprom;
+ e1000e_dump_eeprom(adapter);
+ /*
+ * set MAC address to all zeroes to invalidate and
+ * temporary disable this device for the user. This
+ * blocks regular traffic while still permitting
+ * ethtool ioctls from reaching the hardware as well as
+ * allowing the user to run the interface after
+ * manually setting a hw addr using
+ * `ip link set address`
+ */
+ memset(hw->mac.addr, 0, netdev->addr_len);
+ break;
}
}
e1000_eeprom_checks(adapter);
- /* copy the MAC address out of the NVM */
- if (e1000e_read_mac_addr(&adapter->hw))
- e_err("NVM Read Error while reading MAC address\n");
-
+ /* don't block initalization here due to bad MAC address */
memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);
memcpy(netdev->perm_addr, adapter->hw.mac.addr, netdev->addr_len);
if (!is_valid_ether_addr(netdev->perm_addr)) {
- e_err("Invalid MAC Address: %02x:%02x:%02x:%02x:%02x:%02x\n",
- netdev->perm_addr[0], netdev->perm_addr[1],
- netdev->perm_addr[2], netdev->perm_addr[3],
- netdev->perm_addr[4], netdev->perm_addr[5]);
- err = -EIO;
- goto err_eeprom;
+ DECLARE_MAC_BUF(mac);
+ e_err("Invalid MAC Address: %s\n",
+ print_mac(mac, netdev->perm_addr));
}
init_timer(&adapter->watchdog_timer);
@@ -4640,7 +4694,6 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
err_register:
if (!(adapter->flags & FLAG_HAS_AMT))
e1000_release_hw_control(adapter);
-err_eeprom:
if (!e1000_check_reset_block(&adapter->hw))
e1000_phy_hw_reset(&adapter->hw);
err_hw_init:
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 08/12] e1000e: allow bad checksum
2008-09-30 3:19 ` [RFC PATCH 08/12] e1000e: allow bad checksum Jesse Brandeburg
@ 2008-09-30 8:38 ` Jiri Kosina
0 siblings, 0 replies; 46+ messages in thread
From: Jiri Kosina @ 2008-09-30 8:38 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, Thomas Gleixner,
chris.jones, tim.gardner, airlied
On Mon, 29 Sep 2008, Jesse Brandeburg wrote:
> for (i = 0;; i++) {
> - if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
> + if (e1000_validate_nvm_checksum(hw) >= 0) {
> + /* copy the MAC address out of the NVM */
> + if (e1000e_read_mac_addr(&adapter->hw))
> + e_err("NVM Read Error reading MAC address\n");
> break;
> + }
> if (i == 2) {
> e_err("The NVM Checksum Is Not Valid\n");
> - err = -EIO;
> - goto err_eeprom;
> + e1000e_dump_eeprom(adapter);
> + /*
> + * set MAC address to all zeroes to invalidate and
> + * temporary disable this device for the user. This
> + * blocks regular traffic while still permitting
> + * ethtool ioctls from reaching the hardware as well as
> + * allowing the user to run the interface after
> + * manually setting a hw addr using
> + * `ip link set address`
> + */
> + memset(hw->mac.addr, 0, netdev->addr_len);
> + break;
> }
> }
BTW wouldn't something like
if (e1000_validate_nvm_checksum(hw) >= 0 ||
e1000_validate_nvm_checksum(hw) >= 0) {
/* copy the MAC address out of the NVM */
if (e1000e_read_mac_addr(&adapter->hw))
e_err("NVM Read Error reading MAC address\n");
} else {
e_err("The NVM Checksum Is Not Valid\n");
e1000e_dump_eeprom(adapter);
/*
* set MAC address to all zeroes to invalidate and
* temporary disable this device for the user. This
* blocks regular traffic while still permitting
* ethtool ioctls from reaching the hardware as well as
* allowing the user to run the interface after
* manually setting a hw addr using
* `ip link set address`
*/
memset(hw->mac.addr, 0, netdev->addr_len);
}
just be much more readable? Having for(;;) loop which always performs
three iterations, and having "if" inside that distinguishes two iterations
from each other just looks peculiar to my eyes :)
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH 09/12] e1000e: dump eeprom to dmesg for ich8/9
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (7 preceding siblings ...)
2008-09-30 3:19 ` [RFC PATCH 08/12] e1000e: allow bad checksum Jesse Brandeburg
@ 2008-09-30 3:20 ` Jesse Brandeburg
2008-09-30 3:20 ` [RFC PATCH 10/12] e1000e: Use set_memory_ro()/set_memory_rw() to protect flash memory Jesse Brandeburg
` (2 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:20 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Jesse Brandeburg
dumping the eeprom for now seems like a bit of a verbose
hack, but might be useful when we want to restore it.
if syslogd (or something like) isn't running it won't be kept
however.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/netdev.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 48b0ded..57cead3 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4598,6 +4598,11 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
e1000_eeprom_checks(adapter);
+ /* debug code ... dump the first bytes of the eeprom for
+ * ich parts that might get a corruption */
+ if (adapter->flags & FLAG_IS_ICH)
+ e1000e_dump_eeprom(adapter);
+
/* don't block initalization here due to bad MAC address */
memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);
memcpy(netdev->perm_addr, adapter->hw.mac.addr, netdev->addr_len);
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 10/12] e1000e: Use set_memory_ro()/set_memory_rw() to protect flash memory
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (8 preceding siblings ...)
2008-09-30 3:20 ` [RFC PATCH 09/12] e1000e: dump eeprom to dmesg for ich8/9 Jesse Brandeburg
@ 2008-09-30 3:20 ` Jesse Brandeburg
2008-09-30 3:20 ` [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase Jesse Brandeburg
2008-09-30 3:20 ` [RFC PATCH 12/12] update version Jesse Brandeburg
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:20 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Bruce Allan, Jesse Brandeburg
From: Bruce Allan <bruce.w.allan@intel.com>
A number of users have reported NVM corruption on various ICHx platform
LOMs. One possible reasons for this could be unexpected and/or malicious
writes to the flash memory area mapped into kernel memory. Once the
interface is up, there should be very few reads/writes of the mapped flash
memory. This patch makes use of the x86 set_memory_*() functions to set
the mapped memory read-only and temporarily set it writable only when the
driver needs to write to it. With the memory set read-only, any unexpected
write will be logged with a stack dump indicating the offending code.
Since these LOMs are only on x86 ICHx platforms, it does not matter that
this API is not yet available on other architectures, however it is
dependent on a previous patch that exports these function name symbols.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/e1000.h | 1 +
drivers/net/e1000e/hw.h | 1 +
drivers/net/e1000e/ich8lan.c | 16 ++++++++++++++++
drivers/net/e1000e/netdev.c | 11 +++++++----
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index f80e43a..2a3a311 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -36,6 +36,7 @@
#include <linux/workqueue.h>
#include <linux/io.h>
#include <linux/netdevice.h>
+#include <asm/cacheflush.h>
#include "hw.h"
diff --git a/drivers/net/e1000e/hw.h b/drivers/net/e1000e/hw.h
index 74f263a..dd25009 100644
--- a/drivers/net/e1000e/hw.h
+++ b/drivers/net/e1000e/hw.h
@@ -863,6 +863,7 @@ struct e1000_hw {
u8 __iomem *hw_addr;
u8 __iomem *flash_address;
+ resource_size_t flash_len;
struct e1000_mac_info mac;
struct e1000_fc_info fc;
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 57c6d2f..2b1aa2a 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -176,12 +176,28 @@ static inline u32 __er32flash(struct e1000_hw *hw, unsigned long reg)
static inline void __ew16flash(struct e1000_hw *hw, unsigned long reg, u16 val)
{
+#ifdef _ASM_X86_CACHEFLUSH_H
+ set_memory_rw((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
+#endif
writew(val, hw->flash_address + reg);
+#ifdef _ASM_X86_CACHEFLUSH_H
+ set_memory_ro((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
+#endif
}
static inline void __ew32flash(struct e1000_hw *hw, unsigned long reg, u32 val)
{
+#ifdef _ASM_X86_CACHEFLUSH_H
+ set_memory_rw((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
+#endif
writel(val, hw->flash_address + reg);
+#ifdef _ASM_X86_CACHEFLUSH_H
+ set_memory_ro((unsigned long)hw->flash_address,
+ hw->flash_len >> PAGE_SHIFT);
+#endif
}
#define er16flash(reg) __er16flash(hw, (reg))
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 57cead3..f04de5a 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4402,7 +4402,6 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
struct e1000_hw *hw;
const struct e1000_info *ei = e1000_info_tbl[ent->driver_data];
resource_size_t mmio_start, mmio_len;
- resource_size_t flash_start, flash_len;
static int cards_found;
int i, err, pci_using_dac;
@@ -4472,11 +4471,15 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
if ((adapter->flags & FLAG_HAS_FLASH) &&
(pci_resource_flags(pdev, 1) & IORESOURCE_MEM)) {
- flash_start = pci_resource_start(pdev, 1);
- flash_len = pci_resource_len(pdev, 1);
- adapter->hw.flash_address = ioremap(flash_start, flash_len);
+ adapter->hw.flash_len = pci_resource_len(pdev, 1);
+ adapter->hw.flash_address = ioremap(pci_resource_start(pdev, 1),
+ adapter->hw.flash_len);
if (!adapter->hw.flash_address)
goto err_flashmap;
+#ifdef _ASM_X86_CACHEFLUSH_H
+ set_memory_ro((unsigned long)adapter->hw.flash_address,
+ adapter->hw.flash_len >> PAGE_SHIFT);
+#endif
}
/* construct the net_device struct */
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (9 preceding siblings ...)
2008-09-30 3:20 ` [RFC PATCH 10/12] e1000e: Use set_memory_ro()/set_memory_rw() to protect flash memory Jesse Brandeburg
@ 2008-09-30 3:20 ` Jesse Brandeburg
2008-09-30 12:40 ` Jiri Kosina
2008-09-30 3:20 ` [RFC PATCH 12/12] update version Jesse Brandeburg
11 siblings, 1 reply; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:20 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied, Bruce Allan, Jesse Brandeburg
From: Bruce Allan <bruce.w.allan@intel.com>
Set the hardware to ignore all write/erase cycles to the GbE region in
the ICHx NVM. This feature can be disabled by the WriteProtectNVM module
parameter (enabled by default) though that is not recommended.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/e1000e/e1000.h | 2 ++
drivers/net/e1000e/ethtool.c | 3 +++
drivers/net/e1000e/ich8lan.c | 46 ++++++++++++++++++++++++++++++++++++++++++
drivers/net/e1000e/netdev.c | 8 +++++--
drivers/net/e1000e/param.c | 30 +++++++++++++++++++++++++++
5 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index 2a3a311..6b0eb73 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -307,6 +307,7 @@ struct e1000_info {
#define FLAG_HAS_CTRLEXT_ON_LOAD (1 << 5)
#define FLAG_HAS_SWSM_ON_LOAD (1 << 6)
#define FLAG_HAS_JUMBO_FRAMES (1 << 7)
+#define FLAG_READ_ONLY_NVM (1 << 8)
#define FLAG_IS_ICH (1 << 9)
#define FLAG_HAS_SMART_POWER_DOWN (1 << 11)
#define FLAG_IS_QUAD_PORT_A (1 << 12)
@@ -387,6 +388,7 @@ extern bool e1000e_enable_mng_pass_thru(struct e1000_hw *hw);
extern bool e1000e_get_laa_state_82571(struct e1000_hw *hw);
extern void e1000e_set_laa_state_82571(struct e1000_hw *hw, bool state);
+extern void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw, bool enable);
extern void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw,
bool state);
extern void e1000e_igp3_phy_powerdown_workaround_ich8lan(struct e1000_hw *hw);
diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index f3b49f6..33a3ff1 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -533,6 +533,9 @@ static int e1000_set_eeprom(struct net_device *netdev,
if (eeprom->magic != (adapter->pdev->vendor | (adapter->pdev->device << 16)))
return -EFAULT;
+ if (adapter->flags & FLAG_READ_ONLY_NVM)
+ return -EINVAL;
+
max_len = hw->nvm.word_size * 2;
first_word = eeprom->offset >> 1;
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 2b1aa2a..5e3dac9 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -58,6 +58,7 @@
#define ICH_FLASH_HSFCTL 0x0006
#define ICH_FLASH_FADDR 0x0008
#define ICH_FLASH_FDATA0 0x0010
+#define ICH_FLASH_PR0 0x0074
#define ICH_FLASH_READ_COMMAND_TIMEOUT 500
#define ICH_FLASH_WRITE_COMMAND_TIMEOUT 500
@@ -150,6 +151,19 @@ union ich8_hws_flash_regacc {
u16 regval;
};
+/* ICH Flash Protected Region */
+union ich8_flash_protected_range {
+ struct ich8_pr {
+ u32 base:13; /* 0:12 Protected Range Base */
+ u32 reserved1:2; /* 13:14 Reserved */
+ u32 rpe:1; /* 15 Read Protection Enable */
+ u32 limit:13; /* 16:28 Protected Range Limit */
+ u32 reserved2:2; /* 29:30 Reserved */
+ u32 wpe:1; /* 31 Write Protection Enable */
+ } range;
+ u32 regval;
+};
+
static s32 e1000_setup_link_ich8lan(struct e1000_hw *hw);
static void e1000_clear_hw_cntrs_ich8lan(struct e1000_hw *hw);
static void e1000_initialize_hw_bits_ich8lan(struct e1000_hw *hw);
@@ -1317,6 +1331,7 @@ static s32 e1000_update_nvm_checksum_ich8lan(struct e1000_hw *hw)
* programming failed.
*/
if (ret_val) {
+ /* Possibly read-only, see e1000e_write_protect_nvm_ich8lan() */
hw_dbg(hw, "Flash commit failed.\n");
e1000_release_swflag_ich8lan(hw);
return ret_val;
@@ -1407,6 +1422,37 @@ static s32 e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw)
}
/**
+ * e1000e_write_protect_nvm_ich8lan - Make the NVM read-only
+ * @hw: pointer to the HW structure
+ * @enable: pointer to the HW structure
+ * @enable: TRUE to enable write protection, FALSE to disable write protection
+ *
+ * To prevent malicious write/erase of the NVM, set it to be read-only
+ * so that the hardware ignores all write/erase cycles of the NVM via
+ * the flash control registers. The shadow-ram copy of the NVM will
+ * still be updated, however any updates to this copy will not stick
+ * across driver reloads.
+ **/
+void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw, bool enable)
+{
+ union ich8_flash_protected_range pr0;
+ u32 gfpreg;
+
+ if (hw->nvm.ops.acquire_nvm(hw))
+ return;
+
+ gfpreg = er32flash(ICH_FLASH_GFPREG);
+
+ pr0.regval = er32flash(ICH_FLASH_PR0);
+ pr0.range.base = gfpreg & FLASH_GFPREG_BASE_MASK;
+ pr0.range.limit = ((gfpreg >> 16) & FLASH_GFPREG_BASE_MASK);
+ pr0.range.wpe = enable;
+ ew32flash(ICH_FLASH_PR0, pr0.regval);
+
+ hw->nvm.ops.release_nvm(hw);
+}
+
+/**
* e1000_write_flash_data_ich8lan - Writes bytes to the NVM
* @hw: pointer to the HW structure
* @offset: The offset (in bytes) of the byte/word to read.
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index f04de5a..2626c42 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4508,6 +4508,8 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
adapter->bd_number = cards_found++;
+ e1000e_check_options(adapter);
+
/* setup adapter struct */
err = e1000_sw_init(adapter);
if (err)
@@ -4523,6 +4525,10 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
if (err)
goto err_hw_init;
+ if (adapter->flags & FLAG_IS_ICH)
+ e1000e_write_protect_nvm_ich8lan(&adapter->hw,
+ !!(adapter->flags & FLAG_READ_ONLY_NVM));
+
hw->mac.ops.get_bus_info(&adapter->hw);
adapter->hw.phy.autoneg_wait_to_complete = 0;
@@ -4629,8 +4635,6 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
- e1000e_check_options(adapter);
-
/* Initialize link parameters. User can change them with ethtool */
adapter->hw.mac.autoneg = 1;
adapter->fc_autoneg = 1;
diff --git a/drivers/net/e1000e/param.c b/drivers/net/e1000e/param.c
index ed912e0..d91dbf7 100644
--- a/drivers/net/e1000e/param.c
+++ b/drivers/net/e1000e/param.c
@@ -133,6 +133,15 @@ E1000_PARAM(SmartPowerDownEnable, "Enable PHY smart power down");
*/
E1000_PARAM(KumeranLockLoss, "Enable Kumeran lock loss workaround");
+/*
+ * Write Protect NVM
+ *
+ * Valid Range: 0, 1
+ *
+ * Default Value: 1 (enabled)
+ */
+E1000_PARAM(WriteProtectNVM, "Write-protect NVM [WARNING: disabling this can lead to corrupted NVM]");
+
struct e1000_option {
enum { enable_option, range_option, list_option } type;
const char *name;
@@ -388,4 +397,25 @@ void __devinit e1000e_check_options(struct e1000_adapter *adapter)
opt.def);
}
}
+ { /* Write-protect NVM */
+ const struct e1000_option opt = {
+ .type = enable_option,
+ .name = "Write-protect NVM",
+ .err = "defaulting to Enabled",
+ .def = OPTION_ENABLED
+ };
+
+ if (adapter->flags & FLAG_IS_ICH) {
+ if (num_WriteProtectNVM > bd) {
+ unsigned int write_protect_nvm = WriteProtectNVM[bd];
+ e1000_validate_option(&write_protect_nvm, &opt,
+ adapter);
+ if (write_protect_nvm)
+ adapter->flags |= FLAG_READ_ONLY_NVM;
+ } else {
+ if (opt.def)
+ adapter->flags |= FLAG_READ_ONLY_NVM;
+ }
+ }
+ }
}
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
2008-09-30 3:20 ` [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase Jesse Brandeburg
@ 2008-09-30 12:40 ` Jiri Kosina
2008-09-30 15:47 ` Allan, Bruce W
0 siblings, 1 reply; 46+ messages in thread
From: Jiri Kosina @ 2008-09-30 12:40 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, david.graham,
Bruce Allan, john.ronciak, Thomas Gleixner, chris.jones,
tim.gardner, airlied, Bruce Allan
On Mon, 29 Sep 2008, Jesse Brandeburg wrote:
> Set the hardware to ignore all write/erase cycles to the GbE region in
> the ICHx NVM. This feature can be disabled by the WriteProtectNVM module
> parameter (enabled by default) though that is not recommended.
>
> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
I guess there is no chance to have kernel somehow notified when
write/erase cycle is unsuccessfully tried, is it? This way, it would also
make chasing the root cause easier.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
2008-09-30 12:40 ` Jiri Kosina
@ 2008-09-30 15:47 ` Allan, Bruce W
2008-10-01 13:29 ` Jiri Kosina
0 siblings, 1 reply; 46+ messages in thread
From: Allan, Bruce W @ 2008-09-30 15:47 UTC (permalink / raw)
To: Jiri Kosina, Brandeburg, Jesse
Cc: linux-kernel, linux-netdev, kkeil, agospoda, arjan, Graham,
David, Ronciak, John, Thomas Gleixner, chris.jones, tim.gardner,
airlied
Yeah, we can do that. I need to amend the patch a bit to prevent the protected range lock from being lifted unintentionally and will add some debug statements if/when any write/erase cycles fail.
-----Original Message-----
From: Jiri Kosina [mailto:jkosina@suse.cz]
Sent: Tuesday, September 30, 2008 5:41 AM
To: Brandeburg, Jesse
Cc: linux-kernel@vger.kernel.org; linux-netdev@vger.kernel.org; kkeil@suse.de; agospoda@redhat.com; arjan@linux.intel.com; Graham, David; Allan, Bruce W; Ronciak, John; Thomas Gleixner; chris.jones@canonical.com; tim.gardner@intel.com; airlied@gmail.com; Allan, Bruce W
Subject: Re: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
On Mon, 29 Sep 2008, Jesse Brandeburg wrote:
> Set the hardware to ignore all write/erase cycles to the GbE region in
> the ICHx NVM. This feature can be disabled by the WriteProtectNVM module
> parameter (enabled by default) though that is not recommended.
>
> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
I guess there is no chance to have kernel somehow notified when
write/erase cycle is unsuccessfully tried, is it? This way, it would also
make chasing the root cause easier.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
2008-09-30 15:47 ` Allan, Bruce W
@ 2008-10-01 13:29 ` Jiri Kosina
2008-10-01 19:13 ` Allan, Bruce W
0 siblings, 1 reply; 46+ messages in thread
From: Jiri Kosina @ 2008-10-01 13:29 UTC (permalink / raw)
To: Allan, Bruce W
Cc: Brandeburg, Jesse, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, Graham, David, Ronciak, John, Thomas Gleixner,
chris.jones, tim.gardner, airlied, Olaf Kirch
On Tue, 30 Sep 2008, Allan, Bruce W wrote:
> Yeah, we can do that. I need to amend the patch a bit to prevent the
> protected range lock from being lifted unintentionally and will add some
> debug statements if/when any write/erase cycles fail.
Olaf raised a rather interesting question -- would iAMT be able to access
NVM contents directly, even if the lock bit would be set on the device?
I.e. is iAMT allowed direct access to the EEPROM contents, bypassing
shadow ram mappings?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 46+ messages in thread
* RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase
2008-10-01 13:29 ` Jiri Kosina
@ 2008-10-01 19:13 ` Allan, Bruce W
0 siblings, 0 replies; 46+ messages in thread
From: Allan, Bruce W @ 2008-10-01 19:13 UTC (permalink / raw)
To: Jiri Kosina
Cc: Brandeburg, Jesse, linux-kernel, linux-netdev, kkeil, agospoda,
arjan, Graham, David, Ronciak, John, Thomas Gleixner,
chris.jones, tim.gardner, airlied, Olaf Kirch
On Wednesday, October 01, 2008 6:29 AM, Jiri Kosina wrote:
>
>Olaf raised a rather interesting question -- would iAMT be
>able to access
>NVM contents directly, even if the lock bit would be set on the device?
>I.e. is iAMT allowed direct access to the EEPROM contents, bypassing
>shadow ram mappings?
>
>Thanks,
>
>--
>Jiri Kosina
>SUSE Labs
>
Only write/erase accesses are blocked by hardware after the protected range and lockdown bits are set in this patch; reads are still allowed. I just received confirmation that iAMT does not write to the GbE region of the NVM.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC PATCH 12/12] update version
2008-09-30 3:19 [RFC PATCH 00/12] e1000e debug and protection patches Jesse Brandeburg
` (10 preceding siblings ...)
2008-09-30 3:20 ` [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase Jesse Brandeburg
@ 2008-09-30 3:20 ` Jesse Brandeburg
11 siblings, 0 replies; 46+ messages in thread
From: Jesse Brandeburg @ 2008-09-30 3:20 UTC (permalink / raw)
To: linux-kernel
Cc: linux-netdev, kkeil, agospoda, arjan, david.graham,
bruce.w.allan, jkosina, john.ronciak, tglx, chris.jones,
tim.gardner, airlied
---
drivers/net/e1000e/netdev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 2626c42..df547b6 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -47,7 +47,7 @@
#include "e1000.h"
-#define DRV_VERSION "0.3.3.3-k2"
+#define DRV_VERSION "0.3.3.3-kt"
char e1000e_driver_name[] = "e1000e";
const char e1000e_driver_version[] = DRV_VERSION;
^ permalink raw reply related [flat|nested] 46+ messages in thread