Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

Jarkko Nikula
2017-12-01T09:06:21Z

On 11/30/2017 05:57 PM, Andy Lutomirski wrote:
> __restore_processor_context() had a couple of ordering bugs. It
> restored GSBASE after calling load_gs_index(), and the latter can
> call into tracing code. It also tried to restore segment registers
> before restoring the LDT, which is straight-up wrong.
>
> Reorder the code so that we restore GSBASE, then the descriptor
> tables, then the segments.
>
> This fixes two bugs. First, it fixes a regression that broke resume
> under certain configurations due to irqflag tracing in
> native_load_gs_index(). Second, it fixes resume when the userspace
> process that initiated suspect had funny segments. The latter can be
> reproduced by compiling this:
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * ldt_echo.c - Echo argv[1] while using an LDT segment
> */
>
> int main(int argc, char **argv)
> {
> int ret;
> size_t len;
> char *buf;
>
> const struct user_desc desc = {
> .entry_number = 0,
> .base_addr = 0,
> .limit = 0xfffff,
> .seg_32bit = 1,
> .contents = 0, /* Data, grow-up */
> .read_exec_only = 0,
> .limit_in_pages = 1,
> .seg_not_present = 1,
> .useable = 0
> };
>
> if (argc != 2)
> errx(1, "Usage: %s STRING", argv[0]);
>
> len = asprintf(&buf, "%s\n", argv[1]);
> if (len < 0)
> errx(1, "Out of memory");
>
> ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
> if (ret < -1)
> errno = -ret;
> if (ret)
> err(1, "modify_ldt");
>
> asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
> write(1, buf, len);
> return 0;
> }
>
> and running ldt_echo >/sys/power/mem
>
> Without the fix, the latter causes a triple fault on resume.
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> Jarkko, can you test this version?
>
> arch/x86/power/cpu.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
It does fix the suspend/resume issue I saw. Patch applied on top of
current head df8ba95c572a and a loop below completed fine on a few
machines where I tested it. All of them had issue with the ca37e57bbe0c.

for ((i=0;i<10;i++)); do rtcwake -s 5 -m mem; echo $i; done