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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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.

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.

Jason