| Message ID | 20160727175918.5uvo4p54iuhuw7ss@treble (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@rjwysocki.net> 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@redhat.com> > > >> > > > 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; > } > -- 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
On Thu, Jul 28, 2016 at 12:12:15AM +0200, Rafael J. Wysocki wrote: > On Wednesday, July 27, 2016 12:59:18 PM Josh Poimboeuf wrote: > > 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(). Oops! > > 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? Yes. > 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? Because for suspend to RAM, it enters suspend through do_suspend_lowlevel(), which has the FRAME_BEGIN which corresponds to .Lresume_point's FRAME_END. > wakeup_long64 is invoked by the CPU startup code then and doesn't the > FRAME_END affect that too? Yes, I would imagine that any CPU startup operation (after suspend/resume to RAM) would be affected.
On Thursday, July 28, 2016 12:12:15 AM Rafael J. Wysocki wrote: > 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@rjwysocki.net> 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@redhat.com> > > > >> > > > 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? Ah, I see. wakeup_long64 will restore RSP from saved_rsp and that points to the right address already. OK Thanks, Rafael -- 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
On Wednesday, July 27, 2016 05:17:38 PM Josh Poimboeuf wrote: > On Thu, Jul 28, 2016 at 12:12:15AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, July 27, 2016 12:59:18 PM Josh Poimboeuf wrote: > > > 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(). > > Oops! > > > > 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? > > Yes. OK So it is really interesting why and how that stuff works for everybody. Basically, CPU online should fail after a suspend-resume cycle, but it doesn't most of the time AFAICS. > > 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? > > Because for suspend to RAM, it enters suspend through > do_suspend_lowlevel(), which has the FRAME_BEGIN which corresponds to > .Lresume_point's FRAME_END. > > > wakeup_long64 is invoked by the CPU startup code then and doesn't the > > FRAME_END affect that too? > > Yes, I would imagine that any CPU startup operation (after > suspend/resume to RAM) would be affected. That would mean that your patch is needed anyway, wouldn't it? Thanks, Rafael -- 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
On Thursday, July 28, 2016 01:20:53 AM Rafael J. Wysocki wrote: > On Wednesday, July 27, 2016 05:17:38 PM Josh Poimboeuf wrote: > > On Thu, Jul 28, 2016 at 12:12:15AM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, July 27, 2016 12:59:18 PM Josh Poimboeuf wrote: > > > > 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(). > > > > Oops! > > > > > > 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? > > > > Yes. > > OK > > So it is really interesting why and how that stuff works for everybody. > > Basically, CPU online should fail after a suspend-resume cycle, but it > doesn't most of the time AFAICS. do_boot_cpu() restores those values, so I think we're safe from that angle. That should apply to the CPU online during resume from hibernation too. Thanks, Rafael -- 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 --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; }