| Message ID | 20240923141943.133551-2-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vdso: Use only headers from the vdso/ namespace | expand |
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
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
Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit : > The VDSO implementation includes headers from outside of the > vdso/ namespace. > > Introduce asm/vdso/mman.h to make sure that the generic library > uses only the allowed namespace. > > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason A. Donenfeld <Jason@zx2c4.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > arch/x86/include/asm/vdso/mman.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 arch/x86/include/asm/vdso/mman.h > > diff --git a/arch/x86/include/asm/vdso/mman.h b/arch/x86/include/asm/vdso/mman.h > new file mode 100644 > index 000000000000..4c936c9d11ab > --- /dev/null > +++ b/arch/x86/include/asm/vdso/mman.h > @@ -0,0 +1,15 @@ > + > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_VDSO_MMAN_H > +#define __ASM_VDSO_MMAN_H > + > +#ifndef __ASSEMBLY__ > + > +#include <uapi/linux/mman.h> > + > +#define VDSO_MMAP_PROT PROT_READ | PROT_WRITE > +#define VDSO_MMAP_FLAGS MAP_DROPPABLE | MAP_ANONYMOUS I still can't see the point with that change. Today 4 architectures implement getrandom and none of them require that indirection. Please leave prot and flags as they are in the code. Then this file is totally pointless, VDSO code can include uapi/linux/mman.h directly. VDSO is userland code, it should be safe to include any UAPI file there. Christophe > + > +#endif /* !__ASSEMBLY__ */ > + > +#endif /* __ASM_VDSO_MMAN_H */
On Wed, Sep 25, 2024, at 06:51, Christophe Leroy wrote: > Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit : >> @@ -0,0 +1,15 @@ >> + >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ASM_VDSO_MMAN_H >> +#define __ASM_VDSO_MMAN_H >> + >> +#ifndef __ASSEMBLY__ >> + >> +#include <uapi/linux/mman.h> >> + >> +#define VDSO_MMAP_PROT PROT_READ | PROT_WRITE >> +#define VDSO_MMAP_FLAGS MAP_DROPPABLE | MAP_ANONYMOUS > > I still can't see the point with that change. > > Today 4 architectures implement getrandom and none of them require that > indirection. Please leave prot and flags as they are in the code. > > Then this file is totally pointless, VDSO code can include > uapi/linux/mman.h directly. > > VDSO is userland code, it should be safe to include any UAPI file there. I think we are hitting an unfortunate corner case in the build system here, based on the way we handle the uapi/ file namespace in the kernel: include/uapi/linux/mman.h includes three headers: asm/mman.h, asm-generic/hugetlb_encode.h and linux/types.h. Two of these exist in both include/uapi/ and include/, so while building kernel code we end up picking up the non-uapi version which on some architectures includes many other headers. I agree that moving the contents out of uapi/ into vdso/ namespace is not a solution here because that removes the contents from the installed user headers, but we still need to do something to solve the issue. The easiest workaround I see for this particular file is to move the contents of arch/{arm,arm64,parisc,powerpc,sparc,x86}/\ include/asm/mman.h into a different file to ensure that the only existing file is the uapi/ one. Unfortunately this does not help to avoid it regressing again in the future. To go a little step further I would also move uapi/asm-generic/hugetlb_encode.h to uapi/linux/hugetlb_encode.h or merge it into uapi/linux/mman.h. This file has no business in asm-generic/* since there is only one copy. After looking at this file for way too long, I somehow ended up with a (completely unrelated) cleanup series that I now posted at https://lore.kernel.org/lkml/20240925210615.2572360-1-arnd@kernel.org/T/#t Arnd
Le 25/09/2024 à 23:23, Arnd Bergmann a écrit : > On Wed, Sep 25, 2024, at 06:51, Christophe Leroy wrote: >> Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit : >>> @@ -0,0 +1,15 @@ >>> + >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef __ASM_VDSO_MMAN_H >>> +#define __ASM_VDSO_MMAN_H >>> + >>> +#ifndef __ASSEMBLY__ >>> + >>> +#include <uapi/linux/mman.h> >>> + >>> +#define VDSO_MMAP_PROT PROT_READ | PROT_WRITE >>> +#define VDSO_MMAP_FLAGS MAP_DROPPABLE | MAP_ANONYMOUS >> >> I still can't see the point with that change. >> >> Today 4 architectures implement getrandom and none of them require that >> indirection. Please leave prot and flags as they are in the code. >> >> Then this file is totally pointless, VDSO code can include >> uapi/linux/mman.h directly. >> >> VDSO is userland code, it should be safe to include any UAPI file there. > > I think we are hitting an unfortunate corner case in the build > system here, based on the way we handle the uapi/ file namespace > in the kernel: > > include/uapi/linux/mman.h includes three headers: asm/mman.h, > asm-generic/hugetlb_encode.h and linux/types.h. Two of these > exist in both include/uapi/ and include/, so while building > kernel code we end up picking up the non-uapi version which > on some architectures includes many other headers. Right, and that's the reason why arm64 and powerpc guarded the content of asm/mman.h which an #ifndef BUILD_VDSO. Note that arm64 also has a similar workaround in asm/rwonce.h, brought by commit e35123d83ee3 ("arm64: lto: Strengthen READ_ONCE() to acquire when CONFIG_LTO=y") without explaination on why VDSO builds are excluded. > > I agree that moving the contents out of uapi/ into vdso/ namespace > is not a solution here because that removes the contents from > the installed user headers, but we still need to do something > to solve the issue. Should header inclusion be reworked so that only UAPI and VDSO pathes are looked for when including headers in VDSO builds ? > > The easiest workaround I see for this particular file is to > move the contents of arch/{arm,arm64,parisc,powerpc,sparc,x86}/\ > include/asm/mman.h into a different file to ensure that the > only existing file is the uapi/ one. Unfortunately this does > not help to avoid it regressing again in the future. Could we add a check in checkpatch.pl to ensure UAPI headers do not include headers that exist both in UAPI and non-UAPI space in the future ? Christophe
On Thu, Sep 26, 2024, at 05:51, Christophe Leroy wrote: > Le 25/09/2024 à 23:23, Arnd Bergmann a écrit : >> I agree that moving the contents out of uapi/ into vdso/ namespace >> is not a solution here because that removes the contents from >> the installed user headers, but we still need to do something >> to solve the issue. > > Should header inclusion be reworked so that only UAPI and VDSO pathes > are looked for when including headers in VDSO builds ? I think that could work. Not sure how hard it will be to get to that point, but I like the idea. >> The easiest workaround I see for this particular file is to >> move the contents of arch/{arm,arm64,parisc,powerpc,sparc,x86}/\ >> include/asm/mman.h into a different file to ensure that the >> only existing file is the uapi/ one. Unfortunately this does >> not help to avoid it regressing again in the future. > > Could we add a check in checkpatch.pl to ensure UAPI headers do not > include headers that exist both in UAPI and non-UAPI space in the future ? That is a much weaker check, I suspect it won't actually catch most regressions as it's too easy to ignore checkpatch warnings. Arnd
On 25/09/2024 22:23, Arnd Bergmann wrote: > On Wed, Sep 25, 2024, at 06:51, Christophe Leroy wrote: >> Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit : >>> @@ -0,0 +1,15 @@ ... >> >> I still can't see the point with that change. >> >> Today 4 architectures implement getrandom and none of them require that >> indirection. Please leave prot and flags as they are in the code. >> >> Then this file is totally pointless, VDSO code can include >> uapi/linux/mman.h directly. >> >> VDSO is userland code, it should be safe to include any UAPI file there. > > I think we are hitting an unfortunate corner case in the build > system here, based on the way we handle the uapi/ file namespace > in the kernel: > > include/uapi/linux/mman.h includes three headers: asm/mman.h, > asm-generic/hugetlb_encode.h and linux/types.h. Two of these > exist in both include/uapi/ and include/, so while building > kernel code we end up picking up the non-uapi version which > on some architectures includes many other headers. > > I agree that moving the contents out of uapi/ into vdso/ namespace > is not a solution here because that removes the contents from > the installed user headers, but we still need to do something > to solve the issue. > > The easiest workaround I see for this particular file is to > move the contents of arch/{arm,arm64,parisc,powerpc,sparc,x86}/\ > include/asm/mman.h into a different file to ensure that the > only existing file is the uapi/ one. Unfortunately this does > not help to avoid it regressing again in the future. > > To go a little step further I would also move > uapi/asm-generic/hugetlb_encode.h to uapi/linux/hugetlb_encode.h > or merge it into uapi/linux/mman.h. This file has no business > in asm-generic/* since there is only one copy. > > After looking at this file for way too long, I somehow > ended up with a (completely unrelated) cleanup series that > I now posted at > https://lore.kernel.org/lkml/20240925210615.2572360-1-arnd@kernel.org/T/#t > I had a look at your proposal and it seems definitely better then mine. Thanks Arnd. I am happy to drop my changes and re-post only a small series with PAGE_SIZE/MASK required rework. > Arnd
diff --git a/arch/x86/include/asm/vdso/mman.h b/arch/x86/include/asm/vdso/mman.h new file mode 100644 index 000000000000..4c936c9d11ab --- /dev/null +++ b/arch/x86/include/asm/vdso/mman.h @@ -0,0 +1,15 @@ + +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ASM_VDSO_MMAN_H +#define __ASM_VDSO_MMAN_H + +#ifndef __ASSEMBLY__ + +#include <uapi/linux/mman.h> + +#define VDSO_MMAP_PROT PROT_READ | PROT_WRITE +#define VDSO_MMAP_FLAGS MAP_DROPPABLE | MAP_ANONYMOUS + +#endif /* !__ASSEMBLY__ */ + +#endif /* __ASM_VDSO_MMAN_H */
The VDSO implementation includes headers from outside of the vdso/ namespace. Introduce asm/vdso/mman.h to make sure that the generic library uses only the allowed namespace. Cc: Andy Lutomirski <luto@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason A. Donenfeld <Jason@zx2c4.com> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- arch/x86/include/asm/vdso/mman.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 arch/x86/include/asm/vdso/mman.h