Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752149AbdLAJGV (ORCPT ); Fri, 1 Dec 2017 04:06:21 -0500 Received: from mga14.intel.com ([192.55.52.115]:35043 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751340AbdLAJGO (ORCPT ); Fri, 1 Dec 2017 04:06:14 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,344,1508828400"; d="scan'208";a="8290092" Subject: Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context() To: Andy Lutomirski , x86@kernel.org Cc: linux-kernel@vger.kernel.org, Borislav Petkov , Peter Zijlstra , Linus Torvalds References: <6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org> From: Jarkko Nikula Message-ID: Date: Fri, 1 Dec 2017 11:06:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2549 Lines: 77 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 > Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()") > Signed-off-by: Andy Lutomirski > --- > > 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 Tested-by: Jarkko Nikula