linux-kernel - Re: [patch 1/3] x86/cpu: Remove segment load from switch_to_new

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Jul 2022 11:43:19 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Andrew Cooper <Andrew.Cooper3@...rix.com>,
        Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
        Johannes Wikner <kwikner@...z.ch>,
        Alyssa Milburn <alyssa.milburn@...ux.intel.com>,
        Jann Horn <jannh@...gle.com>, "H.J. Lu" <hjl.tools@...il.com>,
        Joao Moreira <joao.moreira@...el.com>,
        Joseph Nuzman <joseph.nuzman@...el.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Juergen Gross <jgross@...e.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [patch 1/3] x86/cpu: Remove segment load from switch_to_new_gdt()

So I appreciate the added big comments in this code, but looking at this patch:

On Mon, Jul 18, 2022 at 10:52 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> +/**
> + * switch_to_new_gdt - Switch form early GDT to the direct one
> + * @cpu:       The CPU number for which this is invoked
> + *
> + * Invoked during early boot to switch from early GDT and early per CPU
> + * (%fs on 32bit, GS_BASE on 64bit) to the direct GDT and the runtime per
> + * CPU area.
>   */
>  void switch_to_new_gdt(int cpu)
>  {
> -       /* Load the original GDT */
>         load_direct_gdt(cpu);
> -       /* Reload the per-cpu base */
> -       load_percpu_segment(cpu);
> +
> +       /*
> +        * No need to load the %gs (%fs for 32bit) segment. It is already
> +        * correct, %gs is 0 on 64bit and %fs is __KERNEL_PERCPU on 32 bit.
> +        *
> +        * Writing %gs on 64bit would zero GSBASE which would make any per
> +        * CPU operation up to the point of the wrmsrl() fault.
> +        *
> +        * 64bit requires to point GSBASE to the new offset. Until the
> +        * wrmsrl() happens the early mapping is still valid. That means
> +        * the GSBASE update will lose any prior per CPU data which was
> +        * not copied over in setup_per_cpu_areas().
> +        *
> +        * For secondary CPUs this is not a problem because they start
> +        * already with the direct GDT and the real GSBASE. This invocation
> +        * is pointless and will be removed in a subsequent step.
> +        */
> +       if (IS_ENABLED(CONFIG_X86_64))
> +               wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
>  }

... while those comments are nice and all, I do think this retains the
basic insanity of having "switch_to_new_gdt()" do magical things on
x86-64 that don't really match the name.

So honestly, I'd be happier of that whole

       if (IS_ENABLED(CONFIG_X86_64))
               wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));

was migrated to the callers instead. There aren't *that* many callers.

I expect that it is then quite possible that several of the call-sites
would go "GS_BASE is already correct here, I can remove this".

But even if every single caller keeps that wrmsrl() around, at least
it wouldn't be hidden behind a function call that has a name that
implies something completely different is happening.

And no, I don't care *that* deeply, so this is just a suggestion.

But wouldn't it be nice if this function was actually named by what it
does, rather than by what it used to do back in the i386 days when the
GDT affected the segment bases?

                  Linus