login
Search
please be warned this is a test series of patches that we are currently
validating, but we wanted to post them sooner than later. this series of
patches implements:
1-2) base kernel code fixes that might be helpful for debugging
3-6) a series of patches to fix some (possible) buglets in eeprom access code
7) a patch that triggered the bugs that were fixed in 3-6, from Thomas
8-9) (what should be) debug helper patches for testers to log to syslog the
first bytes of the eeprom, that are checksummed at least.
10) protect iomapped memory from write by kernel processes
11) write protect the actual e1000e flash space, add module param to disable[1]
12) update the version so we can tell we're running with this test driver.
# This series applies on GIT commit eaf9e45100b56c18a2236464cadd92d9a3d399ea
Tomorrow we should be able to test and post the eeprom restore patches for
the driver.
[1] we will follow up tomorrow with a patch that locks the actual flash AND
the bits used by 11) that requires a full chip reset to unlock, as
we see a possible way that the iomapped registers could still be poked
in such a way to unlock the flash space.
I will probably update these again but they seem to work for now.
---
Bruce Allan (3):
e1000e: write protect ICHx NVM to prevent malicious write/erase
e1000e: Use set_memory_ro()/set_memory_rw() to protect flash memory
x86: export set_memory_ro and set_memory_rw
Jesse Barnes (1):
pci sysfs warning
Jesse Brandeburg (7):
update version
e1000e: dump eeprom to dmesg for ich8/9
e1000e: allow bad checksum
e1000e: drop stats lock
e1000e: fix lockdep issues
e1000e: do not ever sleep in interrupt context
e1000e: reset swflag after resetting hardware
Thomas Gleixner (1):
e1000e: debug contention on NVM SWFLAG
arch/x86/mm/pageattr.c | 2 +
drivers/net/e1000e/e1000.h | 6 +-
drivers/net/e1000e/ethtool.c | 9 ++
...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 + ...Ping DaveM. Does this look ok? What else would we need for you to remove your range checking from sparc? Thanks, Jesse -- Jesse Barnes, Intel Open Source Technology Center --
From: Jesse Barnes <jbarnes@virtuousgeek.org> 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 --
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 --
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 we plan to keep on working on this until we understand what is going on. --
On Fri, Oct 3, 2008 at 4:28 PM, Jesse Brandeburg I removed the bad addresses from the cc: list --
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 --
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 --
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. --
The WARN_ON triggers with current mainline. Is there any fixlet in 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 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 --
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 --
On Sun, 5 Oct 2008 17:55:14 +0200 (CEST) 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 --
On Sun, 5 Oct 2008 18:16:29 +0200 (CEST) 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 --
From: Jesse Barnes <jbarnes@virtuousgeek.org> I can't do that until you add similar checking to the other pci_mmap_page_range() call site in drivers/pci/proc.c --
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.
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)
{
--
applied to tip/x86/exports, thanks Jesse. Ingo --
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) {
/*
--
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 & ...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 ...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
- */
-
...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 ...
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 --
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 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) --
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 That may help, yes. Olaf -- Neo didn't bring down the Matrix. SOA did. --soafacts.com --
Had Karsten the mutex patch applied or not ?
tglx
--
That was openSuse 11.1 without any patches Olaf -- Neo didn't bring down the Matrix. SOA did. --soafacts.com --
I should have said 11.1 beta1. Olaf -- Neo didn't bring down the Matrix. SOA did. --soafacts.com --
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 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 --
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 ...
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
--
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
+++ ...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 ...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
--
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); --
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 ...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 --
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 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 --
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. --
--- 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; --
