| Message ID | 20240903151437.1002990-2-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vdso: Use only headers from the vdso/ namespace | expand |
Christophe explained the issue with this in https://lore.kernel.org/all/85efc7c5-40c8-4c89-b65f-dd13536fb8c7@cs-soprasteria.com/
On 03/09/2024 16:16, Jason A. Donenfeld wrote: > Christophe explained the issue with this in > https://lore.kernel.org/all/85efc7c5-40c8-4c89-b65f-dd13536fb8c7@cs-soprasteria.com/ And I think I replied to Christophe here: https://lore.kernel.org/all/632b8da1-c165-4d17-804f-4edf1438d55a@arm.com/ Can you please explain what you are referring to explicitly?
Le 03/09/2024 à 17:14, 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 benefit of duplicating those two defines in every arch. I understand your point that some arch might in the future want to use different flags, but unless we already have one such architecture in mind we shouldn't make things more complicated than needed. In case such an architecture is already identified it should be said in the commit message > + > +#endif /* !__ASSEMBLY__ */ > + > +#endif /* __ASM_VDSO_MMAN_H */
Hi Christophe, Thank you for your review. On 04/09/2024 17:56, Christophe Leroy wrote: > > > Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit : ... >> +/* 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 benefit of duplicating those two defines in every arch. > > I understand your point that some arch might in the future want to use different > flags, but unless we already have one such architecture in mind we shouldn't > make things more complicated than needed. > > In case such an architecture is already identified it should be said in the > commit message > I do not have such an architecture in mind, hence I did not add it to the commit message. Apart being future proof the real problem here is to handle the mman.h header. As Arnd was saying [1] (and I agree), including it on some architectures might be problematic if they change it in a way that is incompatible with compat vdso. In this way we make sure that the each architecture that decides to include it specifically validates it for this purpose. I am not a fan of complicating code either but this seems the lesser evil. I am open to any solution you can come up that is better then this one. The other issue I see is that being these defines in a uapi header that is included directly by userspace splitting it requires some validation to make sure we do not break the status quo. [1] https://lore.kernel.org/lkml/cb66b582-ba63-4a5a-9df8-b07288f1f66d@app.fastmail.com/ >> + >> +#endif /* !__ASSEMBLY__ */ >> + >> +#endif /* __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 + +#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