linux-kernel - Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6EB66FDE-DDE1-40FA-A391-AEFF963CA97E@vmware.com>
Date:   Thu, 12 Oct 2023 19:32:43 +0000
From:   Nadav Amit <namit@...are.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     Uros Bizjak <ubizjak@...il.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()

> On Oct 12, 2023, at 8:16 PM, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> !! External Email
> 
> On Thu, 12 Oct 2023 at 08:19, Nadav Amit <namit@...are.com> wrote:
>> 
>> +/*
>> + * Hold a constant alias for current_task, which would allow to avoid caching of
>> + * current task.
>> + *
>> + * We must mark const_current_task with the segment qualifiers, as otherwise gcc
>> + * would do redundant reads of const_current_task.
>> + */
>> +DECLARE_PER_CPU(struct pcpu_hot const __percpu_seg_override, const_pcpu_hot);
> 
> Hmm. The only things I'm not super-happy about with your patch is
> 
> (a) it looks like this depends on the alias analysis knowing that the
> __seg_gs isn't affected by normal memory ops. That implies that this
> will not work well with compiler versions that don't do that?
> 
> (b) This declaration doesn't match the other one. So now there are
> two *different* declarations for const_pcpu_hot, which I really don't
> like.
> 
> That second one would seem to be trivial to just fix (or maybe not,
> and you do it that way for some horrible reason).

If you refer to the difference between DECLARE_PER_CPU_ALIGNED() and
DECLARE_PER_CPU() - that’s just a silly mistake that I made porting my
old patch (I also put “const” in the wrong place of the declaration, sorry).

> 
> The first one sounds bad to me - basically making the *reason* for
> this patch go away - but maybe the compilers that don't support
> address spaces are so rare that we can ignore it.

As far as I understand it has nothing to do with the address spaces, and IIRC
the compiler does not regard gs/fs address spaces as independent from the main
one. That’s the reason a compiler barrier affects regular loads with __seg_gs.

The “trick” that the patch does is to expose a new const_pcpu_hot symbol that has
a “const” qualifier. For compilation units from which the symbol is effectively
constant, we use const_pcpu_hot. The compiler then knows that the value would not
change.

Later, when we actually define the const_pcpu_hot, we tell the compiler using
__attribute__((alias("pcpu_hot”)) that this symbol is actually an alias to pcpu_hot.

Although it is a bit of a trick that I have never seen elsewhere, I don’t see it
violating GCC specifications (“except for top-level qualifiers the alias target
must have the same type as the alias” [1]), and there is nothing that is specific
to the gs address-space. I still have the concern of its interaction with LTO
though, and perhaps using “-fno-lto” when compiling compilation units that
modify current (e.g., arch/x86/kernel/process_64.o) is necessary.

I hope it makes sense.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)