[RFC PATCH 00/12] e1000e debug and protection patches

[RFC PATCH 00/12] e1000e debug and protection patches

[Jesse Brandeburg]

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 ++
 drivers/net/e1000e/hw.h      |    1 
 drivers/net/e1000e/ich8lan.c |   82 +++++++++++++++++++++
 drivers/net/e1000e/netdev.c  |  167 ++++++++++++++++++++++++++++--------------
 drivers/net/e1000e/param.c   |   30 ++++++++
 drivers/pci/pci-sysfs.c      |   14 ++++
 8 files changed, 255 insertions(+), 56 deletions(-)

-- 
Signature

[RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw

[Jesse Brandeburg]

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)
 {

[RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[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.

[RFC PATCH 03/12] e1000e: reset swflag after resetting hardware

[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) {
 		/*

[RFC PATCH 04/12] e1000e: do not ever sleep in interrupt context

[Jesse Brandeburg]

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);
 

[RFC PATCH 05/12] e1000e: fix lockdep issues

[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) &&

[RFC PATCH 06/12] e1000e: drop stats lock

[Jesse Brandeburg]

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)

[RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[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);
 }
 
 /**

[RFC PATCH 08/12] e1000e: allow bad checksum

[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:

[RFC PATCH 09/12] e1000e: dump eeprom to dmesg for ich8/9

[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);

[RFC PATCH 10/12] e1000e: Use set_memory_ro()/set_memory_rw() to protect flash memory

[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 */

[RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase

[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;
+			}
+		}
+	}
 }

[RFC PATCH 12/12] update version

[Jesse Brandeburg]

---

 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;
 

Re: [RFC PATCH 01/12] x86: export set_memory_ro and set_memory_rw

[Ingo Molnar]

* 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

Re: [RFC PATCH 08/12] e1000e: allow bad checksum

[Jiri Kosina]

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

Re: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase

[Jiri Kosina]

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

RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase

[Allan, Bruce W]

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

RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase

[Jiri Kosina]

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

RE: [RFC PATCH 11/12] e1000e: write protect ICHx NVM to prevent malicious write/erase

[Allan, Bruce W]

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.

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Jiri Kosina]

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

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Olaf Kirch]

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

RE: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Brandeburg, Jesse]

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)

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Olaf Kirch]

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

RE: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Thomas Gleixner]

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

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Thomas Gleixner]

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

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Olaf Kirch]

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

Re: [RFC PATCH 07/12] e1000e: debug contention on NVM SWFLAG

[Olaf Kirch]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jesse Barnes]

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

[PATCH] e1000e: prevent concurrent access to NVRAM

[Thomas Gleixner]

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);
 }
 
 /**

Re: [PATCH] e1000e: prevent concurrent access to NVRAM

[Jesse Brandeburg]

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.

Re: [PATCH] e1000e: prevent concurrent access to NVRAM

[Thomas Gleixner]

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
	

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[David Miller]

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.

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jesse Barnes]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jiri Kosina]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jesse Brandeburg]

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.

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jesse Brandeburg]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jiri Kosina]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Thomas Gleixner]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Jesse Brandeburg]

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.

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Thomas Gleixner]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Arjan van de Ven]

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)

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Thomas Gleixner]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Arjan van de Ven]

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.

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Thomas Gleixner]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[Arjan van de Ven]

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

Re: [RFC PATCH 02/12] On Tue, 23 Sep 2008, David Miller wrote:

[David Miller]

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