diff mbox

[RFC] x86, hotplug: Use zero page for monitor when resuming from hibernation

Message ID 1465222749-8388-1-git-send-email-yu.c.chen@intel.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

Chen Yu June 6, 2016, 2:19 p.m. UTC
Stress test from Varun Koyyalagunta reports that, the
nonboot CPU would hang occasionally, when resuming from
hibernation. Further investigation shows that, the precise
phase when nonboot CPU hangs, is the time when the nonboot
CPU been woken up incorrectly, and tries to monitor the
mwait_ptr for the second time, then an exception is
triggered due to illegal vaddr access, say, something like,
'Unable to handler kernel address of 0xffff8800ba800010...'

One of the possible scenarios for this issue is illustrated below,
when the boot CPU tries to resume from hibernation:
1. puts the nonboot CPUs offline, so the nonboot CPUs are monitoring
   at the address of the task_struct.flags.
2. boot CPU copies pages to their original address, which includes
   task_struct.flags, thus wakes up one of the nonboot CPUs.
3. nonboot CPU tries to monitor the task_struct.flags again, but since
   the page table for task_struct.flags has been overwritten by
   boot CPU, and there is probably a changed across hibernation
   (because of inconsistence of e820 memory map), an exception is
   triggered.

As suggested by Rafael and Len, this patch tries to monitor a zero
page instead of task_struct.flags, if it comes from hibernation resume
process. The zero page should be safe because it is located in .bss
and page table for kernel mapping of text/data/bss should keeps
unchanged according to hibernation semantic.

Reported-by: Varun Koyyalagunta <cpudebug@centtech.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=106371
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 arch/x86/kernel/smpboot.c | 16 +++++++++++++++-
 include/linux/suspend.h   |  7 +++++++
 kernel/power/hibernate.c  |  3 +++
 3 files changed, 25 insertions(+), 1 deletion(-)

Comments

Peter Zijlstra June 6, 2016, 2:25 p.m. UTC | #1
On Mon, Jun 06, 2016 at 10:19:09PM +0800, Chen Yu wrote:
> @@ -1595,8 +1596,21 @@ static inline void mwait_play_dead(void)
>  	 * This should be a memory location in a cache line which is
>  	 * unlikely to be touched by other processors.  The actual
>  	 * content is immaterial as it is not actually modified in any way.
> +	 *
> +	 * However in hibernation resume process, this address could be
> +	 * touched by BSP when restoring page frames, if the page table
> +	 * for this address is not coherent across hibernation(due to
> +	 * inconsistence of e820 memory map), access from APs might
> +	 * cause exception. So change the mwait address to zero page,
> +	 * which is located in .bss, in this way we can avoid illegal
> +	 * access from APs because page table for kernel mapping
> +	 * of text/data/bss should keeps unchanged according to
> +	 * hibernation semantic.
>  	 */
> -	mwait_ptr = &current_thread_info()->flags;
> +	if (hibernation_in_resume())
> +		mwait_ptr = empty_zero_page;
> +	else
> +		mwait_ptr = &current_thread_info()->flags;

