linux-kernel - Re: [PATCH v2 1/8] x86: vdso: Introduce asm/vdso/mman.h

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: <9c1d96d4-61ca-4e27-9d67-8c194cd7770d@arm.com>
Date: Tue, 24 Sep 2024 16:10:32 +0100
From: Vincenzo Frascino <vincenzo.frascino@....com>
To: "Jason A. Donenfeld" <Jason@...c4.com>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
 linux-mm@...ck.org, Andy Lutomirski <luto@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>,
 Christophe Leroy <christophe.leroy@...roup.eu>,
 Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>,
 Naveen N Rao <naveen@...nel.org>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H . Peter Anvin" <hpa@...or.com>, Theodore Ts'o <tytso@....edu>,
 Arnd Bergmann <arnd@...db.de>, Andrew Morton <akpm@...ux-foundation.org>,
 Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH v2 1/8] x86: vdso: Introduce asm/vdso/mman.h

Hi Jason,

Thank you for your review.

On 24/09/2024 00:05, Jason A. Donenfeld wrote:
> Hi,
> 
> I have the feeling I said this in the last two revisions, but maybe I
> just thought it or agreed with somebody else who typed it but never
> typed it myself, so now I'm typing it in no uncertain terms.
> 

This is the second revision, I am not sure to which other two revisions you are
referring to. Anyway if I missed your suggestion, I apologize.

> On Mon, Sep 23, 2024 at 03:19:36PM +0100, Vincenzo Frascino wrote:
>> +#define VDSO_MMAP_PROT	PROT_READ | PROT_WRITE
>> +#define VDSO_MMAP_FLAGS	MAP_DROPPABLE | MAP_ANONYMOUS
> 
> No, absolutely not. This is nonsense. Those flags aren't "the vdso
> flags" or something. The variable name makes no sense. Moving the
> definition outside of getrandom.c like the next patch does also makes no
> sense. Do not do this.
> 
> If you need to, for some reason, rename those symbols, then rename them
> each to VDSO_MAP_ANONYMOUS or whatever, and then use those from within
> getrandom.c so it remains as readable and reasonable as it currently is.
> 
> But under no circumstances should you move where this is expressed and
> rename it something generic like "vdso flags", when it is not generic
> but rather very specific to the function where it is currently used.
> IOW, please take a look and try to understand the code that you're
> touching when proposing changes like this.
> 
> 
> Also, though, I don't quite see what this patch accomplishes. If you're
> fine doing #include <notvdso/whatever.h> into here, importing this
> header into vdso code will transitively include notvdso/whatever.h with
> it. So in that case, either we can keep using MAP_ANONYMOUS and whatnot
> in the original sane symbol names, or this approach isn't correct in the
> first place.
> 
> Maybe what you want instead is a simpler vdso/whatever.h header that
> just includes nonvdso/whatever.h, and then you let getrandom.c and other
> things keep using the same symbols as they were using before.
>

In past we had a problem with compiling vDSOs on certain architectures.
Since then:
 - The generic vDSO library can include only the common denominator of the
headers required to build the library on all the architectures that support it.
 - The headers must come from the vdso/ namespace only (with rare documented
exceptions).
 - The generic vDSO library does not mandate how an architecture organizes its
headers or provides the required symbols.

Based on this it is not fine to include directly "notvdso/whatever.h" into
"vdso/whatever.h" because a future change to first might work on one
architecture but might break another one.

To the naming problem: I agree, maybe the naming is not self explanatory and
might need some comments to clarify its purpose.

The reasons why I introduced an extra indirection are the following:
 - Allow the architecture to decide if it wants to include directly mman.h or
not. As it was discussed already [1] a future update might cause problems (Note:
for x86 I honored your original strategy).
 - A future architecture might need different prot/flags.

[1]
https://lore.kernel.org/lkml/cb66b582-ba63-4a5a-9df8-b07288f1f66d@app.fastmail.com/

I am open to suggestions on what's your preference to address the problem. Let
me know your thoughts.
> Jason

-- 
Regards,
Vincenzo