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@xxxxxxxxxxxxxxxx/ I am open to suggestions on what's your preference to address the problem. Let me know your thoughts. > Jason -- Regards, Vincenzo