Why is this conditional? Is there any case in which the zero page is not
also correct?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Yu June 6, 2016, 3:59 p.m. UTC | #2
Hi Peter,

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Monday, June 06, 2016 10:25 PM
> To: Chen, Yu C
> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; Thomas Gleixner; Ingo
> Molnar; H . Peter Anvin; linux-pm@vger.kernel.org; Rafael J . Wysocki; Pavel
> Machek; Brown, Len; Borislav Petkov; Zhu Guihua; Juergen Gross
> Subject: Re: [PATCH][RFC] x86, hotplug: Use zero page for monitor when
> resuming from hibernation
> 
> On Mon, Jun 06, 2016 at 10:19:09PM +0800, Chen Yu wrote:
> > @@ -1595,8 +1596,21 @@ static inline void mwait_play_dead(void)
> >  	 * This should be a memory location in a cache line which is
> >  	 * unlikely to be touched by other processors.  The actual
> >  	 * content is immaterial as it is not actually modified in any way.
> > +	 *
> > +	 * However in hibernation resume process, this address could be
> > +	 * touched by BSP when restoring page frames, if the page table
> > +	 * for this address is not coherent across hibernation(due to
> > +	 * inconsistence of e820 memory map), access from APs might
> > +	 * cause exception. So change the mwait address to zero page,
> > +	 * which is located in .bss, in this way we can avoid illegal
> > +	 * access from APs because page table for kernel mapping
> > +	 * of text/data/bss should keeps unchanged according to
> > +	 * hibernation semantic.
> >  	 */
> > -	mwait_ptr = &current_thread_info()->flags;
> > +	if (hibernation_in_resume())
> > +		mwait_ptr = empty_zero_page;
> > +	else
> > +		mwait_ptr = &current_thread_info()->flags;
> 
> Why is this conditional? Is there any case in which the zero page is not also
> correct?
I'm thinking of avoid unnecessary wakeup for normal CPU offline,  for example, 
if one  driver uses  the zero page and access it.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra June 6, 2016, 4:40 p.m. UTC | #3
On Mon, Jun 06, 2016 at 03:59:06PM +0000, Chen, Yu C wrote:

> > > +	if (hibernation_in_resume())
> > > +		mwait_ptr = empty_zero_page;
> > > +	else
> > > +		mwait_ptr = &current_thread_info()->flags;
> > 
> > Why is this conditional? Is there any case in which the zero page is not also
> > correct?
> I'm thinking of avoid unnecessary wakeup for normal CPU offline,  for example, 
> if one  driver uses  the zero page and access it.

Writing to the zero page would be a major fail.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Gerst June 6, 2016, 5:34 p.m. UTC | #4
On Mon, Jun 6, 2016 at 12:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jun 06, 2016 at 03:59:06PM +0000, Chen, Yu C wrote:
>
>> > > + if (hibernation_in_resume())
>> > > +         mwait_ptr = empty_zero_page;
>> > > + else
>> > > +         mwait_ptr = &current_thread_info()->flags;
>> >
>> > Why is this conditional? Is there any case in which the zero page is not also
>> > correct?
>> I'm thinking of avoid unnecessary wakeup for normal CPU offline,  for example,
>> if one  driver uses  the zero page and access it.
>
> Writing to the zero page would be a major fail.

I would think the safest thing to do during resume from hibernation is
to use hlt instead of mwait, so there is no dependency on any memory
address.  It doesn't need the power management features of mwait
either because the CPU will be reset soon after the restored kernel
resumes.

--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
H. Peter Anvin June 6, 2016, 9:05 p.m. UTC | #5
On 06/06/16 09:40, Peter Zijlstra wrote:
> On Mon, Jun 06, 2016 at 03:59:06PM +0000, Chen, Yu C wrote:
> 
>>>> +	if (hibernation_in_resume())
>>>> +		mwait_ptr = empty_zero_page;
>>>> +	else
>>>> +		mwait_ptr = &current_thread_info()->flags;
>>>
>>> Why is this conditional? Is there any case in which the zero page is not also
>>> correct?
>> I'm thinking of avoid unnecessary wakeup for normal CPU offline,  for example, 
>> if one  driver uses  the zero page and access it.
> 
> Writing to the zero page would be a major fail.
> 

One implementation of MWAIT is for the waiting processor to claim the
line in an exclusive state, which would be highly undesirable for the
zero page.  I don't actually know if this is done by any real
implementation, however.

Either way, the problem at hand seems limited to post-hibernation, as
for regular CPU offline the page tables will be fully populated, so it
seems like it should not be an issue.

	-hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek June 7, 2016, 8:03 a.m. UTC | #6
