x86: check for and defend against BIOS memory corruption

+ while(addr < 0x10000 && num_scan_areas < MAX_SCAN_AREAS) {> + u64 size;> + addr = find_e820_area_size(addr, &size, PAGE_SIZE);> +> + if (addr == 0)> + break;> +> + if ((addr + size) > 0x10000)> + size = 0x10000 - addr;> +> + if (size == 0)> + break;> +> + e820_update_range(addr, size, E820_RAM, E820_RESERVED);> + scan_areas[num_scan_areas].addr = addr;> + scan_areas[num_scan_areas].size = size;> + num_scan_areas++;> +> + /* Assume we've already mapped this early memory */> + memset(__va(addr), 0, size);> +> + addr += size;> + }> +> + printk(KERN_INFO "scanning %d areas for BIOS corruption\n",> + num_scan_areas);> + update_e820();> +}> +> +static int __read_mostly bios_corruption_check = 1;

Quoted 74 lines · Jeremy Fitzhardinge

Some BIOSes have been observed to corrupt memory in the low 64k. This patch does two things:
- Reserves all memory which does not have to be in that area, to prevent it from being used as general memory by the kernel. Things like the SMP trampoline are still in the memory, however.
- Clears the reserved memory so we can observe changes to it.
- Adds a function check_for_bios_corruption() which checks and reports on memory becoming unexpectedly non-zero. Currently it's called in the x86 fault handler, and the powermanagement debug output.

RFC: What other places should we check for corruption in?

[ Alan, Rafał: could you check you see:
1: corruption messages
2: no crashes
Thanks -J
]

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Alan Jenkins <[email protected]>

Cc Hugh Dickens <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Rafał Miłecki <[email protected]>

Cc H. Peter Anvin <[email protected]>

---
 Documentation/kernel-parameters.txt |    5 ++
 arch/x86/Kconfig                    |    3 +
 arch/x86/kernel/setup.c             |   86 +++++++++++++++++++++++++++++++++++
 arch/x86/mm/fault.c                 |    2
 drivers/base/power/main.c           |    1
 include/linux/kernel.h              |   12 ++++
 6 files changed, 109 insertions(+)

===================================================================

Documentation/kernel-parameters.txt +5 -0

@@ -359,6 +359,11 @@
                       BayCom Serial Port AX.25 Modem (Half Duplex Mode)
                       Format: <io>,<irq>,<mode>
                       See header of drivers/net/hamradio/baycom_ser_hdx.c.
+
+       bios_corruption_check=0/1 [X86]
+                       Some BIOSes seem to corrupt the first 64k of memory
+                       when doing things like suspend/resume.  Setting this
+                       option will scan the memory looking for corruption.
 
       boot_delay=     Milliseconds to delay each printk during boot.
                       Values larger than 10 seconds (10000) are changed to

===================================================================

arch/x86/Kconfig +3 -0

@@ -203,6 +203,9 @@
       bool
       depends on X86_SMP || (X86_VOYAGER && SMP) || (64BIT && ACPI_SLEEP)
       default y
+
+config X86_CHECK_BIOS_CORRUPTION
+        def_bool y
 
 config KTIME_SCALAR
       def_bool X86_32

===================================================================

arch/x86/kernel/setup.c +5 -0

@@ -582,6 +582,88 @@
 struct x86_quirks *x86_quirks __initdata = &default_x86_quirks;
 
 /*
+ * Some BIOSes seem to corrupt the low 64k of memory during events
+ * like suspend/resume and unplugging an HDMI cable.  Reserve all
+ * remaining free memory in that area and fill it with a distinct
+ * pattern.
+ */

arch/x86/kernel/setup.c line 599

Quoted 10 lines

arch/x86/kernel/setup.c +9 -0

