[PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

On some platforms, there is occasional panic triggered when trying to
resume from hibernation, a typical panic looks like:

"BUG: unable to handle kernel paging request at ffff880085894000
IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70"

This is because e820 map has been changed by BIOS before/after
hibernation, and one of the page frames from first kernel
is right located in second kernel's unmapped region, so panic
comes out when accessing unmapped kernel address.

In order to tell the user why this happeneded, and for scalability,
we introduce a framework(a new file named hibernation_e820.c) to
compare the e820 maps before/after hibernation. If these two
e820 maps are not compatible with each other, we will print
warning about the first corrupt e820 entry's information
(there might be more than one broken e820 entries) once the
system goes into panic, for example:

BUG: unable to handle kernel paging request at ffff8800a9688000
IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70
PM: Hibernation Caution! Oops might be due to inconsistent e820 table.
PM: mem [0xa963b000-0xa963d000][ACPI Table] is an invalid old e820 region.
PM: Inconsistent with current [mem 0xa963b000-0xa963e000][ACPI Table].
PM: Please update your BIOS, or do not use hibernation on this machine.

The following kind of e820 entries will be regarded as invalid ones:
1.E820_RAM:  old region is not a subset of any current region.
2.E820_ACPI: old region is not strictly the same as any current
             region(example above).

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v6:
 - Fix some compiling errors reported by 0day/LKP, adjust
   Kconfig/variable namings.
v5:
 - Rewrite this patch to just warn user of the broken BIOS
   when panic.
v4:
 - Add __attribute__ ((unused)) for swsusp_page_is_valid,
   to eliminate the warnning of:
   'swsusp_page_is_valid' defined but not used
   on non-x86 platforms.

v3:
 - Adjust the logic to exclude the end_pfn boundary in pfn_mapped
   when invoking mark_valid_pages, because the end_pfn is not
   a mapped page frame, we should not regard it as a valid page.

   Move the sanity check of valid pages to a early stage in resuming
   process(moved to mark_unsafe_pages), in this way, we can avoid
   unnecessarily accessing these invalid pages in later stage(yes,
   move to the original position Joey once introduced in:
   Commit 84c91b7ae07c ("PM / hibernate: avoid unsafe pages in e820
   reserved regions")

   With v3 patch applied, I did 30 cycles on my problematic platform,
   no panic triggered anymore(50% reproducible before patched, by
   plugging/unplugging memory peripheral during hibernation), and it
   just warns of invalid pages.
   
v2:
 - According to Ingo's suggestion, rewrite this patch.

   New version just checks each page frame according to pfn_mapped array.
   So that we do not need to touch existing code related to
   E820_RESERVED_KERN. And this method can naturely guarantee
   that the system before/after hibernation do not need to be of
   the same memory size on x86_64.
---
 arch/x86/Kconfig                |   1 +
 arch/x86/power/Makefile         |   2 +-
 arch/x86/power/hibernate_e820.c | 243 ++++++++++++++++++++++++++++++++++++++++
 include/linux/suspend.h         |  19 ++++
 kernel/power/Kconfig            |   4 +
 kernel/power/power.h            |   8 ++
 kernel/power/snapshot.c         |   8 ++
 7 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/power/hibernate_e820.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 96d058a..9f72144 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -30,6 +30,7 @@ config X86
 	select ARCH_HAS_PMEM_API		if X86_64
 	select ARCH_HAS_MMIO_FLUSH
 	select ARCH_HAS_SG_CHAIN
+	select ARCH_HAS_RESUME_IMAGE_CHECKER	if HIBERNATION
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
 	select ARCH_MIGHT_HAVE_PC_PARPORT
diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
index a6a198c..8877cfb 100644
--- a/arch/x86/power/Makefile
+++ b/arch/x86/power/Makefile
@@ -4,4 +4,4 @@ nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_cpu.o	:= $(nostackp)
 
 obj-$(CONFIG_PM_SLEEP)		+= cpu.o
-obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o
+obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o hibernate_e820.o
diff --git a/arch/x86/power/hibernate_e820.c b/arch/x86/power/hibernate_e820.c
new file mode 100644
index 0000000..f51a892
--- /dev/null
+++ b/arch/x86/power/hibernate_e820.c
@@ -0,0 +1,243 @@
+/*
+ * Hibernation e820 consistence checking for x86.
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Authors: Chen Yu <yu.c.chen@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/suspend.h>
+#include <linux/kdebug.h>
+
+/*
+ * The following code is to check whether the old e820 map
+ * (system before hibernation) is compatible with current
+ * e820 map(system for resuming).
+ * We check two types of regions: E820_RAM and E820_ACPI,
+ * and to make sure the two kinds of regions will satisfy:
+ * 1. E820_RAM: each old region is a subset of the current ones.
+ * 2. E820_ACPI: each old region is strictly the same as the current ones.
+ *
+ * We save the old e820 map inside the swsusp_info page,
+ * then pass it to the second system for resuming, by the
+ * following format:
+ *
+ *
+ *  +--------+---------+------+------+------+
+ *  | swsusp |e820entry|entry0|entry1|entry2|
+ *  |  info  | number  |      |      |      |
+ *  +--------+---------+------+------+------+
+ *  ^                                                        ^
+ *  |                                                        |
+ *  +--------------struct swsusp_info(PAGE_SIZE)-------------+
+ */
+
+/*
+ * Record the first pair of conflicted new/old
+ * e820 entries if there's any.
+ */
+static u32 bad_old_type;
+static u64 bad_old_start, bad_old_end;
+
+static u32 bad_new_type;
+static u64 bad_new_start, bad_new_end;
+
+/**
+ *	arch_image_info_save - save specified e820 data to
+ *		 the hibernation image header
+ *	@dst: address to save the data to.
+ *	@src: source data need to be saved,
+ *	      if NULL then save current system's e820 map.
+ *	@limit_len: max len in bytes to write.
+ */
+int arch_image_info_save(char *dst, const char *src, unsigned int limit_len)
+{
+	unsigned int nr_e820_map;
+	unsigned int size_to_copy;
+	struct e820map *e820_map;
+
+	/*
+	 * The final copied structure is illustrated below:
+	 * [number_of_e820entry][e820entry0)[e820entry1)...
+	 */
+	if (src) {
+		nr_e820_map = *(unsigned int *)src;
+		e820_map = (struct e820map *)(src + sizeof(unsigned int));
+	} else {
+		nr_e820_map = e820_saved.nr_map;
+		e820_map = &e820_saved;
+	}
+
+	size_to_copy = nr_e820_map * sizeof(struct e820entry);
+
+	if ((size_to_copy + sizeof(unsigned int)) > limit_len) {
+		pr_warn("PM: Hibernation can not save extra info due to too many e820 entries\n");
+		return -ENOMEM;
+	}
+	*(unsigned int *)dst = nr_e820_map;
+	dst += sizeof(unsigned int);
+	memcpy(dst, (void *)&e820_map->map[0], size_to_copy);
+	return 0;
+}
+
+/**
+ *	arch_image_info_check - check the relationship between
+ *	new and old e820 map, to make sure that, the E820_RAM
+ *	in old e820, is a subset of the new e820 map, and the
+ *	E820_ACPI regions in old e820 map, are strictly the
+ *	same as new e820 map. If it is, return true, otherwise return false.
+ *
+ *	@new: New e820 map address, usually it is the
+ *	      current system's e820_saved.
+ *	@old: Old e820 map address, it is usually the
+ *	      e820 map before hibernation.
+ */
+bool arch_image_info_check(const char *new, const char *old)
+{
+	struct e820map *e820_old, *e820_new;
+	int i, j, nr_e820_old, nr_e820_new;
+
+	e820_old = (struct e820map *)old;
+	nr_e820_old = *(unsigned int *)e820_old;
+
+	if (new)
+		e820_new = (struct e820map *)new;
+	else
+		e820_new = &e820_saved;
+
+	nr_e820_new = e820_new->nr_map;
+
+	if ((nr_e820_old == 0) || (nr_e820_new == 0) ||
+		(nr_e820_old > E820_X_MAX) || (nr_e820_new > E820_X_MAX))
+		return false;
+
+	for (i = 0; i < nr_e820_old; i++) {
+		u64 old_start, old_end;
+		struct e820entry *ei_old;
+		bool valid_old_entry = false;
+
+		ei_old = &e820_old->map[i];
+
+		/*
+		 * Only check RAM memory and ACPI table regions,
+		 * and we follow this policy:
+		 * 1.The old e820 RAM region must be new RAM's subset.
+		 * 2.The old e820 ACPI table region must be the same
+		 *   as the new one.
+		 */
+		if (ei_old->type != E820_RAM && ei_old->type != E820_ACPI)
+			continue;
+
+		old_start = ei_old->addr;
+		old_end = ei_old->addr + ei_old->size;
+
+		for (j = 0; j < nr_e820_new; j++) {
+			u64 new_start, new_end;
+			struct e820entry *ei_new;
+
+			if (valid_old_entry)
+				break;
+
+			ei_new = &e820_new->map[i];
+			new_start = ei_new->addr;
+			new_end = ei_new->addr + ei_new->size;
+
+			/*
+			 * Check the relationship between these two regions.
+			 */
+			if (old_start >= new_start && old_start < new_end) {
+				   /* Must be of the same type. */
+				if ((ei_old->type != ei_new->type) ||
+				   /* E820_RAM must be the subset */
+				    ((ei_old->type == E820_RAM) &&
+				     (old_end > new_end)) ||
+				   /* E820_ACPI must remain unchanged. */
+				    ((ei_old->type == E820_ACPI) &&
+				     (old_start != new_start ||
+						old_end != new_end))) {
+					bad_old_start = old_start;
+					bad_old_end = old_end;
+					bad_old_type = ei_old->type;
+					bad_new_start = new_start;
+					bad_new_end = new_end;
+					bad_new_type = ei_new->type;
+
+					return false;
+				}
+				/* OK, this one is a valid e820 region. */
+				valid_old_entry = true;
+			}
+		}
+		/* If we did not find any overlapping between this old e820
+		 * region and the new regions, return invalid.
+		 */
+		if (!valid_old_entry) {
+			bad_old_start = old_start;
+			bad_old_end = old_end;
+			return false;
+		}
+	}
+	/* All the old e820 entries are valid */
+	return true;
+}
+
+static void e820_dump_map(struct e820map *e820_dump)
+{
+	int i;
+
+	for (i = 0; i < e820_dump->nr_map; i++) {
+		printk(KERN_ERR "[mem %#018Lx-%#018Lx] [%d]\n",
+		       (unsigned long long) e820_dump->map[i].addr,
+		       (unsigned long long)
+		       (e820_dump->map[i].addr + e820_dump->map[i].size - 1),
+			e820_dump->map[i].type);
+	}
+}
+
+/*
+ * This hook is invoked when kernel dies, and will print the broken e820 map
+ * if it is caused by BIOS memory bug.
+ */
+static int arch_hibernation_die_check(struct notifier_block *nb,
+				      unsigned long action,
+				      void *data)
+{
+	if (!bad_old_start || !bad_old_end)
+		return 0;
+
+	pr_err("PM: Hibernation Caution! Oops might be due to inconsistent e820 table.\n");
+	pr_err("PM: [mem %#010llx-%#010llx][%s] is an invalid old e820 region.\n",
+			bad_old_start, bad_old_end - 1,
+			(bad_old_type == E820_RAM) ? "RAM" : "ACPI Table");
+	if (bad_new_start && bad_new_end)
+		pr_err("PM: Inconsistent with current [mem %#010llx-%#010llx][%s]\n",
+			bad_new_start, bad_new_end - 1,
+			(bad_new_type == E820_RAM) ? "RAM" : "ACPI Table");
+	pr_err("PM: Please update your BIOS, or do not use hibernation on this machine.\n");
+	pr_err("PM: Current system's e820 table:\n");
+	e820_dump_map(&e820_saved);
+	/* Avoid nested die print*/
+	bad_old_start = bad_old_end = 0;
+
+	return 0;
+}
+
+static struct notifier_block hibernation_notifier = {
+	.notifier_call = arch_hibernation_die_check,
+};
+
+static int __init arch_init_hibernation(void)
+{
+	int retval;
+
+	retval = register_die_notifier(&hibernation_notifier);
+	if (retval)
+		return retval;
+
+	return 0;
+}
+
+late_initcall(arch_init_hibernation);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..5946b5c 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -361,6 +361,25 @@ static inline bool system_entering_hibernation(void) { return false; }
 static inline bool hibernation_available(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
+#ifdef CONFIG_ARCH_HAS_RESUME_IMAGE_CHECKER
+extern int arch_image_info_save(char *dst, const char *src,
+				unsigned int limit_len);
+extern bool arch_image_info_check(const char *new, const char *old);
+#else
+static inline bool arch_image_info_check(const char *new,
+					 const char *old)
+{
+	return true;
+}
+
+static inline int arch_image_info_save(char *dst,
+					const char *src,
+					unsigned int limit_len)
+{
+	return 0;
+}
+#endif
+
 /* Hibernation and suspend events */
 #define PM_HIBERNATION_PREPARE	0x0001 /* Going to hibernate */
 #define PM_POST_HIBERNATION	0x0002 /* Hibernation finished */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 02e8dfa..4d8e6d8 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -79,6 +79,10 @@ config HIBERNATION
 config ARCH_SAVE_PAGE_KEYS
 	bool
 
+config ARCH_HAS_RESUME_IMAGE_CHECKER
+	bool
+	depends on HIBERNATION
+
 config PM_STD_PARTITION
 	string "Default resume partition"
 	depends on HIBERNATION
diff --git a/kernel/power/power.h b/kernel/power/power.h
index caadb56..d279907 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -14,6 +14,14 @@ struct swsusp_info {
 	unsigned long		size;
 } __aligned(PAGE_SIZE);
 
+/*
+ *  Since struct swsusp_info will take one page size,
+ *  some platforms save the extra data right after the
+ *  last structure element.
+ */
+#define SWSUSP_INFO_ACTUAL_SIZE \
+	(offsetof(struct swsusp_info, size) + sizeof(unsigned long))
+
 #ifdef CONFIG_HIBERNATION
 /* kernel/power/snapshot.c */
 extern void __init hibernate_reserved_size_init(void);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 5235dd4..394d20d 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1970,6 +1970,11 @@ int snapshot_read_next(struct snapshot_handle *handle)
 		error = init_header((struct swsusp_info *)buffer);
 		if (error)
 			return error;
+
+		arch_image_info_save((char *)buffer + SWSUSP_INFO_ACTUAL_SIZE,
+				     NULL,
+				     PAGE_SIZE-SWSUSP_INFO_ACTUAL_SIZE);
+
 		handle->buffer = buffer;
 		memory_bm_position_reset(&orig_bm);
 		memory_bm_position_reset(&copy_bm);
@@ -2491,6 +2496,9 @@ int snapshot_write_next(struct snapshot_handle *handle)
 		if (error)
 			return error;
 
+		arch_image_info_check(NULL,
+				     (char *)buffer + SWSUSP_INFO_ACTUAL_SIZE);
+
 		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
 		if (error)
 			return error;
-- 
1.8.4.2

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[joeyli]

Hi all, 

On Wed, Oct 21, 2015 at 01:21:40PM +0800, Chen Yu wrote:
> On some platforms, there is occasional panic triggered when trying to
> resume from hibernation, a typical panic looks like:
> 
> "BUG: unable to handle kernel paging request at ffff880085894000
> IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70"
> 
> This is because e820 map has been changed by BIOS before/after
> hibernation, and one of the page frames from first kernel
> is right located in second kernel's unmapped region, so panic
> comes out when accessing unmapped kernel address.
> 
> In order to tell the user why this happeneded, and for scalability,
> we introduce a framework(a new file named hibernation_e820.c) to
> compare the e820 maps before/after hibernation. If these two
> e820 maps are not compatible with each other, we will print
> warning about the first corrupt e820 entry's information
> (there might be more than one broken e820 entries) once the
> system goes into panic, for example:
> 
> BUG: unable to handle kernel paging request at ffff8800a9688000
> IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70
> PM: Hibernation Caution! Oops might be due to inconsistent e820 table.
> PM: mem [0xa963b000-0xa963d000][ACPI Table] is an invalid old e820 region.
> PM: Inconsistent with current [mem 0xa963b000-0xa963e000][ACPI Table].
> PM: Please update your BIOS, or do not use hibernation on this machine.
> 
> The following kind of e820 entries will be regarded as invalid ones:
> 1.E820_RAM:  old region is not a subset of any current region.
> 2.E820_ACPI: old region is not strictly the same as any current
>              region(example above).
> 
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v6:
>  - Fix some compiling errors reported by 0day/LKP, adjust
>    Kconfig/variable namings.
> v5:
>  - Rewrite this patch to just warn user of the broken BIOS
>    when panic.
> v4:
>  - Add __attribute__ ((unused)) for swsusp_page_is_valid,
>    to eliminate the warnning of:
>    'swsusp_page_is_valid' defined but not used
>    on non-x86 platforms.
> 
> v3:
>  - Adjust the logic to exclude the end_pfn boundary in pfn_mapped
>    when invoking mark_valid_pages, because the end_pfn is not
>    a mapped page frame, we should not regard it as a valid page.
> 
>    Move the sanity check of valid pages to a early stage in resuming
>    process(moved to mark_unsafe_pages), in this way, we can avoid
>    unnecessarily accessing these invalid pages in later stage(yes,
>    move to the original position Joey once introduced in:
>    Commit 84c91b7ae07c ("PM / hibernate: avoid unsafe pages in e820
>    reserved regions")
> 
>    With v3 patch applied, I did 30 cycles on my problematic platform,
>    no panic triggered anymore(50% reproducible before patched, by
>    plugging/unplugging memory peripheral during hibernation), and it
>    just warns of invalid pages.
>    
> v2:
>  - According to Ingo's suggestion, rewrite this patch.
> 
>    New version just checks each page frame according to pfn_mapped array.
>    So that we do not need to touch existing code related to
>    E820_RESERVED_KERN. And this method can naturely guarantee
>    that the system before/after hibernation do not need to be of
>    the same memory size on x86_64.

What's the progress of this patch? Looks already have experts review it.
Why this patch didn't accept?


Thanks a lot!
Joey Lee

> ---
>  arch/x86/Kconfig                |   1 +
>  arch/x86/power/Makefile         |   2 +-
>  arch/x86/power/hibernate_e820.c | 243 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/suspend.h         |  19 ++++
>  kernel/power/Kconfig            |   4 +
>  kernel/power/power.h            |   8 ++
>  kernel/power/snapshot.c         |   8 ++
>  7 files changed, 284 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/power/hibernate_e820.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 96d058a..9f72144 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -30,6 +30,7 @@ config X86
>  	select ARCH_HAS_PMEM_API		if X86_64
>  	select ARCH_HAS_MMIO_FLUSH
>  	select ARCH_HAS_SG_CHAIN
> +	select ARCH_HAS_RESUME_IMAGE_CHECKER	if HIBERNATION
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG
>  	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
> diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
> index a6a198c..8877cfb 100644
> --- a/arch/x86/power/Makefile
> +++ b/arch/x86/power/Makefile
> @@ -4,4 +4,4 @@ nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_cpu.o	:= $(nostackp)
>  
>  obj-$(CONFIG_PM_SLEEP)		+= cpu.o
> -obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o
> +obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o hibernate_e820.o
> diff --git a/arch/x86/power/hibernate_e820.c b/arch/x86/power/hibernate_e820.c
> new file mode 100644
> index 0000000..f51a892
> --- /dev/null
> +++ b/arch/x86/power/hibernate_e820.c
> @@ -0,0 +1,243 @@
> +/*
> + * Hibernation e820 consistence checking for x86.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + * Authors: Chen Yu <yu.c.chen@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/suspend.h>
> +#include <linux/kdebug.h>
> +
> +/*
> + * The following code is to check whether the old e820 map
> + * (system before hibernation) is compatible with current
> + * e820 map(system for resuming).
> + * We check two types of regions: E820_RAM and E820_ACPI,
> + * and to make sure the two kinds of regions will satisfy:
> + * 1. E820_RAM: each old region is a subset of the current ones.
> + * 2. E820_ACPI: each old region is strictly the same as the current ones.
> + *
> + * We save the old e820 map inside the swsusp_info page,
> + * then pass it to the second system for resuming, by the
> + * following format:
> + *
> + *
> + *  +--------+---------+------+------+------+
> + *  | swsusp |e820entry|entry0|entry1|entry2|
> + *  |  info  | number  |      |      |      |
> + *  +--------+---------+------+------+------+
> + *  ^                                                        ^
> + *  |                                                        |
> + *  +--------------struct swsusp_info(PAGE_SIZE)-------------+
> + */
> +
> +/*
> + * Record the first pair of conflicted new/old
> + * e820 entries if there's any.
> + */
> +static u32 bad_old_type;
> +static u64 bad_old_start, bad_old_end;
> +
> +static u32 bad_new_type;
> +static u64 bad_new_start, bad_new_end;
> +
> +/**
> + *	arch_image_info_save - save specified e820 data to
> + *		 the hibernation image header
> + *	@dst: address to save the data to.
> + *	@src: source data need to be saved,
> + *	      if NULL then save current system's e820 map.
> + *	@limit_len: max len in bytes to write.
> + */
> +int arch_image_info_save(char *dst, const char *src, unsigned int limit_len)
> +{
> +	unsigned int nr_e820_map;
> +	unsigned int size_to_copy;
> +	struct e820map *e820_map;
> +
> +	/*
> +	 * The final copied structure is illustrated below:
> +	 * [number_of_e820entry][e820entry0)[e820entry1)...
> +	 */
> +	if (src) {
> +		nr_e820_map = *(unsigned int *)src;
> +		e820_map = (struct e820map *)(src + sizeof(unsigned int));
> +	} else {
> +		nr_e820_map = e820_saved.nr_map;
> +		e820_map = &e820_saved;
> +	}
> +
> +	size_to_copy = nr_e820_map * sizeof(struct e820entry);
> +
> +	if ((size_to_copy + sizeof(unsigned int)) > limit_len) {
> +		pr_warn("PM: Hibernation can not save extra info due to too many e820 entries\n");
> +		return -ENOMEM;
> +	}
> +	*(unsigned int *)dst = nr_e820_map;
> +	dst += sizeof(unsigned int);
> +	memcpy(dst, (void *)&e820_map->map[0], size_to_copy);
> +	return 0;
> +}
> +
> +/**
> + *	arch_image_info_check - check the relationship between
> + *	new and old e820 map, to make sure that, the E820_RAM
> + *	in old e820, is a subset of the new e820 map, and the
> + *	E820_ACPI regions in old e820 map, are strictly the
> + *	same as new e820 map. If it is, return true, otherwise return false.
> + *
> + *	@new: New e820 map address, usually it is the
> + *	      current system's e820_saved.
> + *	@old: Old e820 map address, it is usually the
> + *	      e820 map before hibernation.
> + */
> +bool arch_image_info_check(const char *new, const char *old)
> +{
> +	struct e820map *e820_old, *e820_new;
> +	int i, j, nr_e820_old, nr_e820_new;
> +
> +	e820_old = (struct e820map *)old;
> +	nr_e820_old = *(unsigned int *)e820_old;
> +
> +	if (new)
> +		e820_new = (struct e820map *)new;
> +	else
> +		e820_new = &e820_saved;
> +
> +	nr_e820_new = e820_new->nr_map;
> +
> +	if ((nr_e820_old == 0) || (nr_e820_new == 0) ||
> +		(nr_e820_old > E820_X_MAX) || (nr_e820_new > E820_X_MAX))
> +		return false;
> +
> +	for (i = 0; i < nr_e820_old; i++) {
> +		u64 old_start, old_end;
> +		struct e820entry *ei_old;
> +		bool valid_old_entry = false;
> +
> +		ei_old = &e820_old->map[i];
> +
> +		/*
> +		 * Only check RAM memory and ACPI table regions,
> +		 * and we follow this policy:
> +		 * 1.The old e820 RAM region must be new RAM's subset.
> +		 * 2.The old e820 ACPI table region must be the same
> +		 *   as the new one.
> +		 */
> +		if (ei_old->type != E820_RAM && ei_old->type != E820_ACPI)
> +			continue;
> +
> +		old_start = ei_old->addr;
> +		old_end = ei_old->addr + ei_old->size;
> +
> +		for (j = 0; j < nr_e820_new; j++) {
> +			u64 new_start, new_end;
> +			struct e820entry *ei_new;
> +
> +			if (valid_old_entry)
> +				break;
> +
> +			ei_new = &e820_new->map[i];
> +			new_start = ei_new->addr;
> +			new_end = ei_new->addr + ei_new->size;
> +
> +			/*
> +			 * Check the relationship between these two regions.
> +			 */
> +			if (old_start >= new_start && old_start < new_end) {
> +				   /* Must be of the same type. */
> +				if ((ei_old->type != ei_new->type) ||
> +				   /* E820_RAM must be the subset */
> +				    ((ei_old->type == E820_RAM) &&
> +				     (old_end > new_end)) ||
> +				   /* E820_ACPI must remain unchanged. */
> +				    ((ei_old->type == E820_ACPI) &&
> +				     (old_start != new_start ||
> +						old_end != new_end))) {
> +					bad_old_start = old_start;
> +					bad_old_end = old_end;
> +					bad_old_type = ei_old->type;
> +					bad_new_start = new_start;
> +					bad_new_end = new_end;
> +					bad_new_type = ei_new->type;
> +
> +					return false;
> +				}
> +				/* OK, this one is a valid e820 region. */
> +				valid_old_entry = true;
> +			}
> +		}
> +		/* If we did not find any overlapping between this old e820
> +		 * region and the new regions, return invalid.
> +		 */
> +		if (!valid_old_entry) {
> +			bad_old_start = old_start;
> +			bad_old_end = old_end;
> +			return false;
> +		}
> +	}
> +	/* All the old e820 entries are valid */
> +	return true;
> +}
> +
> +static void e820_dump_map(struct e820map *e820_dump)
> +{
> +	int i;
> +
> +	for (i = 0; i < e820_dump->nr_map; i++) {
> +		printk(KERN_ERR "[mem %#018Lx-%#018Lx] [%d]\n",
> +		       (unsigned long long) e820_dump->map[i].addr,
> +		       (unsigned long long)
> +		       (e820_dump->map[i].addr + e820_dump->map[i].size - 1),
> +			e820_dump->map[i].type);
> +	}
> +}
> +
> +/*
> + * This hook is invoked when kernel dies, and will print the broken e820 map
> + * if it is caused by BIOS memory bug.
> + */
> +static int arch_hibernation_die_check(struct notifier_block *nb,
> +				      unsigned long action,
> +				      void *data)
> +{
> +	if (!bad_old_start || !bad_old_end)
> +		return 0;
> +
> +	pr_err("PM: Hibernation Caution! Oops might be due to inconsistent e820 table.\n");
> +	pr_err("PM: [mem %#010llx-%#010llx][%s] is an invalid old e820 region.\n",
> +			bad_old_start, bad_old_end - 1,
> +			(bad_old_type == E820_RAM) ? "RAM" : "ACPI Table");
> +	if (bad_new_start && bad_new_end)
> +		pr_err("PM: Inconsistent with current [mem %#010llx-%#010llx][%s]\n",
> +			bad_new_start, bad_new_end - 1,
> +			(bad_new_type == E820_RAM) ? "RAM" : "ACPI Table");
> +	pr_err("PM: Please update your BIOS, or do not use hibernation on this machine.\n");
> +	pr_err("PM: Current system's e820 table:\n");
> +	e820_dump_map(&e820_saved);
> +	/* Avoid nested die print*/
> +	bad_old_start = bad_old_end = 0;
> +
> +	return 0;
> +}
> +
> +static struct notifier_block hibernation_notifier = {
> +	.notifier_call = arch_hibernation_die_check,
> +};
> +
> +static int __init arch_init_hibernation(void)
> +{
> +	int retval;
> +
> +	retval = register_die_notifier(&hibernation_notifier);
> +	if (retval)
> +		return retval;
> +
> +	return 0;
> +}
> +
> +late_initcall(arch_init_hibernation);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..5946b5c 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -361,6 +361,25 @@ static inline bool system_entering_hibernation(void) { return false; }
>  static inline bool hibernation_available(void) { return false; }
>  #endif /* CONFIG_HIBERNATION */
>  
> +#ifdef CONFIG_ARCH_HAS_RESUME_IMAGE_CHECKER
> +extern int arch_image_info_save(char *dst, const char *src,
> +				unsigned int limit_len);
> +extern bool arch_image_info_check(const char *new, const char *old);
> +#else
> +static inline bool arch_image_info_check(const char *new,
> +					 const char *old)
> +{
> +	return true;
> +}
> +
> +static inline int arch_image_info_save(char *dst,
> +					const char *src,
> +					unsigned int limit_len)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /* Hibernation and suspend events */
>  #define PM_HIBERNATION_PREPARE	0x0001 /* Going to hibernate */
>  #define PM_POST_HIBERNATION	0x0002 /* Hibernation finished */
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 02e8dfa..4d8e6d8 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -79,6 +79,10 @@ config HIBERNATION
>  config ARCH_SAVE_PAGE_KEYS
>  	bool
>  
> +config ARCH_HAS_RESUME_IMAGE_CHECKER
> +	bool
> +	depends on HIBERNATION
> +
>  config PM_STD_PARTITION
>  	string "Default resume partition"
>  	depends on HIBERNATION
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index caadb56..d279907 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -14,6 +14,14 @@ struct swsusp_info {
>  	unsigned long		size;
>  } __aligned(PAGE_SIZE);
>  
> +/*
> + *  Since struct swsusp_info will take one page size,
> + *  some platforms save the extra data right after the
> + *  last structure element.
> + */
> +#define SWSUSP_INFO_ACTUAL_SIZE \
> +	(offsetof(struct swsusp_info, size) + sizeof(unsigned long))
> +
>  #ifdef CONFIG_HIBERNATION
>  /* kernel/power/snapshot.c */
>  extern void __init hibernate_reserved_size_init(void);
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 5235dd4..394d20d 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1970,6 +1970,11 @@ int snapshot_read_next(struct snapshot_handle *handle)
>  		error = init_header((struct swsusp_info *)buffer);
>  		if (error)
>  			return error;
> +
> +		arch_image_info_save((char *)buffer + SWSUSP_INFO_ACTUAL_SIZE,
> +				     NULL,
> +				     PAGE_SIZE-SWSUSP_INFO_ACTUAL_SIZE);
> +
>  		handle->buffer = buffer;
>  		memory_bm_position_reset(&orig_bm);
>  		memory_bm_position_reset(&copy_bm);
> @@ -2491,6 +2496,9 @@ int snapshot_write_next(struct snapshot_handle *handle)
>  		if (error)
>  			return error;
>  
> +		arch_image_info_check(NULL,
> +				     (char *)buffer + SWSUSP_INFO_ACTUAL_SIZE);
> +
>  		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
>  		if (error)
>  			return error;
> -- 
> 1.8.4.2
> 
> --
> 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/

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

Hi,
thanks for your interest :)
On Tue, Aug 23, 2016 at 05:45:27PM +0800, joeyli wrote:
> Hi all, 
> 
> On Wed, Oct 21, 2015 at 01:21:40PM +0800, Chen Yu wrote:
> > On some platforms, there is occasional panic triggered when trying to
> > resume from hibernation, a typical panic looks like:
> > 
> > "BUG: unable to handle kernel paging request at ffff880085894000
> > IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70"
> > 
> > This is because e820 map has been changed by BIOS before/after
> > hibernation, and one of the page frames from first kernel
> > is right located in second kernel's unmapped region, so panic
> > comes out when accessing unmapped kernel address.
> > 
> > In order to tell the user why this happeneded, and for scalability,
> > we introduce a framework(a new file named hibernation_e820.c) to
> > compare the e820 maps before/after hibernation. If these two
> > e820 maps are not compatible with each other, we will print
> > warning about the first corrupt e820 entry's information
> > (there might be more than one broken e820 entries) once the
> > system goes into panic, for example:
> > 
> > BUG: unable to handle kernel paging request at ffff8800a9688000
> > IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70
> > PM: Hibernation Caution! Oops might be due to inconsistent e820 table.
> > PM: mem [0xa963b000-0xa963d000][ACPI Table] is an invalid old e820 region.
> > PM: Inconsistent with current [mem 0xa963b000-0xa963e000][ACPI Table].
> > PM: Please update your BIOS, or do not use hibernation on this machine.
> > 
> > The following kind of e820 entries will be regarded as invalid ones:
> > 1.E820_RAM:  old region is not a subset of any current region.
> > 2.E820_ACPI: old region is not strictly the same as any current
> >              region(example above).
> > 
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> > v6:
> >  - Fix some compiling errors reported by 0day/LKP, adjust
> >    Kconfig/variable namings.
> > v5:
> >  - Rewrite this patch to just warn user of the broken BIOS
> >    when panic.
> > v4:
> >  - Add __attribute__ ((unused)) for swsusp_page_is_valid,
> >    to eliminate the warnning of:
> >    'swsusp_page_is_valid' defined but not used
> >    on non-x86 platforms.
> > 
> > v3:
> >  - Adjust the logic to exclude the end_pfn boundary in pfn_mapped
> >    when invoking mark_valid_pages, because the end_pfn is not
> >    a mapped page frame, we should not regard it as a valid page.
> > 
> >    Move the sanity check of valid pages to a early stage in resuming
> >    process(moved to mark_unsafe_pages), in this way, we can avoid
> >    unnecessarily accessing these invalid pages in later stage(yes,
> >    move to the original position Joey once introduced in:
> >    Commit 84c91b7ae07c ("PM / hibernate: avoid unsafe pages in e820
> >    reserved regions")
> > 
> >    With v3 patch applied, I did 30 cycles on my problematic platform,
> >    no panic triggered anymore(50% reproducible before patched, by
> >    plugging/unplugging memory peripheral during hibernation), and it
> >    just warns of invalid pages.
> >    
> > v2:
> >  - According to Ingo's suggestion, rewrite this patch.
> > 
> >    New version just checks each page frame according to pfn_mapped array.
> >    So that we do not need to touch existing code related to
> >    E820_RESERVED_KERN. And this method can naturely guarantee
> >    that the system before/after hibernation do not need to be of
> >    the same memory size on x86_64.
> 
> What's the progress of this patch? Looks already have experts review it.
> Why this patch didn't accept?
This patch is a little overkilled, and I have saved another simpler
version to only check the md5 hash (as people suggested) for it. I can post it later.

thanks,
Yu

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[joeyli]

On Tue, Aug 23, 2016 at 06:01:55PM +0800, Chen Yu wrote:
> Hi,
> thanks for your interest :)
> On Tue, Aug 23, 2016 at 05:45:27PM +0800, joeyli wrote:
> > Hi all, 
> > 
> > On Wed, Oct 21, 2015 at 01:21:40PM +0800, Chen Yu wrote:
> > > On some platforms, there is occasional panic triggered when trying to
> > > resume from hibernation, a typical panic looks like:
> > > 
> > > "BUG: unable to handle kernel paging request at ffff880085894000
> > > IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70"
> > > 
> > > This is because e820 map has been changed by BIOS before/after
> > > hibernation, and one of the page frames from first kernel
> > > is right located in second kernel's unmapped region, so panic
> > > comes out when accessing unmapped kernel address.
> > > 
> > > In order to tell the user why this happeneded, and for scalability,
> > > we introduce a framework(a new file named hibernation_e820.c) to
> > > compare the e820 maps before/after hibernation. If these two
> > > e820 maps are not compatible with each other, we will print
> > > warning about the first corrupt e820 entry's information
> > > (there might be more than one broken e820 entries) once the
> > > system goes into panic, for example:
> > > 
> > > BUG: unable to handle kernel paging request at ffff8800a9688000
> > > IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70
> > > PM: Hibernation Caution! Oops might be due to inconsistent e820 table.
> > > PM: mem [0xa963b000-0xa963d000][ACPI Table] is an invalid old e820 region.
> > > PM: Inconsistent with current [mem 0xa963b000-0xa963e000][ACPI Table].
> > > PM: Please update your BIOS, or do not use hibernation on this machine.
> > > 
> > > The following kind of e820 entries will be regarded as invalid ones:
> > > 1.E820_RAM:  old region is not a subset of any current region.
> > > 2.E820_ACPI: old region is not strictly the same as any current
> > >              region(example above).
> > > 
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > > v6:
> > >  - Fix some compiling errors reported by 0day/LKP, adjust
> > >    Kconfig/variable namings.
> > > v5:
> > >  - Rewrite this patch to just warn user of the broken BIOS
> > >    when panic.
> > > v4:
> > >  - Add __attribute__ ((unused)) for swsusp_page_is_valid,
> > >    to eliminate the warnning of:
> > >    'swsusp_page_is_valid' defined but not used
> > >    on non-x86 platforms.
> > > 
> > > v3:
> > >  - Adjust the logic to exclude the end_pfn boundary in pfn_mapped
> > >    when invoking mark_valid_pages, because the end_pfn is not
> > >    a mapped page frame, we should not regard it as a valid page.
> > > 
> > >    Move the sanity check of valid pages to a early stage in resuming
> > >    process(moved to mark_unsafe_pages), in this way, we can avoid
> > >    unnecessarily accessing these invalid pages in later stage(yes,
> > >    move to the original position Joey once introduced in:
> > >    Commit 84c91b7ae07c ("PM / hibernate: avoid unsafe pages in e820
> > >    reserved regions")
> > > 
> > >    With v3 patch applied, I did 30 cycles on my problematic platform,
> > >    no panic triggered anymore(50% reproducible before patched, by
> > >    plugging/unplugging memory peripheral during hibernation), and it
> > >    just warns of invalid pages.
> > >    
> > > v2:
> > >  - According to Ingo's suggestion, rewrite this patch.
> > > 
> > >    New version just checks each page frame according to pfn_mapped array.
> > >    So that we do not need to touch existing code related to
> > >    E820_RESERVED_KERN. And this method can naturely guarantee
> > >    that the system before/after hibernation do not need to be of
> > >    the same memory size on x86_64.
> > 
> > What's the progress of this patch? Looks already have experts review it.
> > Why this patch didn't accept?
> This patch is a little overkilled, and I have saved another simpler
> version to only check the md5 hash (as people suggested) for it. I can post it later.
> 
> thanks,
> Yu

I am happy to test and review it.

Thanks a lot!
Joey Lee

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

On Wed, Aug 24, 2016 at 09:36:10AM +0800, joeyli wrote:
> > > What's the progress of this patch? Looks already have experts review it.
> > > Why this patch didn't accept?
> > This patch is a little overkilled, and I have saved another simpler
> > version to only check the md5 hash (as people suggested) for it. I can post it later.
> > 
> 
> I am happy to test and review it.
>
Here it is. As Rafael is on travel, it would be grateful
if you can give some advance on this, thanks!


On some platforms, there is occasional panic triggered when trying to
resume from hibernation, a typical panic looks like:

"BUG: unable to handle kernel paging request at ffff880085894000
IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70"

This is because e820 map has been changed by BIOS across
hibernation, and one of the page frames from first kernel
is right located in second kernel's unmapped region, so panic
comes out when accessing unmapped kernel address.

In order to tell the user why this happened, we compare the two
e820 maps according to their md5 value. If these two e820 maps
are not the same, we will warn users of the possible e820 conflict
once the system goes into panic, for example:

BUG: unable to handle kernel paging request at ffff8800a9688000
IP: [<ffffffff810c5dc2>] load_image_lzo+0x8c2/0xe70
PM: Hibernation Caution! Panic might be caused by inconsistent e820 table.
PM: Please update your BIOS, or do not use hibernation on this machine.

Note: It is possible although BIOS has provided a consistent memory map,
but the e820_saved is not strictly the same across hibernation.
This is because in theory mptable might modify the e820_saved dynamically
during early stage, but that situation is quite rare and we don't
deal with that for now.

Suggested-by: Pavel Machek <pavel@ucw.cz>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v7:
 - Use md5 hash to compare the e820 map.
v6:
 - Fix some compiling errors reported by 0day/LKP, adjust
   Kconfig/variable namings.
v5:
 - Rewrite this patch to just warn user of the broken BIOS
   when panic.
v4:
 - Add __attribute__ ((unused)) for swsusp_page_is_valid,
   to eliminate the warnning of:
   'swsusp_page_is_valid' defined but not used
   on non-x86 platforms.

v3:
 - Adjust the logic to exclude the end_pfn boundary in pfn_mapped
   when invoking mark_valid_pages, because the end_pfn is not
   a mapped page frame, we should not regard it as a valid page.

   Move the sanity check of valid pages to a early stage in resuming
   process(moved to mark_unsafe_pages), in this way, we can avoid
   unnecessarily accessing these invalid pages in later stage(yes,
   move to the original position Joey once introduced in:
   Commit 84c91b7ae07c ("PM / hibernate: avoid unsafe pages in e820
   reserved regions")

   With v3 patch applied, I did 30 cycles on my problematic platform,
   no panic triggered anymore(50% reproducible before patched, by
   plugging/unplugging memory peripheral during hibernation), and it
   just warns of invalid pages.
   
v2:
 - According to Ingo's suggestion, rewrite this patch.

   New version just checks each page frame according to pfn_mapped array.
   So that we do not need to touch existing code related to
   E820_RESERVED_KERN. And this method can naturely guarantee
   that the system before/after hibernation do not need to be of
   the same memory size on x86_64.
---
 arch/x86/power/hibernate_64.c | 131 ++++++++++++++++++++++++++++++++++++++++++
 kernel/power/Kconfig          |   9 +++
 2 files changed, 140 insertions(+)

diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
index 9634557..56ab7f4 100644
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -11,6 +11,10 @@
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/suspend.h>
+#include <linux/scatterlist.h>
+#include <linux/kdebug.h>
+
+#include <crypto/hash.h>
 
 #include <asm/init.h>
 #include <asm/proto.h>
@@ -186,6 +190,119 @@ struct restore_data_record {
 
 #define RESTORE_MAGIC	0x123456789ABCDEF0UL
 
+/* The resume kernel's e820 is conflict with the one in suspend kernel */
+static bool e820_conflict;
+
+#ifdef CONFIG_HIBERNATION_CHECK_E820
+#define MD5_HASH_SIZE 128
+
+/**
+ *	get_e820_md5 - calculate md5 according to struct e820map
+ *
+ *	@map: the e820 map to be calculated
+ *	@buf: the md5 result to be stored to
+ */
+static int get_e820_md5(struct e820map *map, void *buf)
+{
+	struct scatterlist sg;
+	struct crypto_ahash *tfm;
+	struct ahash_request *req;
+	int ret = 0;
+
+	tfm = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(tfm))
+		return -ENOMEM;
+
+	req = ahash_request_alloc(tfm, GFP_ATOMIC);
+	if (!req) {
+		ret = -ENOMEM;
+		goto free_ahash;
+	}
+
+	sg_init_one(&sg, (char *)map, sizeof(struct e820map));
+	ahash_request_set_callback(req, 0, NULL, NULL);
+	ahash_request_set_crypt(req, &sg, (char *)buf, sizeof(struct e820map));
+
+	if (crypto_ahash_digest(req))
+		ret = -EINVAL;
+
+	ahash_request_free(req);
+ free_ahash:
+	crypto_free_ahash(tfm);
+
+	return ret;
+}
+
+static int hibernation_e820_save(void *buf)
+{
+	int ret;
+	char result[MD5_HASH_SIZE] = {0};
+
+	ret = get_e820_md5(&e820_saved, result);
+	if (ret)
+		return ret;
+
+	memcpy((char *)buf, result, MD5_HASH_SIZE);
+
+	return 0;
+}
+
+static int hibernation_e820_check(void *buf)
+{
+	int ret;
+	char result[MD5_HASH_SIZE] = {0};
+
+	ret = get_e820_md5(&e820_saved, result);
+	if (ret)
+		return ret;
+
+	if (memcmp(result, buf, MD5_HASH_SIZE))
+		e820_conflict = true;
+
+	return 0;
+}
+
+#else
+static int hibernation_e820_save(void *buf)
+{
+	return 0;
+}
+
+static int hibernation_e820_check(void *buf)
+{
+	return 0;
+}
+#endif
+
+static int arch_hibernation_die_check(struct notifier_block *nb,
+				      unsigned long action,
+				      void *data)
+{
+	if (e820_conflict) {
+		pr_err("PM: Hibernation Caution! Panic might be caused by inconsistent e820 table.\n");
+		pr_err("PM: Please update your BIOS, or do not use hibernation on this machine.\n");
+	}
+
+	return 0;
+}
+
+static struct notifier_block hibernation_notifier = {
+	.notifier_call = arch_hibernation_die_check,
+};
+
+static int __init arch_init_hibernation(void)
+{
+	int retval;
+
+	retval = register_die_notifier(&hibernation_notifier);
+	if (retval)
+		return retval;
+
+	return 0;
+}
+
+late_initcall(arch_init_hibernation);
+
 /**
  *	arch_hibernation_header_save - populate the architecture specific part
  *		of a hibernation image header
@@ -201,6 +318,14 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 	rdr->jump_address_phys = __pa_symbol(&restore_registers);
 	rdr->cr3 = restore_cr3;
 	rdr->magic = RESTORE_MAGIC;
+
+	/*
+	 * A page has been allocated previously to store the hibernation
+	 * image header, so we can safely store the md5 result behind
+	 * struct restore_data_record, with size of 128 bytes.
+	 */
+	hibernation_e820_save(addr + sizeof(struct restore_data_record));
+
 	return 0;
 }
 
@@ -216,5 +341,11 @@ int arch_hibernation_header_restore(void *addr)
 	restore_jump_address = rdr->jump_address;
 	jump_address_phys = rdr->jump_address_phys;
 	restore_cr3 = rdr->cr3;
+
+	/*
+	 * Check if suspend e820 map is the same with the resume e820 map.
+	 */
+	hibernation_e820_check(addr + sizeof(struct restore_data_record));
+
 	return (rdr->magic == RESTORE_MAGIC) ? 0 : -EINVAL;
 }
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 68d3ebc..f0ba5e7 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -76,6 +76,15 @@ config HIBERNATION
 
 	  For more information take a look at <file:Documentation/power/swsusp.txt>.
 
+config HIBERNATION_CHECK_E820
+	bool "Check the consistence of e820 map across hibernation"
+	depends on ARCH_HIBERNATION_HEADER
+	---help---
+	  Check if the resume kernel has the same BIOS-provided
+	  e820 memory map as the one in suspend kernel(requested
+	  by ACPI spec), by comparing the md5 digest of the two e820
+	  regions.
+
 config ARCH_SAVE_PAGE_KEYS
 	bool
 
-- 
2.7.4

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Pavel Machek]

Hi!

> > > > What's the progress of this patch? Looks already have experts review it.
> > > > Why this patch didn't accept?
> > > This patch is a little overkilled, and I have saved another simpler
> > > version to only check the md5 hash (as people suggested) for it. I can post it later.
> > > 
> > 
> > I am happy to test and review it.
> >
> Here it is. As Rafael is on travel, it would be grateful
> if you can give some advance on this, thanks!

Better than last one.

> +#ifdef CONFIG_HIBERNATION_CHECK_E820
> +#define MD5_HASH_SIZE 128
> +
> +/**
> + *	get_e820_md5 - calculate md5 according to struct e820map
> + *
> + *	@map: the e820 map to be calculated
> + *	@buf: the md5 result to be stored to
> + */
> +static int get_e820_md5(struct e820map *map, void *buf)
> +{
> +	struct scatterlist sg;
> +	struct crypto_ahash *tfm;
> +	struct ahash_request *req;
> +	int ret = 0;
> +
> +	tfm = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
> +	if (IS_ERR(tfm))
> +		return -ENOMEM;
> +
> +	req = ahash_request_alloc(tfm, GFP_ATOMIC);

what context is this called from? GFP_ATOMIC allocations like to fail...

> +static int hibernation_e820_check(void *buf)
> +{
> +	int ret;
> +	char result[MD5_HASH_SIZE] = {0};
> +
> +	ret = get_e820_md5(&e820_saved, result);
> +	if (ret)
> +		return ret;
> +
> +	if (memcmp(result, buf, MD5_HASH_SIZE))
> +		e820_conflict = true;

Passing return value using global variable is ugly. Can you just print
the warning and kill the box here?
> +

> +	/*
> +	 * A page has been allocated previously to store the hibernation
> +	 * image header, so we can safely store the md5 result behind
> +	 * struct restore_data_record, with size of 128 bytes.
> +	 */
> +	hibernation_e820_save(addr + sizeof(struct restore_data_record));
> +

Please just allocate space in struct restore_data_record . And I don't
think md5 sum is 128 _bytes_.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

Hi,
On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > What's the progress of this patch? Looks already have experts review it.
> > > > > Why this patch didn't accept?
> > > > This patch is a little overkilled, and I have saved another simpler
> > > > version to only check the md5 hash (as people suggested) for it. I can post it later.
> > > > 
> > > 
> > > I am happy to test and review it.
> > >
> > Here it is. As Rafael is on travel, it would be grateful
> > if you can give some advance on this, thanks!
> 
> Better than last one.
> 
> > +		return -ENOMEM;
> > +
> > +	req = ahash_request_alloc(tfm, GFP_ATOMIC);
> 
> what context is this called from? GFP_ATOMIC allocations like to fail...
>
It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > +static int hibernation_e820_check(void *buf)
> > +{
> > +	int ret;
> > +	char result[MD5_HASH_SIZE] = {0};
> > +
> > +	ret = get_e820_md5(&e820_saved, result);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (memcmp(result, buf, MD5_HASH_SIZE))
> > +		e820_conflict = true;
> 
> Passing return value using global variable is ugly. Can you just print
> the warning and kill the box here?
Do you mean get rid of the panic hooker and just print the warning here?
> > +
> 
> > +	/*
> > +	 * A page has been allocated previously to store the hibernation
> > +	 * image header, so we can safely store the md5 result behind
> > +	 * struct restore_data_record, with size of 128 bytes.
> > +	 */
> > +	hibernation_e820_save(addr + sizeof(struct restore_data_record));
> > +
> 
> Please just allocate space in struct restore_data_record . And I don't
> think md5 sum is 128 _bytes_.
>
OK. The md5 sum should be 128 bits thus 16 bytes.

Thanks!
Yu 

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Pavel Machek]

On Sun 2016-08-28 10:07:10, Chen Yu wrote:
> Hi,
> On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > What's the progress of this patch? Looks already have experts review it.
> > > > > > Why this patch didn't accept?
> > > > > This patch is a little overkilled, and I have saved another simpler
> > > > > version to only check the md5 hash (as people suggested) for it. I can post it later.
> > > > > 
> > > > 
> > > > I am happy to test and review it.
> > > >
> > > Here it is. As Rafael is on travel, it would be grateful
> > > if you can give some advance on this, thanks!
> > 
> > Better than last one.
> > 
> > > +		return -ENOMEM;
> > > +
> > > +	req = ahash_request_alloc(tfm, GFP_ATOMIC);
> > 
> > what context is this called from? GFP_ATOMIC allocations like to fail...
> >
> It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > > +static int hibernation_e820_check(void *buf)
> > > +{
> > > +	int ret;
> > > +	char result[MD5_HASH_SIZE] = {0};
> > > +
> > > +	ret = get_e820_md5(&e820_saved, result);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (memcmp(result, buf, MD5_HASH_SIZE))
> > > +		e820_conflict = true;
> > 
> > Passing return value using global variable is ugly. Can you just print
> > the warning and kill the box here?
> Do you mean get rid of the panic hooker and just print the warning
> here?

Yep, I'd do that... (And you probably want to rise the severity).

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen, Yu C]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Sunday, August 28, 2016 8:48 PM
> To: Chen, Yu C
> Cc: joeyli; Rafael J. Wysocki; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when
> resuming with inconsistent e820 map
> 
> On Sun 2016-08-28 10:07:10, Chen Yu wrote:
> > Hi,
> > On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> > > Hi!
> > >
> > > > > > > What's the progress of this patch? Looks already have experts
> review it.
> > > > > > > Why this patch didn't accept?
> > > > > > This patch is a little overkilled, and I have saved another
> > > > > > simpler version to only check the md5 hash (as people suggested) for
> it. I can post it later.
> > > > > >
> > > > >
> > > > > I am happy to test and review it.
> > > > >
> > > > Here it is. As Rafael is on travel, it would be grateful if you
> > > > can give some advance on this, thanks!
> > >
> > > Better than last one.
> > >
> > > > +		return -ENOMEM;
> > > > +
> > > > +	req = ahash_request_alloc(tfm, GFP_ATOMIC);
> > >
> > > what context is this called from? GFP_ATOMIC allocations like to fail...
> > >
> > It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > > > +static int hibernation_e820_check(void *buf) {
> > > > +	int ret;
> > > > +	char result[MD5_HASH_SIZE] = {0};
> > > > +
> > > > +	ret = get_e820_md5(&e820_saved, result);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (memcmp(result, buf, MD5_HASH_SIZE))
> > > > +		e820_conflict = true;
> > >
> > > Passing return value using global variable is ugly. Can you just
> > > print the warning and kill the box here?
> > Do you mean get rid of the panic hooker and just print the warning
> > here?
> 
> Yep, I'd do that... (And you probably want to rise the severity).
Sometime the users might not be able to see the warning during resume(too fast)
and got a system panic later, so I was thinking if it might help user to see the warning
in panic too, how about this:

if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
	pr_err("PM: e820 map conflict detected!\n");
	register_die_notifier(&hibernation_die_notifier);
} 
So we can print warning in hibernation_die_notifier without introducing a global variable?

Thanks,
Yu

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Pavel Machek]

On Sun 2016-08-28 13:08:02, Chen, Yu C wrote:
> > -----Original Message-----
> > From: Pavel Machek [mailto:pavel@ucw.cz]
> > Sent: Sunday, August 28, 2016 8:48 PM
> > To: Chen, Yu C
> > Cc: joeyli; Rafael J. Wysocki; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when
> > resuming with inconsistent e820 map
> > 
> > On Sun 2016-08-28 10:07:10, Chen Yu wrote:
> > > Hi,
> > > On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> > > > Hi!
> > > >
> > > > > > > > What's the progress of this patch? Looks already have experts
> > review it.
> > > > > > > > Why this patch didn't accept?
> > > > > > > This patch is a little overkilled, and I have saved another
> > > > > > > simpler version to only check the md5 hash (as people suggested) for
> > it. I can post it later.
> > > > > > >
> > > > > >
> > > > > > I am happy to test and review it.
> > > > > >
> > > > > Here it is. As Rafael is on travel, it would be grateful if you
> > > > > can give some advance on this, thanks!
> > > >
> > > > Better than last one.
> > > >
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	req = ahash_request_alloc(tfm, GFP_ATOMIC);
> > > >
> > > > what context is this called from? GFP_ATOMIC allocations like to fail...
> > > >
> > > It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > > > > +static int hibernation_e820_check(void *buf) {
> > > > > +	int ret;
> > > > > +	char result[MD5_HASH_SIZE] = {0};
> > > > > +
> > > > > +	ret = get_e820_md5(&e820_saved, result);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (memcmp(result, buf, MD5_HASH_SIZE))
> > > > > +		e820_conflict = true;
> > > >
> > > > Passing return value using global variable is ugly. Can you just
> > > > print the warning and kill the box here?
> > > Do you mean get rid of the panic hooker and just print the warning
> > > here?
> > 
> > Yep, I'd do that... (And you probably want to rise the severity).
> Sometime the users might not be able to see the warning during resume(too fast)
> and got a system panic later, so I was thinking if it might help user to see the warning
> in panic too, how about this:
> 
> if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
> 	pr_err("PM: e820 map conflict detected!\n");
> 	register_die_notifier(&hibernation_die_notifier);
> } 
> So we can print warning in hibernation_die_notifier without introducing a global variable?
> 

Actually, I'd kill the machine right away.

if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
	pr_err("PM: e820 map conflict detected!\n");
	panic("BIOS is playing funny tricks with us.\n");
} 

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen, Yu C]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Sunday, August 28, 2016 9:15 PM
> To: Chen, Yu C
> Cc: joeyli; Rafael J. Wysocki; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when
> resuming with inconsistent e820 map
> 
> On Sun 2016-08-28 13:08:02, Chen, Yu C wrote:
> > > -----Original Message-----
> > > From: Pavel Machek [mailto:pavel@ucw.cz]
> > > Sent: Sunday, August 28, 2016 8:48 PM
> > > To: Chen, Yu C
> > > Cc: joeyli; Rafael J. Wysocki; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH][v6] PM / hibernate: Print the possible panic
> > > reason when resuming with inconsistent e820 map
> > >
> > > On Sun 2016-08-28 10:07:10, Chen Yu wrote:
> > > > Hi,
> > > > On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> > > > > Hi!
> > > > >
> > > > > > > > > What's the progress of this patch? Looks already have
> > > > > > > > > experts
> > > review it.
> > > > > > > > > Why this patch didn't accept?
> > > > > > > > This patch is a little overkilled, and I have saved
> > > > > > > > another simpler version to only check the md5 hash (as
> > > > > > > > people suggested) for
> > > it. I can post it later.
> > > > > > > >
> > > > > > >
> > > > > > > I am happy to test and review it.
> > > > > > >
> > > > > > Here it is. As Rafael is on travel, it would be grateful if
> > > > > > you can give some advance on this, thanks!
> > > > >
> > > > > Better than last one.
> > > > >
> > > > > > +		return -ENOMEM;
> > > > > > +
> > > > > > +	req = ahash_request_alloc(tfm, GFP_ATOMIC);
> > > > >
> > > > > what context is this called from? GFP_ATOMIC allocations like to fail...
> > > > >
> > > > It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > > > > > +static int hibernation_e820_check(void *buf) {
> > > > > > +	int ret;
> > > > > > +	char result[MD5_HASH_SIZE] = {0};
> > > > > > +
> > > > > > +	ret = get_e820_md5(&e820_saved, result);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	if (memcmp(result, buf, MD5_HASH_SIZE))
> > > > > > +		e820_conflict = true;
> > > > >
> > > > > Passing return value using global variable is ugly. Can you just
> > > > > print the warning and kill the box here?
> > > > Do you mean get rid of the panic hooker and just print the warning
> > > > here?
> > >
> > > Yep, I'd do that... (And you probably want to rise the severity).
> > Sometime the users might not be able to see the warning during
> > resume(too fast) and got a system panic later, so I was thinking if it
> > might help user to see the warning in panic too, how about this:
> >
> > if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
> > 	pr_err("PM: e820 map conflict detected!\n");
> > 	register_die_notifier(&hibernation_die_notifier);
> > }
> > So we can print warning in hibernation_die_notifier without introducing a
> global variable?
> >
> 
> Actually, I'd kill the machine right away.
> 
> if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
> 	pr_err("PM: e820 map conflict detected!\n");
> 	panic("BIOS is playing funny tricks with us.\n");
> }
> 
OK, this is more reasonable, will take this one.

Thanks,
Yu

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Andreas Mohr]

Hi,

[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]

> > So we can print warning in hibernation_die_notifier without
> > introducing a global variable?
> > 
> 
> Actually, I'd kill the machine right away.
> 
> if (memcmp(result, buf, MD5_DIGEST_SIZE)) {
> 	pr_err("PM: e820 map conflict detected!\n");
> 	panic("BIOS is playing funny tricks with us.\n");
> } 
> Best regards,
> 								Pavel

+1.

I would tend to think that
it's rather preferable to
kill an affected environment scope
(in this case: whole system), hard
(except for perhaps some emergency epilogue state saving),
in case of its state having become
suspicious/unpredictable/dangerous,
rather than having things carry on in a merry-go-round manner and thus
making improper state progress
which translates into
*continued activity* of *corrupting things*
(possibly even leading to *persistent* i.e. storage-recorded corruption!)
willy-nilly.

Plus, killing a machine hard in such a questionable case
would increase our influx of valuable reports
due to users being
hard-pressed to report this rather than
silently ignoring / not even knowing this issue
(in those cases where we already know that
no further development fixes will be determinable,
carrying on with a dire warning probably still is preferable to
making the machine completely unusable, eternally,
though).

Thus, think robustness.

Andreas Mohr