Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752375AbdLKSmF (ORCPT ); Mon, 11 Dec 2017 13:42:05 -0500 Received: from mail-io0-f171.google.com ([209.85.223.171]:36574 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751307AbdLKSmB (ORCPT ); Mon, 11 Dec 2017 13:42:01 -0500 X-Google-Smtp-Source: ACJfBovLVSE4dSwBoRRCxZOHwMHtBlEgJHsmStQ6s2/TSJ7KaZNl3X5lDsOqpKloqYQa4xVnryRSsgsPbtUiDZaYK6U= MIME-Version: 1.0 In-Reply-To: References: <2809506.pL8kVbvXcY@aspire.rjw.lan> <20171210213804.GA4660@amd> <76028A95-1CE1-49AA-9929-9C15FFC520EB@amacapital.net> <151331264.Sjvx2oHGGq@aspire.rjw.lan> From: Andy Lutomirski Date: Mon, 11 Dec 2017 10:41:40 -0800 Message-ID: Subject: Re: [PATCH] Fix resume on x86-32 machines To: Linus Torvalds Cc: "Rafael J. Wysocki" , Pavel Machek , Andrew Lutomirski , Zhang Rui , Thomas Gleixner , Jarkko Nikula , Linux Kernel Mailing List , "the arch/x86 maintainers" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2255 Lines: 60 On Mon, Dec 11, 2017 at 10:31 AM, Linus Torvalds wrote: > On Mon, Dec 11, 2017 at 6:22 AM, Rafael J. Wysocki wrote: >> On Sunday, December 10, 2017 10:58:23 PM CET Andy Lutomirski wrote: >>> >>> I'm guessing that the real issue is that 32-bit needs %fs restored early for TLS. >> >> I *think* you are right. >> >> Anyway, that should be easy enough to verify. >> >> Pavel, can you please check if the below change works too? > > So Jarkko confirmed this works for him, but the more I look at this > crap, the less I like it. > > Why do we save fs/ds/es/ss at all on x86-32? Don't they all have fixed > values in the kernel, with %fs being __KERNEL_PERCPU, and the others > being __USER_DS? > > Nothing else can possibly be valid, as far as I can tell. > > I think we actually leave the user-space percpu segment in %gs (or the > stack canary base), so that one we should actually save/restore, but > I'm getting the feeling that we should just reset the other segment > registers to known values on 32-bit. > > Also, why does the 32-bit code do > > loadsegment(es, ctxt->es); > > but the 64-bit code does > > asm volatile ("movw %0, %%es" :: "r" (ctxt->es)); > > And look at that confusion between MSR_GS_BASE and MSR_KERNEL_GS_BASE > all within the 64-bit case. > > In particular, note how we reload the %gs segment in between the two - > wouldn't that mess with the currently active gs base if %gs can be > non-zero? > > Christ, what a mess. > > So I think that whole sequence is garbage. It has been written as some > kind of "save and restore registers", but that's not what it really > then does - or what it should do. > > It should make sure to restore a sane kernel state, not some random > register state. > > And the 32-bit and 64-bit code really should strive to be at least > _sanely_ different, not this randomly and insanely different mess. > > But yes, Rafael's patch looks like the minimal one-liner. But I think > we should do the %gs load early too for the 32-bit stack canary case, > kind of like we need to do %fs for percpu base. I'll try to get to this in a day or so -- is that okay? Or should we do some trivial fix/revert and fix it for real next time around?