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

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

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