+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+#define MAX_SCAN_AREAS 8
+static struct e820entry scan_areas[MAX_SCAN_AREAS];
+static int num_scan_areas;
+
+static void __init setup_bios_corruption_check(void)
+{
+       u64 addr = PAGE_SIZE;   /* assume first page is reserved anyway */
+
 

can you please not punish systems without this bios problem?

if (!bios_corruption_check)
return;

Quoted 1 line

Quoted 1 line

- Clears the reserved memory so we can observe changes to it.

Yeah, OK, but I think it should default to ON for now. The problem is that we had two very different systems (Sony Vaio and Intel desktop)

I understand that disabling chunk of RAM on every, even not affected, machine is far from perfect, but we have to remember about not advanced end-users.

Out of current discussion I tried using s2ram on patched kernel (I did not try s2ram earlier, my problem was (un)plugging HDMI - some ACPI code probably).

Corruption output is quite huge, I attached it to bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=11237
http://bugzilla.kernel.org/attachment.cgi?id=17526

2008/8/29 Hugh Dickins <[email protected]>:

Quoted 30 lines

Quoted 10 lines · Rafał Miłecki

Out of current discussion I tried using s2ram on patched kernel (I did not try s2ram earlier, my problem was (un)plugging HDMI - some ACPI code probably).

Corruption output is quite huge, I attached it to bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=11237
http://bugzilla.kernel.org/attachment.cgi?id=17526

Not quite the output we were expecting! I've not got around to trying it yet, so beware, but I think Jeremy's patch needs the following on top. Or you may prefer to wait until one of us reports that it is now working as intended.

arch/x86/kernel/setup.c 2008-08-29 11:19:24.000000000 +0100 renamed from arch/x86/kernel/setup.c 2008-08-29 11:17:16.000000000 +0100 +2 -1

@@ -636,11 +636,12 @@ void check_for_bios_corruption(void)
               unsigned long *addr = __va(scan_areas[i].addr);
               unsigned long size = scan_areas[i].size;
 
-               for(; size; addr++, size--) {
+               for(; size; addr++, size -= sizeof(unsigned long)) {
                       if (!*addr)
                               continue;
                       printk(KERN_ERR "Corrupted low memory at %p (%lx phys) = %08lx\n",
                              addr, __pa(addr), *addr);
+                       *addr = 0;
                       corruption = 1;
               }

arch/x86/kernel/setup.c 2008-08-29 11:19:24.000000000 +0100 void check_for_bios_corruption(void) · line 647

Quoted 2 lines

arch/x86/kernel/setup.c 2008-08-29 11:19:24.000000000 +0100 renamed from arch/x86/kernel/setup.c 2008-08-29 11:17:16.000000000 +0100 +0 -0

       }

I tried your patch anyway (after applying Jeremy's patch of course) and it doesn't seem to work. The only output is:
scanning 2 areas for BIOS corruption
after using s2ram. I do not get any
Corrupted low memory at*

Quoted 7 lines · Rafał Miłecki

I tried your patch anyway (after applying Jeremy's patch of course) and it doesn't seem to work. The only output is:
scanning 2 areas for BIOS corruption
after using s2ram. I do not get any
Corrupted low memory at*

It seemed to work for me. Did you remember to plug HDMI to trigger the corruption before you used s2ram?

Quoted 16 lines

2008/8/29 Rafał Miłecki <[email protected]>:
2008/8/29 Hugh Dickins <[email protected]>:

Quoted 6 lines

Here's my version of Jeremy's patch, that I've now tested on my machines, as x86_32 and as x86_64. It addresses none of the points Alan Cox made, and it stays silent for me, even after suspend+resume, unless I actually introduce corruption myself. Omits Jeremy's check in fault.c, but does a check every minute, so should soon detect Rafał's HDMI corruption without any need to suspend+resume.

Your periodic test works fine:

Corrupted low memory at ffff88000000be9c (be9c phys) = b02a0004

 <IRQ>  [<ffffffff8020fc9b>] check_for_bios_corruption+0x93/0x9f
 [<ffffffff8020fca7>] ? periodic_check_for_corruption+0x0/0x25
 [<ffffffff8020fcb0>] periodic_check_for_corruption+0x9/0x25

Quoted 2 lines

By the way I confirmed this bug on Sony Vaio FW11M (my one is FW11S). Probably more machines from FW11* are affected.

If this patch is known to work fine for Sony Vaio FW* and Alan's machine, could it go mainline somehow?

Well.

Thanks for the prod, and I'm certainly remiss for not following up sooner. But I'm really not at all keen on such a patch going into mainline myself.

It's an interesting experiment, and I'd be happy to see such a patch (adjusted to make sure output goes to kerneloops.org) spending a little while in Fedora Rawhide (who'd be the right contact for that?).

But so far as mainline goes, I share Alan Cox's opinion that we should not be chopping pages out of every x86 user's memory, just because a couple of machines with faulty BIOSes have been observed.

Particularly now it's evident that the 64kB "limit" is no more than a reflection of where the directmap pagetable changes have caught such corruption.

If lots more such corruptions are reported, of course I would change my position; but those bad directmap PMD crashes are themselves quite recognizable now we know to look out for them.

I would prefer you both to use the minimal memmap= solutions for now; but others may disagree.

Quoted 2 lines

I would prefer you both to use the minimal memmap= solutions for now; but others may disagree.

The fact that we're seeing this problem in two completely different systems with different BIOSes and everything else makes me worried that this is quite widespread. It's only the persistence and diligence of our bug reporters that we managed to work out that they're the same problem. How many other people are getting strange crashes and haven't managed to correlate it any particular BIOS interaction? Or just happen to be corrupting memory we don't care about right now, but is only a small code change or link order change away from disaster?

Not quite the output we were expecting! I've not got around to trying it yet, so beware, but I think Jeremy's patch needs the following on top. Or you may prefer to wait until one of us reports that it is now working as intended.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- Clears the reserved memory so we can observe changes to it.

You ought to pick different values on different runs 0x00, 0xFF, 0xAA, 0x55 etc...

Quoted 3 lines

Yeah, OK, but I think it should default to ON for now. The problem is that we had two very different systems (Sony Vaio and Intel desktop)

So a zillion people should lose a chunk of RAM because of what is probably an obscure bug in a single version of a piece of SMM code on two systems total ?

That appears to be totally out of proportion - plus the defaults are *irrelevant* for mass user coverage. If you want large scale coverage ask the Fedora, OpenSuSE and Ubuntu people to turn it on for a testing kernel release and report back.

Quoted 1 line · Yinghai Lu

On Thu, Aug 28, 2008 at 12:52 PM, Jeremy Fitzhardinge <[email protected]> wrote:

Yeah, OK, but I think it should default to ON for now. The problem is that we had two very different systems (Sony Vaio and Intel desktop) exhibit the same problem in two different ways. These systems worked

fine until we slightly changed the way that pagetable memory was allocated. We just don't know how many other systems are doing this kind of subtle corruption of low memory.

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 2 lines · Yinghai Lu

On Thu, Aug 28, 2008 at 12:52 PM, Jeremy Fitzhardinge <[email protected]> wrote:

<snip>

Quoted 4 lines

Yeah, OK, but I think it should default to ON for now. The problem is that we had two very different systems (Sony Vaio and Intel desktop) exhibit the same problem in two different ways. These systems worked

This is very interresting. I have a gigabyte X48 board, and everything works perfect, however, in memtest86+ i get thousands of errors in the range 0-0.9mb. I am certain my ram is fine, i've tried it in other computers, and reversed. I've run dozens of stress tests within my booted linux, no trouble, also the userspace memtester (allthough im aware it wont actually ever get to grab offending addresses).

Any thoughts?

2008/8/28 Jeremy Fitzhardinge <[email protected]>:

Quoted 10 lines

Some BIOSes have been observed to corrupt memory in the low 64k. This patch does two things:
- Reserves all memory which does not have to be in that area, to prevent it from being used as general memory by the kernel. Things like the SMP trampoline are still in the memory, however.
- Clears the reserved memory so we can observe changes to it.
- Adds a function check_for_bios_corruption() which checks and reports on memory becoming unexpectedly non-zero. Currently it's called in the x86 fault handler, and the powermanagement debug output.

Quoted 7 lines

RFC: What other places should we check for corruption in?

[ Alan, Rafał: could you check you see:
1: corruption messages
2: no crashes
Thanks -J
]

I was trying my best to crash system with this patch applied and failed :)

Works great.

Just wonder if I should expect any printk from
check_for_bios_corruption? I do not see any:

zajec@sony:~> dmesg | grep -i corr
scanning 2 areas for BIOS corruption

Quoted 24 lines · Rafał Miłecki

2008/8/28 Jeremy Fitzhardinge <[email protected]>:

Quoted 10 lines

Some BIOSes have been observed to corrupt memory in the low 64k. This patch does two things:
- Reserves all memory which does not have to be in that area, to prevent it from being used as general memory by the kernel. Things like the SMP trampoline are still in the memory, however.
- Clears the reserved memory so we can observe changes to it.
- Adds a function check_for_bios_corruption() which checks and reports on memory becoming unexpectedly non-zero. Currently it's called in the x86 fault handler, and the powermanagement debug output.

Quoted 8 lines

RFC: What other places should we check for corruption in?

[ Alan, Rafał: could you check you see:
1: corruption messages
2: no crashes
Thanks -J
]

I was trying my best to crash system with this patch applied and failed :)

Works great.

Quoted 6 lines

Just wonder if I should expect any printk from
check_for_bios_corruption? I do not see any:

zajec@sony:~> dmesg | grep -i corr
scanning 2 areas for BIOS corruption

that's _very_ weird.

maybe the BIOS expects _zeroes_ somewhere? Do you suddenly see crashes if you change this line in Jeremy's patch:

+ memset(__va(addr), 0, size);

to something like:

+ memset(__va(addr), 0x55, size);

If this does not tickle any messages either, then maybe the problem is in the identity of the entities we allocate in the first 64K. Is there a list of allocations that go there when Jeremy's patch is not applied?

but ... i think with an earlier patch you saw corruption, right? Far-fetched idea: maybe it's some CPU erratum during suspend/resume that corrupts pagetables if the pagetables are allocated in the first 64K of RAM? In that case we should use a bootmem allocation for pagetables that give a minimum address of 64K.

Rafał's corruption was definitely non-zero. I think the corruption is happening, but it's just not reported.

Quoted 3 lines · Jeremy Fitzhardinge

Rafał's corruption was definitely non-zero. I think the corruption is happening, but it's just not reported.

i thought we did a check after coming back from resume? That would be a natural place to do the check. (the corruption is suspend/resume related after all, right?)

I was trying my best to crash system with this patch applied and failed :)

Works great.

Just wonder if I should expect any printk from
check_for_bios_corruption? I do not see any:

zajec@sony:~> dmesg | grep -i corr
scanning 2 areas for BIOS corruption

Some BIOSes have been observed to corrupt memory in the low 64k.

This patch does two things:
- Reserves all memory which does not have to be in that area, to prevent it from being used as general memory by the kernel. Things like the SMP trampoline are still in the memory, however.
- Clears the reserved memory so we can observe changes to it.
- Adds a function check_for_bios_corruption() which checks and reports on memory becoming unexpectedly non-zero. Currently it's called in the x86 fault handler, and the powermanagement debug output.

RFC: What other places should we check for corruption in?

[ Alan, Rafał: could you check you see:
1: corruption messages
2: no crashes
Thanks -J
]

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Alan Jenkins <[email protected]>

Cc Hugh Dickens <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Rafał Miłecki <[email protected]>

Cc H. Peter Anvin <[email protected]>

---
 Documentation/kernel-parameters.txt |    5 ++
 arch/x86/Kconfig                    |    3 +
 arch/x86/kernel/setup.c             |   86 +++++++++++++++++++++++++++++++++++
 arch/x86/mm/fault.c                 |    2 
 drivers/base/power/main.c           |    1 
 include/linux/kernel.h              |   12 ++++
 6 files changed, 109 insertions(+)

===================================================================

arch/x86/Kconfig +0 -0

 config KTIME_SCALAR
 	def_bool X86_32

===================================================================

arch/x86/kernel/setup.c +13 -0

@@ -582,6 +582,88 @@
 struct x86_quirks *x86_quirks __initdata = &default_x86_quirks;
 
 /*
+ * Some BIOSes seem to corrupt the low 64k of memory during events
+ * like suspend/resume and unplugging an HDMI cable.  Reserve all
+ * remaining free memory in that area and fill it with a distinct
+ * pattern.
+ */
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+#define MAX_SCAN_AREAS	8
+static struct e820entry scan_areas[MAX_SCAN_AREAS];
+static int num_scan_areas;
+
+static void __init setup_bios_corruption_check(void)
+{
+	u64 addr = PAGE_SIZE;	/* assume first page is reserved anyway */

arch/x86/kernel/setup.c +22 -0

+
+		if (size == 0)
+			break;
+
+		e820_update_range(addr, size, E820_RAM, E820_RESERVED);
+		scan_areas[num_scan_areas].addr = addr;
+		scan_areas[num_scan_areas].size = size;
+		num_scan_areas++;
+
+		/* Assume we've already mapped this early memory */
+		memset(__va(addr), 0, size);
+
+		addr += size;
+	}
+
+	printk(KERN_INFO "scanning %d areas for BIOS corruption\n",
+	       num_scan_areas);
+	update_e820();
+}
+
+static int __read_mostly bios_corruption_check = 1;
+