On Mon 2016-06-06 22:19:09, Chen Yu wrote:
> Stress test from Varun Koyyalagunta reports that, the
> nonboot CPU would hang occasionally, when resuming from
> hibernation. Further investigation shows that, the precise
> phase when nonboot CPU hangs, is the time when the nonboot
> CPU been woken up incorrectly, and tries to monitor the
> mwait_ptr for the second time, then an exception is
> triggered due to illegal vaddr access, say, something like,
> 'Unable to handler kernel address of 0xffff8800ba800010...'
> 
> One of the possible scenarios for this issue is illustrated below,
> when the boot CPU tries to resume from hibernation:
> 1. puts the nonboot CPUs offline, so the nonboot CPUs are monitoring
>    at the address of the task_struct.flags.
> 2. boot CPU copies pages to their original address, which includes
>    task_struct.flags, thus wakes up one of the nonboot CPUs.
> 3. nonboot CPU tries to monitor the task_struct.flags again, but since
>    the page table for task_struct.flags has been overwritten by
>    boot CPU, and there is probably a changed across hibernation
>    (because of inconsistence of e820 memory map), an exception is
>    triggered.

If memory map changes between suspend and resume, there'll be fun. If
that's suspected, should we attach md5 sum of e820 to the hibernation
image?

I doubt mwait fix is enough to catch everything in that case...

Best regards,
									Pavel
Chen Yu June 7, 2016, 8:44 a.m. UTC | #7
Hi Pavel,

On 2016年06月07日 16:03, Pavel Machek wrote:
> On Mon 2016-06-06 22:19:09, Chen Yu wrote:
>> Stress test from Varun Koyyalagunta reports that, the
>> nonboot CPU would hang occasionally, when resuming from
>> hibernation. Further investigation shows that, the precise
>> phase when nonboot CPU hangs, is the time when the nonboot
>> CPU been woken up incorrectly, and tries to monitor the
>> mwait_ptr for the second time, then an exception is
>> triggered due to illegal vaddr access, say, something like,
>> 'Unable to handler kernel address of 0xffff8800ba800010...'
>>
>> One of the possible scenarios for this issue is illustrated below,
>> when the boot CPU tries to resume from hibernation:
>> 1. puts the nonboot CPUs offline, so the nonboot CPUs are monitoring
>>     at the address of the task_struct.flags.
>> 2. boot CPU copies pages to their original address, which includes
>>     task_struct.flags, thus wakes up one of the nonboot CPUs.
>> 3. nonboot CPU tries to monitor the task_struct.flags again, but since
>>     the page table for task_struct.flags has been overwritten by
>>     boot CPU, and there is probably a changed across hibernation
>>     (because of inconsistence of e820 memory map), an exception is
>>     triggered.
> If memory map changes between suspend and resume, there'll be fun. If
> that's suspected, should we attach md5 sum of e820 to the hibernation
> image?
Actually what I described  in the  scenario might be not so accurate,
it might not be related to inconsistence of e820 map,
because there is no guarantee that boot kernel and resume kernel
have the same memory layout(page table).

I've re-checked the logs from reporter, it seems that, the fault
access is caused by accessing a page without PRESENT flag,
and the pte entry for this vaddr is zero:

    // DATA ADDRESS TRANSLATION, virtual address = 0xFFFF8800BA803E88
    //   DATA TABLE WALK, virtual address = 0xFFFF8800BA803E88
    pml4read   0x0001C12880    0x01FD2067; // pgd
    pdptread   0x0001FD2010    0x9BEBF063; // pud
    pderead    0x009BEBFEA0    0x2D9D9063; // pmd
    pteread    0x002D9D9018    0x00000000; // pte

The last line above is a pte entry located at physical address 0x2d9d9018,
with value zero, thus access to this vaddr results in a page-not-present exception.


