Re: Fwd: [Bug 150021] New: kernel panic: "kernel tried to execute NX-protected page" when resuming from hibernate to disk

[Rafael J. Wysocki]

On Wednesday, July 27, 2016 12:59:18 PM Josh Poimboeuf wrote:
> On Wed, Jul 27, 2016 at 01:08:21AM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jul 27, 2016 at 12:42 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > > On Tuesday, July 26, 2016 04:53:19 PM Josh Poimboeuf wrote:
> > >> On Tue, Jul 26, 2016 at 10:15:39PM +0200, Rafael J. Wysocki wrote:
> > >> > On Tuesday, July 26, 2016 09:39:05 AM Josh Poimboeuf wrote:
> > >> > > On Tue, Jul 26, 2016 at 01:32:28PM +0200, Rafael J. Wysocki wrote:
> > >> > > > Hi,
> > >> > > >
> > >> > > > The following commit:
> > >> > > >
> > >> > > > commit 13523309495cdbd57a0d344c0d5d574987af007f
> > >> > > > Author: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > >> > > > Date:   Thu Jan 21 16:49:21 2016 -0600
> > >> > > >
> > >> > > >     x86/asm/acpi: Create a stack frame in do_suspend_lowlevel()
> > >> > > >
> > >> > > >     do_suspend_lowlevel() is a callable non-leaf function which doesn't
> > >> > > >     honor CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > >> > > >
> > >> > > >     Create a stack frame for it when CONFIG_FRAME_POINTER is enabled.
> > >> > > >
> > >> > > > is reported to cause a resume-from-hibernation regression due to an attempt
> > >> > > > to execute an NX page (we've seen quite a bit of that recently).
> > >> > > >
> > >> > > > I'm asking the reporter to try 4.7, but if the problem is still there, we'll
> > >> > > > need to revert the above I'm afraid.
> > >> >
> > >> > So the bug is still there in 4.7 and it goes away after reverting the above
> > >> > commit.  I guess I'll send a revert then.
> > >>
> > >> Hm, the code in wakeup_64.S seems quite magical, but I can't figure out
> > >> why this change causes a panic.  Is it really causing the panic or is it
> > >> uncovering some other bug?
> > >
> > > It doesn't matter really.
> > >
> > > It surely interacts with something in a really odd way, but that only means
> > > that its impact goes far beyond what was expected when it was applied.  Its
> > > changelog is inadequate as a result and so on.
> > >
> > >> Maybe we should hold off on reverting until we understand the issue.
> > >
> > > Which very well may take forever.
> > >
> > > And AFAICS this is a fix for a theoretical issue and it *reliably* triggers a
> > > very practical kernel panic for this particular reporter.  I'd rather live
> > > with the theoretical issue unfixed to be honest.
> > 
> > Well, actually, the best part is that do_suspend_lowlevel() is not
> > even called during hibernation or resume from it.  It only is called
> > during suspend-to-RAM.
> > 
> > Question now is how the change made by the commit in question can
> > affect hibernation which is an unrelated code path.  We know for a
> > fact that it does affect it, but how?
> 
> Hm... I have a theory, but I'm not sure about it.  I noticed that
> x86_acpi_enter_sleep_state(),

I think you mean x86_acpi_suspend_lowlevel().

> which is involved in suspend, overwrites
> several global variables (e.g, initial_code) which are used by the CPU
> boot code in head_64.S.  But surprisingly, it doesn't restore those
> variables to their original values after it resumes.

Is the head_64.S code also used to bring up offline CPUs?

If not, then this is not the problem, because hibernation doesn't use it
for the boot CPU anyway.

> So if a suspend and resume were done before the hibernate, those
> variables would presumably have suspend-centric values, and the first
> time a CPU is brought up during the hibernation restore operation, it
> would jump to wakeup_long64() (the suspend resume function) instead of
> start_secondary (which is the normal CPU boot function).
> 
> So, if true, that would explain why my patch triggers a bug:
> wakeup_long64() always[*] jumps to .Lresume_point, which my patch
> affected.  Because of the FRAME_END, it would pop an extra value off the
> stack.  So when restore_processor_state() returns, it would return to
> whatever random address is on the stack after the real RIP.  Which is
> consistent with the oops from the bug.  It had a bad instruction
> pointer, which looked like a stack address.

OK, so why doesn't it break resume from suspend to RAM?  wakeup_long64 is
invoked by the CPU startup code then and doesn't the FRAME_END affect
that too?

> But then again, maybe there's a hole in that theory, because how could
> hibernate after suspend/resume possibly even work today if the CPU boot
> goes to wakeup_long64() instead of start_secondary?

Right.

> So I could be missing something, or even completely off base.  But the
> missing restore of those variables does seem like a pretty huge
> oversight.  I wonder if the following patch would fix it?

We'll need to ask the reporter. :-)

> 
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index adb3eaf..cd76fc5 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -45,6 +45,12 @@ acpi_status asmlinkage __visible x86_acpi_enter_sleep_state(u8 state)
>   */
>  int x86_acpi_suspend_lowlevel(void)
>  {
> +#ifdef CONFIG_64BIT
> +	unsigned long prev_initial_code;
> +#ifdef CONFIG_SMP
> +	unsigned long prev_stack_start, prev_gdt_address, prev_initial_gs;
> +#endif
> +#endif
>  	struct wakeup_header *header =
>  		(struct wakeup_header *) __va(real_mode_header->wakeup_header);
>  
> @@ -99,13 +105,18 @@ int x86_acpi_suspend_lowlevel(void)
>  	saved_magic = 0x12345678;
>  #else /* CONFIG_64BIT */
>  #ifdef CONFIG_SMP
> +	prev_stack_start = stack_start;
> +	prev_gdt_address = early_gdt_descr.address;
> +	prev_initial_gs = initial_gs;
> +
>  	stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
>  	early_gdt_descr.address =
>  			(unsigned long)get_cpu_gdt_table(smp_processor_id());
>  	initial_gs = per_cpu_offset(smp_processor_id());
>  #endif
> +	prev_initial_code = initial_code;
>  	initial_code = (unsigned long)wakeup_long64;
> -       saved_magic = 0x123456789abcdef0L;
> +	saved_magic = 0x123456789abcdef0L;
>  #endif /* CONFIG_64BIT */
>  
>  	/*
> @@ -115,6 +126,16 @@ int x86_acpi_suspend_lowlevel(void)
>  	pause_graph_tracing();
>  	do_suspend_lowlevel();
>  	unpause_graph_tracing();
> +
> +#ifdef CONFIG_64BIT
> +	initial_code = prev_initial_code;
> +#ifdef CONFIG_SMP
> +	initial_gs = prev_initial_gs;
> +	early_gdt_descr.address = prev_gdt_address;
> +	stack_start = prev_stack_start;
> +#endif
> +#endif
> +
>  	return 0;
>  }
>