arch/x86/kernel/setup.c +18 -0

+}
+
+static int set_bios_corruption_check(char *arg)
+{
+	char *end;
+
+	bios_corruption_check = simple_strtol(arg, &end, 10);
+
+	return (*end == 0) ? 0 : -EINVAL;
+}
+early_param("bios_corruption_check", set_bios_corruption_check);
+#endif
+
+/*
  * Determine if we were loaded by an EFI loader.  If so, then we have also been
  * passed the efi memmap, systab, etc., so we should use these data structures
  * for initialization.  Note, the efi init code path is determined by the

@@ -766,6 +848,10 @@
 		max_low_pfn = max_pfn;
 
 	high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
+#endif
+
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+	setup_bios_corruption_check();
 #endif
 
 	/* max_pfn_mapped is updated here */

===================================================================

arch/x86/mm/fault.c +2 -0

@@ -864,6 +864,8 @@
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.
  */
+	check_for_bios_corruption();
+
 #ifdef CONFIG_X86_32
 	bust_spinlocks(1);
 #else

===================================================================

drivers/base/power/main.c +1 -0

@@ -254,6 +254,7 @@
 
 static void pm_dev_dbg(struct device *dev, pm_message_t state, char *info)
 {
+	check_for_bios_corruption();
 	dev_dbg(dev, "%s%s%s\n", info, pm_verb(state.event),
 		((state.event & PM_EVENT_SLEEP) && device_may_wakeup(dev)) ?
 		", may wakeup" : "");

===================================================================

include/linux/kernel.h +5 -0

@@ -246,6 +246,18 @@
 extern void add_taint(unsigned);
 extern int root_mountflags;
 
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+/*
+ * This is obviously not a great place for this, but we want to be
+ * able to scatter it around anywhere in the kernel.
+ */

Thanks for taking this on, Jeremy: I had too many doubts to do so.

Quoted 2 lines · Jeremy Fitzhardinge

Some BIOSes have been observed to corrupt memory in the low 64k.

hpa introduced the 64k idea, and we've all been repeating it; but I've not heard the reasoning behind it. Is it a fundamental addressing limitation within the BIOS memory model? Or a case that Windows treats the bottom 64k as scratch, so BIOS testers won't notice if they corrupt it?

The two instances of corruption we've been studying have indeed been below 64k (one in page 8 and one in page 11), but that's because they were both recognizable corruptions of direct map PMDs.

If there is not a very strong justification for that 64k limit, then I don't think this approach will be very useful, and we should simply continue to rely on analyzing corruption when it appears, and recommend memmap= as a way of avoiding it once analyzed. If there is a strong justification for it, please dispel my ignorance!

Quoted 9 lines

This patch does two things:
- Reserves all memory which does not have to be in that area, to prevent it from being used as general memory by the kernel. Things like the SMP trampoline are still in the memory, however.
- Clears the reserved memory so we can observe changes to it.
- Adds a function check_for_bios_corruption() which checks and reports on memory becoming unexpectedly non-zero. Currently it's called in the x86 fault handler, and the powermanagement debug output.

Quoted 2 lines

RFC: What other places should we check for corruption in?

I don't know: the easy answer would be just to do it once every minute. As the patch stands, we'll only learn more on machines going through suspend+resume (disk or ram). If the corruption avoidance works, then the fault case should never trigger.

Quoted 10 lines

[ Alan, Rafał: could you check you see:
1: corruption messages
2: no crashes
Thanks -J
]

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Alan Jenkins <[email protected]>

Cc Hugh Dickens <[email protected]>

Dickins

Quoted 23 lines

Cc Ingo Molnar <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Rafał Miłecki <[email protected]>

Cc H. Peter Anvin <[email protected]>

---
 Documentation/kernel-parameters.txt |    5 ++
 arch/x86/Kconfig                    |    3 +
 arch/x86/kernel/setup.c             |   86 +++++++++++++++++++++++++++++++++++
 arch/x86/mm/fault.c                 |    2 
 drivers/base/power/main.c           |    1 
 include/linux/kernel.h              |   12 ++++
 6 files changed, 109 insertions(+)

===================================================================

Documentation/kernel-parameters.txt +3 -0

@@ -359,6 +359,11 @@
 			BayCom Serial Port AX.25 Modem (Half Duplex Mode)
 			Format: <io>,<irq>,<mode>
 			See header of drivers/net/hamradio/baycom_ser_hdx.c.
+
+	bios_corruption_check=0/1 [X86]
+			Some BIOSes seem to corrupt the first 64k of memory

Documentation/kernel-parameters.txt line 367

Quoted 3 lines

Documentation/kernel-parameters.txt +2 -0

+			when doing things like suspend/resume.  Setting this
+			option will scan the memory looking for corruption.
 

It's actually a bottom_of_memory corruption_check: it would pick up corruption there whether it's caused by the BIOS or not. The boot parameter description ought to refer to the config
option: ah, the config option is always on for x86, hmm.

If the 64k is in any doubt, maybe the corruption_check boot option should specify limiting address rather than just off/on.

Quoted 10 lines

Documentation/kernel-parameters.txt +0 -0

 	boot_delay=	Milliseconds to delay each printk during boot.
 			Values larger than 10 seconds (10000) are changed to

===================================================================

arch/x86/Kconfig +0 -0

@@ -203,6 +203,9 @@
 	bool
 	depends on X86_SMP || (X86_VOYAGER && SMP) || (64BIT && ACPI_SLEEP)
 	default y

arch/x86/Kconfig line 209

Quoted 4 lines

arch/x86/Kconfig +3 -0

+
+config X86_CHECK_BIOS_CORRUPTION
+        def_bool y
 

Always on? For the moment, perhaps. I'm very pleased to see you've set it on for x86_32 as well as for x86_64, I'd been wanting to ask what's been happening on 32-bit.

Quoted 28 lines

arch/x86/Kconfig +0 -0

 config KTIME_SCALAR
 	def_bool X86_32

===================================================================

arch/x86/kernel/setup.c +18 -0

@@ -582,6 +582,88 @@
 struct x86_quirks *x86_quirks __initdata = &default_x86_quirks;
 
 /*
+ * Some BIOSes seem to corrupt the low 64k of memory during events
+ * like suspend/resume and unplugging an HDMI cable.  Reserve all
+ * remaining free memory in that area and fill it with a distinct
+ * pattern.
+ */
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+#define MAX_SCAN_AREAS	8
+static struct e820entry scan_areas[MAX_SCAN_AREAS];
+static int num_scan_areas;
+
+static void __init setup_bios_corruption_check(void)
+{
+	u64 addr = PAGE_SIZE;	/* assume first page is reserved anyway */
+
+	while(addr < 0x10000 && num_scan_areas < MAX_SCAN_AREAS) {
+		u64 size;
+		addr = find_e820_area_size(addr, &size, PAGE_SIZE);
+

arch/x86/kernel/setup.c

Quoted 6 lines

arch/x86/kernel/setup.c +5 -0

+		if (addr == 0)
+			break;
+
+		if ((addr + size) > 0x10000)
+			size = 0x10000 - addr;
 

Here (and the patch description) might be the right place to justify 64k.

Quoted 29 lines

arch/x86/kernel/setup.c +29 -0

+
+		if (size == 0)
+			break;
+
+		e820_update_range(addr, size, E820_RAM, E820_RESERVED);
+		scan_areas[num_scan_areas].addr = addr;
+		scan_areas[num_scan_areas].size = size;
+		num_scan_areas++;
+
+		/* Assume we've already mapped this early memory */
+		memset(__va(addr), 0, size);
+
+		addr += size;
+	}
+
+	printk(KERN_INFO "scanning %d areas for BIOS corruption\n",
+	       num_scan_areas);
+	update_e820();
+}
+
+static int __read_mostly bios_corruption_check = 1;
+
+void check_for_bios_corruption(void)
+{
+	int i;
+	int corruption = 0;
+
+	if (!bios_corruption_check)
+		return;

arch/x86/kernel/setup.c

Quoted 4 lines

arch/x86/kernel/setup.c +3 -0

+
+	for(i = 0; i < num_scan_areas; i++) {
+		unsigned long *addr = __va(scan_areas[i].addr);
 

Small point, but since we're doing the same on 32-bit and 64-bit, I think it would be better to operate on unsigned ints, to get the same kind of printout in the two cases.

arch/x86/kernel/setup.c

Quoted 8 lines

arch/x86/kernel/setup.c +7 -0

+		unsigned long size = scan_areas[i].size;
+
+		for(; size; addr++, size--) {
+			if (!*addr)
+				continue;
+			printk(KERN_ERR "Corrupted low memory at %p (%lx phys) = %08lx\n",
+			       addr, __pa(addr), *addr);
 

Don't you need to reset *addr to 0 here, to avoid noise ever after?

Quoted 5 lines

arch/x86/kernel/setup.c +5 -0

+			corruption = 1;
+		}
+	}
+
+	if (corruption)

arch/x86/kernel/setup.c line 652

Quoted 2 lines

arch/x86/kernel/setup.c +1 -0

+		dump_stack();

The purpose of the dump_stack being to insert a recognizable and irritating spew into the log, prompting people to report these cases:
the stacktrace itself is unlikely to be relevant. Is a simple dump_stack enough to get it reported to kerneloops.org, or is more tweaking necessary?

Quoted 64 lines

arch/x86/kernel/setup.c +14 -0

+}
+
+static int set_bios_corruption_check(char *arg)
+{
+	char *end;
+
+	bios_corruption_check = simple_strtol(arg, &end, 10);
+
+	return (*end == 0) ? 0 : -EINVAL;
+}
+early_param("bios_corruption_check", set_bios_corruption_check);
+#endif
+
+/*

* Determine if we were loaded by an EFI loader. If so, then we have also been
* passed the efi memmap, systab, etc., so we should use these data structures
* for initialization. Note, the efi init code path is determined by the

arch/x86/kernel/setup.c +4 -0

@@ -766,6 +848,10 @@
 		max_low_pfn = max_pfn;
 
 	high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
+#endif
+
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+	setup_bios_corruption_check();
 #endif
 
 	/* max_pfn_mapped is updated here */

===================================================================

arch/x86/mm/fault.c +2 -0

@@ -864,6 +864,8 @@
  * Oops. The kernel tried to access some bad page. We'll have to
  * terminate things with extreme prejudice.
  */
+	check_for_bios_corruption();
+
 #ifdef CONFIG_X86_32
 	bust_spinlocks(1);
 #else

===================================================================

drivers/base/power/main.c +1 -0

@@ -254,6 +254,7 @@
 
 static void pm_dev_dbg(struct device *dev, pm_message_t state, char *info)
 {
+	check_for_bios_corruption();
 	dev_dbg(dev, "%s%s%s\n", info, pm_verb(state.event),
 		((state.event & PM_EVENT_SLEEP) && device_may_wakeup(dev)) ?
 		", may wakeup" : "");

===================================================================

include/linux/kernel.h +5 -0

@@ -246,6 +246,18 @@
 extern void add_taint(unsigned);
 extern int root_mountflags;
 
+#ifdef CONFIG_X86_CHECK_BIOS_CORRUPTION
+/*
+ * This is obviously not a great place for this, but we want to be
+ * able to scatter it around anywhere in the kernel.
+ */

include/linux/kernel.h line 263

Quoted 10 lines

include/linux/kernel.h +7 -0

+void check_for_bios_corruption(void);
+#else
+static inline void check_for_bios_corruption(void)
+{
+}
+#endif
+
 /* Values used for system_state */
 extern enum system_states {
 	SYSTEM_BOOTING,

I'll give this or something like it a try on my machines later on, 32 and 64, to see if anything comes up which hasn't caused any visible problem before (but will either need to do suspend+resume on machines not tried before, or add in the periodic test).

I do wonder whether for 2.6.27 we should simply go back to the previous kernel pagetable layout, so there's no regression while we investigate the issue further. But I don't recall how well- defined that layout was, and whether your perturbation was just in that one patch or not.

Is this the right moment for me to mention again that I'm not sure your reuse of existing pagetables was quite right anyway: NX being excluded from level2_ident_pgt, but wanted in the direct map?

It would be easy to add another kernel parameter to select how much memory to reserve for corruption checking. 64k? 1M?

Quoted 3 lines · Hugh Dickins

Is this the right moment for me to mention again that I'm not sure your reuse of existing pagetables was quite right anyway: NX being excluded from level2_ident_pgt, but wanted in the direct map?

We could add NX. What's the behaviour of setting NX in a non-NX-supporting CPU? I don't think it would trigger a "reserved bit" exception (the other high pte flags don't). Or failing that, we could mask out NX once we've worked out the CPU doesn't support it (at the same time it relocates the pagetables to the kernel's load-time address).

Quoted 7 lines · Jeremy Fitzhardinge

Quoted 4 lines · Hugh Dickins

Is this the right moment for me to mention again that I'm not sure your reuse of existing pagetables was quite right anyway: NX being excluded from level2_ident_pgt, but wanted in the direct map?

Quoted 6 lines

We could add NX. What's the behaviour of setting NX in a non-NX-supporting CPU? I don't think it would trigger a "reserved bit" exception (the other high pte flags don't). Or failing that, we could mask out NX once we've worked out the CPU doesn't support it (at the same time it relocates the pagetables to the kernel's load-time address).

I've no experience of what happens if NX is set to a non-NX-supporting CPU, so can't advise on that at all. But I think you're looking at it the wrong way round - or else I am.

Here's the declaration and comment on level2_ident_pgt in head_64.S:

NEXT_PAGE(level2_ident_pgt)

	/* Since I easily can, map the first 1G.
	 * Don't set NX because code runs from these pages.
	 */
	PMDS(0, __PAGE_KERNEL_LARGE_EXEC, PTRS_PER_PMD)

(The "Since I easily can" comment is there because it used to map only a subset needed for early kernel startup, not the whole 1G.)

So it's very intentionally leaving NX out there. I believe the level2_ident_pgt page appears twice or more in the pagetable layout, used to map two or more areas of virtual address space - once to provide the direct map at ffff880000000000 and once to provide the kernel image virtual mapping at ffffffff80200000.

I think we don't want NX on Linux kernel text ;-?

Before your 2.6.27-rc changes, init_memory_mapping subsequently replaced the direct map usage by a separately constructed pagetable, similar to it but with NX set throughout. After your 2.6.27-rc changes, level2_ident_pgt is found there already so left untouched - leaving NX out of that first 1G of the direct map forever (when CPA splits it up, the smaller pages inherit the lack of NX too).

I'm reluctant to delve in there again at present, and unsure what the right fix should be: perhaps the code which checks if an entry is already there, should check if it has the desired flags?

Well, if we never want the direct map to be non-executable (which I

think would be OK, since all the code is either core kernel or modules which are mapped elsewhere),

then we can set NX on the level4 for the
linear mapping which will make everything below non-executable.

Quoted 2 lines · Jeremy Fitzhardinge

Well, if we never want the direct map to be non-executable (which I

One too many negatives?

Quoted 3 lines

think would be OK, since all the code is either core kernel or modules which are mapped elsewhere),

I had thought so until just before replying.

Quoted 3 lines

then we can set NX on the level4 for the
linear mapping which will make everything below non-executable.

That would be much the neatest answer: I hadn't realized that inheritance (perhaps I'm still living in early-i386 days, when IIRC there was a bug in inheriting WP from higher levels).

But then I stumbled across static_protections() in pageattr.c (takes both addr and pfn, latter seems weird),

A simple answer might be to go the way you suggest, but remove the special casing from pageattr.c and ioremap.c; but I fear that will slow something down, or introduce further bugs.

hpa introduced the 64k idea, and we've all been repeating it; but I've not heard the reasoning behind it. Is it a fundamental addressing limitation within the BIOS memory model? Or a case that Windows treats the bottom 64k as scratch, so BIOS testers won't notice if they corrupt it?

The two instances of corruption we've been studying have indeed been below 64k (one in page 8 and one in page 11), but that's because they were both recognizable corruptions of direct map PMDs.

If there is not a very strong justification for that 64k limit, then I don't think this approach will be very useful, and we should simply continue to rely on analyzing corruption when it appears, and recommend memmap= as a way of avoiding it once analyzed. If there is a strong justification for it, please dispel my ignorance!

hpa introduced the 64k idea, and we've all been repeating it; but I've not heard the reasoning behind it. Is it a fundamental addressing limitation within the BIOS memory model? Or a case that Windows treats the bottom 64k as scratch, so BIOS testers won't notice if they corrupt it?