On Thu, Jun 11, 2015 at 11:07 PM, Ingo Molnar <mi...@kernel.org> wrote: > > * Srinivas Pandruvada <srinivas.pandruv...@linux.intel.com> wrote: > >> Suspend to RAM process is returning to userspsace with DS set to KERNEL_DS >> after resume, this cause general protection fault. [...] > > But s2ram has no influence on 'returning to user-space'. So unless I'm missing > something this changelog makes no sense. > > Unfortunately the patch seems to be completely bogus as well: > >> [...] This is very difficult to reproduce, but this does happen on 32 bit >> systems. This crash is not reproduced after save/restore DS during calls to >> _save_processor_state/ __restore_processor_state respectively. >> >> Similar issue was reported in the past >> https://bugzilla.kernel.org/show_bug.cgi?id=61781, for which the root cause >> was not identified. >> >> Signed-off-by: Srinivas Pandruvada <srinivas.pandruv...@linux.intel.com> >> Reviewed-by: Andi Kleen <a...@linux.intel.com> >> --- >> arch/x86/include/asm/suspend_32.h | 2 +- >> arch/x86/power/cpu.c | 2 ++ >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/suspend_32.h >> b/arch/x86/include/asm/suspend_32.h >> index 552d6c9..3503d0b 100644 >> --- a/arch/x86/include/asm/suspend_32.h >> +++ b/arch/x86/include/asm/suspend_32.h >> @@ -11,7 +11,7 @@ >> >> /* image of the saved processor state */ >> struct saved_context { >> - u16 es, fs, gs, ss; >> + u16 ds, es, fs, gs, ss; >> unsigned long cr0, cr2, cr3, cr4; >> u64 misc_enable; >> bool misc_enable_saved; >> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c >> index 757678f..e0dfb01 100644 >> --- a/arch/x86/power/cpu.c >> +++ b/arch/x86/power/cpu.c >> @@ -79,6 +79,7 @@ static void __save_processor_state(struct saved_context >> *ctxt) >> * segment registers >> */ >> #ifdef CONFIG_X86_32 >> + savesegment(ds, ctxt->ds); >> savesegment(es, ctxt->es); >> savesegment(fs, ctxt->fs); >> savesegment(gs, ctxt->gs); >> @@ -198,6 +199,7 @@ static void notrace __restore_processor_state(struct >> saved_context *ctxt) >> * segment registers >> */ >> #ifdef CONFIG_X86_32 >> + loadsegment(ds, ctxt->ds); >> loadsegment(es, ctxt->es); >> loadsegment(fs, ctxt->fs); >> loadsegment(gs, ctxt->gs); > > So save_processor_state() is used by 32-bit s2ram in the following place: > > arch/x86/kernel/acpi/wakeup_32.S: call save_processor_state > > Other uses are: > > arch/x86/kernel/acpi/wakeup_64.S: call save_processor_state > arch/x86/kernel/apm_32.c: save_processor_state(); > arch/x86/kernel/machine_kexec_32.c: save_processor_state(); > arch/x86/kernel/machine_kexec_64.c: save_processor_state(); > arch/x86/platform/olpc/xo1-wakeup.S: call save_processor_state > kernel/power/hibernate.c: save_processor_state(); > kernel/power/hibernate.c: save_processor_state(); > > but neither of these call sites seems to matter to the bug: the 32-bit system > does > not use APM, kexec, is not an OLPC and does not use hibernation. > > And if we look at arch/x86/kernel/acpi/wakeup_32.S it's a straightforward full > state save/restore before ACPI (BIOS) downcalls. > > Furthermore, the bug report in: > > https://bugzilla.kernel.org/show_bug.cgi?id=61781 > > talks about SIGSEGVs, and claims that this regression triggers in v3.11 but > does > not trigger in v3.10. > > 1) > > So the first critical question is: if the ACPI/BIOS suspend code corrupts the > kernel's DS, how can we get so far as to resume fully, return to user-space, > and > segfault there so that it can all be reported? > > So neither the explanation nor the code makes any sense in the context of the > reported bugs. Can anyone else offer any plausible theory about why this patch > would fix 32-bit user-space segfaults? > > 2) > > Btw., I don't mind the idea of the patch itself per se: saving/restoring DS is > prudent to avoid the BIOS stomping on our DS. > > But the restoration done in this patch is bogus, it's done way too late in a C > function, there's a number of places where we might use the kernel DS before > it's > restored, such as restore_registers(): > > restore_registers: > movl saved_context_ebp, %ebp > movl saved_context_ebx, %ebx > movl saved_context_esi, %esi > movl saved_context_edi, %edi > pushl saved_context_eflags > popfl > ret > > this is called before restore_processor_state(). > > those 'saved_context_*' references are already using the kernel DS! So if it's > corrupted, we'll crash there already. So your patch seems totally nonsensical. > > 3) > > So the patch below (totally untested) does the DS restoration early on, as the > first thing after we emerge from the sleep. This should be equivalent to your > patch, but is more robust and much simpler: there's no need to save DS, > because we > know that it has to be __KERNEL_DS. > > Could you please test whether this solves the problem as well? Also, could you > please describe how the failure triggers in your system: how many times do you > have to suspend/resume to trigger the segfaults, and is there anything that > makes > the failures less or more likely? > > Thanks, > > Ingo > > =================> > arch/x86/kernel/acpi/wakeup_32.S | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kernel/acpi/wakeup_32.S > b/arch/x86/kernel/acpi/wakeup_32.S > index 665c6b7d2ea9..9a3ce66e0dd8 100644 > --- a/arch/x86/kernel/acpi/wakeup_32.S > +++ b/arch/x86/kernel/acpi/wakeup_32.S > @@ -81,6 +81,10 @@ ENTRY(do_suspend_lowlevel) > jmp ret_point > .p2align 4,,7 > ret_point: > + /* In case the BIOS corrupted DS, make the kernel context minimally > functional: */ > + movl $__KERNEL_DS, %eax > + movl %eax, %ds > +
On further thought, I think you want movl $__USER_DS, %eax. The 32-bit kernel is a strange beast. Also, you should probably fix up %es as well. --Andy > call restore_registers > call restore_processor_state > ret -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