Since some of the pud/pde/pte are allocated dynamically during bootup
(kernel_physical_mapping_init),
it is possible that, when the boot cpu writes to this vaddr,
the page table(especially for pud/pmd/pte) for this vaddr
are not the same as it is before hibernation,  thus an exception would be
triggered due to incorrect page table, even e820 is consistent.

I'm doing more test to verify this.

thanks,
Yu

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov June 7, 2016, 9:13 a.m. UTC | #8
On Tue, Jun 07, 2016 at 04:44:24PM +0800, Chen Yu wrote:
> I'm doing more test to verify this.

I think it would be better if you looked into doing HLT as Brian
suggested, instead of touching any memory during s/r.
Chen Yu June 7, 2016, 9:43 a.m. UTC | #9
On 2016年06月07日 17:13, Borislav Petkov wrote:
> On Tue, Jun 07, 2016 at 04:44:24PM +0800, Chen Yu wrote:
>> I'm doing more test to verify this.
> I think it would be better if you looked into doing HLT as Brian
> suggested, instead of touching any memory during s/r.
>
OK, I'll also test this solution. thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fafe8b9..b2732ae 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -53,6 +53,7 @@ 
 #include <linux/stackprotector.h>
 #include <linux/gfp.h>
 #include <linux/cpuidle.h>
+#include <linux/suspend.h>
 
 #include <asm/acpi.h>
 #include <asm/desc.h>
@@ -1595,8 +1596,21 @@  static inline void mwait_play_dead(void)
 	 * This should be a memory location in a cache line which is
 	 * unlikely to be touched by other processors.  The actual
 	 * content is immaterial as it is not actually modified in any way.
+	 *
+	 * However in hibernation resume process, this address could be
+	 * touched by BSP when restoring page frames, if the page table
+	 * for this address is not coherent across hibernation(due to
+	 * inconsistence of e820 memory map), access from APs might
+	 * cause exception. So change the mwait address to zero page,
+	 * which is located in .bss, in this way we can avoid illegal
+	 * access from APs because page table for kernel mapping
+	 * of text/data/bss should keeps unchanged according to
+	 * hibernation semantic.
 	 */
-	mwait_ptr = &current_thread_info()->flags;
+	if (hibernation_in_resume())
+		mwait_ptr = empty_zero_page;
+	else
+		mwait_ptr = &current_thread_info()->flags;
 
 	wbinvd();
 
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 8b6ec7e..422e87a 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -384,6 +384,12 @@  extern bool system_entering_hibernation(void);
 extern bool hibernation_available(void);
 asmlinkage int swsusp_save(void);
 extern struct pbe *restore_pblist;
+extern bool in_resume_hibernate;
+
+static inline bool hibernation_in_resume(void)
+{
+	return in_resume_hibernate;
+}
 #else /* CONFIG_HIBERNATION */
 static inline void register_nosave_region(unsigned long b, unsigned long e) {}
 static inline void register_nosave_region_late(unsigned long b, unsigned long e) {}
@@ -395,6 +401,7 @@  static inline void hibernation_set_ops(const struct platform_hibernation_ops *op
 static inline int hibernate(void) { return -ENOSYS; }
 static inline bool system_entering_hibernation(void) { return false; }
 static inline bool hibernation_available(void) { return false; }
+static inline bool hibernation_in_resume(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
 /* Hibernation and suspend events */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254..13c229a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -43,6 +43,7 @@  static char resume_file[256] = CONFIG_PM_STD_PARTITION;
 dev_t swsusp_resume_device;
 sector_t swsusp_resume_block;
 __visible int in_suspend __nosavedata;
+bool in_resume_hibernate;
 
 enum {
 	HIBERNATION_INVALID,
@@ -433,7 +434,9 @@  static int resume_target_kernel(bool platform_mode)
 	if (error)
 		goto Cleanup;
 
+	in_resume_hibernate = true;
 	error = disable_nonboot_cpus();
+	in_resume_hibernate = false;
 	if (error)
 		goto Enable_cpus;