[PATCH v2 0/8] vdso: Use only headers from the vdso/ namespace

[PATCH v2 0/8] vdso: Use only headers from the vdso/ namespace

[Vincenzo Frascino]

The recent implementation of getrandom in the generic vdso library,
includes headers from outside of the vdso/ namespace.

The purpose of this patch series is to refactor the code to make sure
that the library uses only the allowed namespace.

The series has been rebased on [1] to simplify the testing. 

[1] git://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git master

Changes:
--------
v2:
  - Added common PAGE_SIZE and PAGE_MASK definitions.
  - Added opencoded macros where not defined.
  - Dropped VDSO_PAGE_* redefinitions.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

Vincenzo Frascino (8):
  x86: vdso: Introduce asm/vdso/mman.h
  arm64: vdso: Introduce asm/vdso/mman.h
  vdso: Introduce vdso/mman.h
  vdso: Introduce vdso/page.h
  x86: vdso: Modify asm/vdso/getrandom.h to include datapage
  vdso: Modify vdso/getrandom.h to include the asm header
  vdso: Introduce uapi/vdso/random.h
  vdso: Modify getrandom to include the correct namespace.

 arch/alpha/include/asm/page.h         |  6 +----
 arch/arc/include/uapi/asm/page.h      |  7 +++--
 arch/arm/include/asm/page.h           |  5 +---
 arch/arm64/include/asm/page-def.h     |  5 +---
 arch/arm64/include/asm/vdso/mman.h    | 15 +++++++++++
 arch/csky/include/asm/page.h          |  8 ++----
 arch/hexagon/include/asm/page.h       |  4 +--
 arch/loongarch/include/asm/page.h     |  7 +----
 arch/m68k/include/asm/page.h          |  6 ++---
 arch/microblaze/include/asm/page.h    |  5 +---
 arch/mips/include/asm/page.h          |  7 +----
 arch/nios2/include/asm/page.h         |  7 +----
 arch/openrisc/include/asm/page.h      | 11 +-------
 arch/parisc/include/asm/page.h        |  4 +--
 arch/powerpc/include/asm/page.h       | 10 +------
 arch/riscv/include/asm/page.h         |  4 +--
 arch/s390/include/asm/page.h          |  4 +--
 arch/sh/include/asm/page.h            |  6 ++---
 arch/sparc/include/asm/page_32.h      |  4 +--
 arch/sparc/include/asm/page_64.h      |  4 +--
 arch/um/include/asm/page.h            |  5 +---
 arch/x86/include/asm/page_types.h     |  5 +---
 arch/x86/include/asm/vdso/getrandom.h |  2 ++
 arch/x86/include/asm/vdso/mman.h      | 15 +++++++++++
 arch/xtensa/include/asm/page.h        |  8 +-----
 include/uapi/linux/random.h           | 26 +-----------------
 include/uapi/vdso/random.h            | 38 +++++++++++++++++++++++++++
 include/vdso/datapage.h               |  2 ++
 include/vdso/getrandom.h              |  1 +
 include/vdso/mman.h                   |  7 +++++
 include/vdso/page.h                   | 18 +++++++++++++
 lib/vdso/getrandom.c                  | 22 ++++++++--------
 32 files changed, 137 insertions(+), 141 deletions(-)
 create mode 100644 arch/arm64/include/asm/vdso/mman.h
 create mode 100644 arch/x86/include/asm/vdso/mman.h
 create mode 100644 include/uapi/vdso/random.h
 create mode 100644 include/vdso/mman.h
 create mode 100644 include/vdso/page.h

-- 
2.34.1

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

[Vincenzo Frascino]

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
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_MMAN_H */
-- 
2.34.1

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

[Jason A. Donenfeld]

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

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

[Vincenzo Frascino]

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

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

[Christophe Leroy]

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 */

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

[Arnd Bergmann]

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

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

[Christophe Leroy]

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

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

[Arnd Bergmann]

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

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

[Vincenzo Frascino]

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

-- 
Regards,
Vincenzo

[PATCH v2 2/8] arm64: vdso: Introduce asm/vdso/mman.h

[Vincenzo Frascino]

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/arm64/include/asm/vdso/mman.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 arch/arm64/include/asm/vdso/mman.h

diff --git a/arch/arm64/include/asm/vdso/mman.h b/arch/arm64/include/asm/vdso/mman.h
new file mode 100644
index 000000000000..4c936c9d11ab
--- /dev/null
+++ b/arch/arm64/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 */
-- 
2.34.1

Re: [PATCH v2 2/8] arm64: vdso: Introduce asm/vdso/mman.h

[Christophe Leroy]

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/arm64/include/asm/vdso/mman.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>   create mode 100644 arch/arm64/include/asm/vdso/mman.h
> 
> diff --git a/arch/arm64/include/asm/vdso/mman.h b/arch/arm64/include/asm/vdso/mman.h
> new file mode 100644
> index 000000000000..4c936c9d11ab
> --- /dev/null
> +++ b/arch/arm64/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
> +

Same comment here as for x86, the flags are the same on all archictures, 
no need for such an indirection which makes the code less readable.

Christophe

[PATCH v2 3/8] vdso: Introduce vdso/mman.h

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Introduce 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>
---
 include/vdso/mman.h | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 include/vdso/mman.h

diff --git a/include/vdso/mman.h b/include/vdso/mman.h
new file mode 100644
index 000000000000..95e3da714c64
--- /dev/null
+++ b/include/vdso/mman.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_MMAN_H
+#define __VDSO_MMAN_H
+
+#include <asm/vdso/mman.h>
+
+#endif	/* __VDSO_MMAN_H */
-- 
2.34.1

Re: [PATCH v2 3/8] vdso: Introduce vdso/mman.h

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Introduce vdso/mman.h to make sure that the generic library
> uses only the allowed namespace.

I can't see the added value of this header.

VDSO code can include <asm/vdso/mman.h> directly.

Christophe

> 
> 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>
> ---
>   include/vdso/mman.h | 7 +++++++
>   1 file changed, 7 insertions(+)
>   create mode 100644 include/vdso/mman.h
> 
> diff --git a/include/vdso/mman.h b/include/vdso/mman.h
> new file mode 100644
> index 000000000000..95e3da714c64
> --- /dev/null
> +++ b/include/vdso/mman.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __VDSO_MMAN_H
> +#define __VDSO_MMAN_H
> +
> +#include <asm/vdso/mman.h>
> +
> +#endif	/* __VDSO_MMAN_H */

[PATCH v2 4/8] vdso: Introduce vdso/page.h

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Introduce vdso/page.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/alpha/include/asm/page.h      |  6 +-----
 arch/arc/include/uapi/asm/page.h   |  7 +++----
 arch/arm/include/asm/page.h        |  5 +----
 arch/arm64/include/asm/page-def.h  |  5 +----
 arch/csky/include/asm/page.h       |  8 ++------
 arch/hexagon/include/asm/page.h    |  4 +---
 arch/loongarch/include/asm/page.h  |  7 +------
 arch/m68k/include/asm/page.h       |  6 ++----
 arch/microblaze/include/asm/page.h |  5 +----
 arch/mips/include/asm/page.h       |  7 +------
 arch/nios2/include/asm/page.h      |  7 +------
 arch/openrisc/include/asm/page.h   | 11 +----------
 arch/parisc/include/asm/page.h     |  4 +---
 arch/powerpc/include/asm/page.h    | 10 +---------
 arch/riscv/include/asm/page.h      |  4 +---
 arch/s390/include/asm/page.h       |  4 +---
 arch/sh/include/asm/page.h         |  6 ++----
 arch/sparc/include/asm/page_32.h   |  4 +---
 arch/sparc/include/asm/page_64.h   |  4 +---
 arch/um/include/asm/page.h         |  5 +----
 arch/x86/include/asm/page_types.h  |  5 +----
 arch/xtensa/include/asm/page.h     |  8 +-------
 include/vdso/page.h                | 18 ++++++++++++++++++
 23 files changed, 45 insertions(+), 105 deletions(-)
 create mode 100644 include/vdso/page.h

diff --git a/arch/alpha/include/asm/page.h b/arch/alpha/include/asm/page.h
index 70419e6be1a3..261af54fd601 100644
--- a/arch/alpha/include/asm/page.h
+++ b/arch/alpha/include/asm/page.h
@@ -4,11 +4,7 @@
 
 #include <linux/const.h>
 #include <asm/pal.h>
-
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arc/include/uapi/asm/page.h b/arch/arc/include/uapi/asm/page.h
index 7fd9e741b527..4606a326af5c 100644
--- a/arch/arc/include/uapi/asm/page.h
+++ b/arch/arc/include/uapi/asm/page.h
@@ -14,7 +14,7 @@
 
 /* PAGE_SHIFT determines the page size */
 #ifdef __KERNEL__
-#define PAGE_SHIFT CONFIG_PAGE_SHIFT
+#include <vdso/page.h>
 #else
 /*
  * Default 8k
@@ -24,11 +24,10 @@
  * not available
  */
 #define PAGE_SHIFT 13
+#define PAGE_SIZE	_BITUL(PAGE_SHIFT)	/* Default 8K */
+#define PAGE_MASK	(~(PAGE_SIZE-1))
 #endif
 
-#define PAGE_SIZE	_BITUL(PAGE_SHIFT)	/* Default 8K */
 #define PAGE_OFFSET	_AC(0x80000000, UL)	/* Kernel starts at 2G onwrds */
 
-#define PAGE_MASK	(~(PAGE_SIZE-1))
-
 #endif /* _UAPI__ASM_ARC_PAGE_H */
diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
index 62af9f7f9e96..ef11b721230e 100644
--- a/arch/arm/include/asm/page.h
+++ b/arch/arm/include/asm/page.h
@@ -7,10 +7,7 @@
 #ifndef _ASMARM_PAGE_H
 #define _ASMARM_PAGE_H
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT		CONFIG_PAGE_SHIFT
-#define PAGE_SIZE		(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK		(~((1 << PAGE_SHIFT) - 1))
+#include <vdso/page.h>
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/include/asm/page-def.h b/arch/arm64/include/asm/page-def.h
index 792e9fe881dc..d402e08442ee 100644
--- a/arch/arm64/include/asm/page-def.h
+++ b/arch/arm64/include/asm/page-def.h
@@ -10,9 +10,6 @@
 
 #include <linux/const.h>
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT		CONFIG_PAGE_SHIFT
-#define PAGE_SIZE		(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK		(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #endif /* __ASM_PAGE_DEF_H */
diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
index 0ca6c408c07f..f8beae295afb 100644
--- a/arch/csky/include/asm/page.h
+++ b/arch/csky/include/asm/page.h
@@ -7,12 +7,8 @@
 #include <asm/cache.h>
 #include <linux/const.h>
 
-/*
- * PAGE_SHIFT determines the page size: 4KB
- */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE - 1))
+#include <vdso/page.h>
+
 #define THREAD_SIZE	(PAGE_SIZE * 2)
 #define THREAD_MASK	(~(THREAD_SIZE - 1))
 #define THREAD_SHIFT	(PAGE_SHIFT + 1)
diff --git a/arch/hexagon/include/asm/page.h b/arch/hexagon/include/asm/page.h
index 8a6af57274c2..b01f8df69dd4 100644
--- a/arch/hexagon/include/asm/page.h
+++ b/arch/hexagon/include/asm/page.h
@@ -45,9 +45,7 @@
 #define HVM_HUGEPAGE_SIZE 0x5
 #endif
 
-#define PAGE_SHIFT CONFIG_PAGE_SHIFT
-#define PAGE_SIZE  (1UL << PAGE_SHIFT)
-#define PAGE_MASK  (~((1 << PAGE_SHIFT) - 1))
+#include <vdso/page.h>
 
 #ifdef __KERNEL__
 #ifndef __ASSEMBLY__
diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
index e85df33f11c7..83f3533e31a4 100644
--- a/arch/loongarch/include/asm/page.h
+++ b/arch/loongarch/include/asm/page.h
@@ -8,12 +8,7 @@
 #include <linux/const.h>
 #include <asm/addrspace.h>
 
-/*
- * PAGE_SHIFT determines the page size
- */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE - 1))
+#include <vdso/page.h>
 
 #define HPAGE_SHIFT	(PAGE_SHIFT + PAGE_SHIFT - 3)
 #define HPAGE_SIZE	(_AC(1, UL) << HPAGE_SHIFT)
diff --git a/arch/m68k/include/asm/page.h b/arch/m68k/include/asm/page.h
index 8cfb84b49975..b173ba27d36f 100644
--- a/arch/m68k/include/asm/page.h
+++ b/arch/m68k/include/asm/page.h
@@ -6,10 +6,8 @@
 #include <asm/setup.h>
 #include <asm/page_offset.h>
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
+
 #define PAGE_OFFSET	(PAGE_OFFSET_RAW)
 
 #ifndef __ASSEMBLY__
diff --git a/arch/microblaze/include/asm/page.h b/arch/microblaze/include/asm/page.h
index 8810f4f1c3b0..d1ec3806edab 100644
--- a/arch/microblaze/include/asm/page.h
+++ b/arch/microblaze/include/asm/page.h
@@ -19,10 +19,7 @@
 
 #ifdef __KERNEL__
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(ASM_CONST(1) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #define LOAD_OFFSET	ASM_CONST((CONFIG_KERNEL_START-CONFIG_KERNEL_BASE_ADDR))
 
diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
index 4609cb0326cf..bc3e3484c1bf 100644
--- a/arch/mips/include/asm/page.h
+++ b/arch/mips/include/asm/page.h
@@ -14,12 +14,7 @@
 #include <linux/kernel.h>
 #include <asm/mipsregs.h>
 
-/*
- * PAGE_SHIFT determines the page size
- */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
+#include <vdso/page.h>
 
 /*
  * This is used for calculating the real page sizes
diff --git a/arch/nios2/include/asm/page.h b/arch/nios2/include/asm/page.h
index 0722f88e63cc..2897ec1b74f6 100644
--- a/arch/nios2/include/asm/page.h
+++ b/arch/nios2/include/asm/page.h
@@ -18,12 +18,7 @@
 #include <linux/pfn.h>
 #include <linux/const.h>
 
-/*
- * PAGE_SHIFT determines the page size
- */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE - 1))
+#include <vdso/page.h>
 
 /*
  * PAGE_OFFSET -- the first address of the first page of memory.
diff --git a/arch/openrisc/include/asm/page.h b/arch/openrisc/include/asm/page.h
index 1d5913f67c31..124a2db4b160 100644
--- a/arch/openrisc/include/asm/page.h
+++ b/arch/openrisc/include/asm/page.h
@@ -15,16 +15,7 @@
 #ifndef __ASM_OPENRISC_PAGE_H
 #define __ASM_OPENRISC_PAGE_H
 
-
-/* PAGE_SHIFT determines the page size */
-
-#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
-#ifdef __ASSEMBLY__
-#define PAGE_SIZE       (1 << PAGE_SHIFT)
-#else
-#define PAGE_SIZE       (1UL << PAGE_SHIFT)
-#endif
-#define PAGE_MASK       (~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #define PAGE_OFFSET	0xc0000000
 #define KERNELBASE	PAGE_OFFSET
diff --git a/arch/parisc/include/asm/page.h b/arch/parisc/include/asm/page.h
index 4bea2e95798f..6c4836fb5407 100644
--- a/arch/parisc/include/asm/page.h
+++ b/arch/parisc/include/asm/page.h
@@ -4,9 +4,7 @@
 
 #include <linux/const.h>
 
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #define HAVE_ARCH_HUGETLB_UNMAPPED_AREA
 
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 83d0a4fc5f75..af9a2628d1df 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -21,8 +21,7 @@
  * page size. When using 64K pages however, whether we are really supporting
  * 64K pages in HW or not is irrelevant to those definitions.
  */
-#define PAGE_SHIFT		CONFIG_PAGE_SHIFT
-#define PAGE_SIZE		(ASM_CONST(1) << PAGE_SHIFT)
+#include <vdso/page.h>
 
 #ifndef __ASSEMBLY__
 #ifndef CONFIG_HUGETLB_PAGE
@@ -41,13 +40,6 @@ extern unsigned int hpage_shift;
 #define HUGE_MAX_HSTATE		(MMU_PAGE_COUNT-1)
 #endif
 
-/*
- * Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
- * assign PAGE_MASK to a larger type it gets extended the way we want
- * (i.e. with 1s in the high bits)
- */
-#define PAGE_MASK      (~((1 << PAGE_SHIFT) - 1))
-
 /*
  * KERNELBASE is the virtual address of the start of the kernel, it's often
  * the same as PAGE_OFFSET, but _might not be_.
diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index 7ede2111c591..1b685896340a 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -12,9 +12,7 @@
 #include <linux/pfn.h>
 #include <linux/const.h>
 
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE - 1))
+#include <vdso/page.h>
 
 #define HPAGE_SHIFT		PMD_SHIFT
 #define HPAGE_SIZE		(_AC(1, UL) << HPAGE_SHIFT)
diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h
index 16e4caa931f1..773e0aee8ce3 100644
--- a/arch/s390/include/asm/page.h
+++ b/arch/s390/include/asm/page.h
@@ -11,9 +11,7 @@
 #include <linux/const.h>
 #include <asm/types.h>
 
-#define _PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define _PAGE_SIZE	(_AC(1, UL) << _PAGE_SHIFT)
-#define _PAGE_MASK	(~(_PAGE_SIZE - 1))
+#include <vdso/page.h>
 
 /* PAGE_SHIFT determines the page size */
 #define PAGE_SHIFT	_PAGE_SHIFT
diff --git a/arch/sh/include/asm/page.h b/arch/sh/include/asm/page.h
index f780b467e75d..fc39b8171bfb 100644
--- a/arch/sh/include/asm/page.h
+++ b/arch/sh/include/asm/page.h
@@ -8,10 +8,8 @@
 
 #include <linux/const.h>
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
+
 #define PTE_MASK	PAGE_MASK
 
 #if defined(CONFIG_HUGETLB_PAGE_SIZE_64K)
diff --git a/arch/sparc/include/asm/page_32.h b/arch/sparc/include/asm/page_32.h
index 9977c77374cd..9954254ea569 100644
--- a/arch/sparc/include/asm/page_32.h
+++ b/arch/sparc/include/asm/page_32.h
@@ -11,9 +11,7 @@
 
 #include <linux/const.h>
 
-#define PAGE_SHIFT   CONFIG_PAGE_SHIFT
-#define PAGE_SIZE    (_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK    (~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/sparc/include/asm/page_64.h b/arch/sparc/include/asm/page_64.h
index e9bd24821c93..2a68ff5b6eab 100644
--- a/arch/sparc/include/asm/page_64.h
+++ b/arch/sparc/include/asm/page_64.h
@@ -4,9 +4,7 @@
 
 #include <linux/const.h>
 
-#define PAGE_SHIFT   CONFIG_PAGE_SHIFT
-#define PAGE_SIZE    (_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK    (~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 /* Flushing for D-cache alias handling is only needed if
  * the page size is smaller than 16K.
diff --git a/arch/um/include/asm/page.h b/arch/um/include/asm/page.h
index 9ef9a8aedfa6..834313ecd3d6 100644
--- a/arch/um/include/asm/page.h
+++ b/arch/um/include/asm/page.h
@@ -9,10 +9,7 @@
 
 #include <linux/const.h>
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(_AC(1, UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
index 52f1b4ff0cc1..974688973cf6 100644
--- a/arch/x86/include/asm/page_types.h
+++ b/arch/x86/include/asm/page_types.h
@@ -6,10 +6,7 @@
 #include <linux/types.h>
 #include <linux/mem_encrypt.h>
 
-/* PAGE_SHIFT determines the page size */
-#define PAGE_SHIFT		CONFIG_PAGE_SHIFT
-#define PAGE_SIZE		(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK		(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
 
diff --git a/arch/xtensa/include/asm/page.h b/arch/xtensa/include/asm/page.h
index 4db56ef052d2..595c1037b738 100644
--- a/arch/xtensa/include/asm/page.h
+++ b/arch/xtensa/include/asm/page.h
@@ -18,13 +18,7 @@
 #include <asm/cache.h>
 #include <asm/kmem_layout.h>
 
-/*
- * PAGE_SHIFT determines the page size
- */
-
-#define PAGE_SHIFT	CONFIG_PAGE_SHIFT
-#define PAGE_SIZE	(__XTENSA_UL_CONST(1) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#include <vdso/page.h>
 
 #ifdef CONFIG_MMU
 #define PAGE_OFFSET	XCHAL_KSEG_CACHED_VADDR
diff --git a/include/vdso/page.h b/include/vdso/page.h
new file mode 100644
index 000000000000..df8d307f38cd
--- /dev/null
+++ b/include/vdso/page.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_PAGE_H
+#define __VDSO_PAGE_H
+
+#include <uapi/linux/const.h>
+
+/* PAGE_SHIFT determines the page size */
+#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
+
+#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
+
+#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
+#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
+#else
+#define PAGE_MASK	(~(PAGE_SIZE-1))
+#endif
+
+#endif	/* __VDSO_PAGE_H */
-- 
2.34.1

Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

[Arnd Bergmann]

On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
>
> Introduce vdso/page.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>

Thanks for the new version. This looks all good, just some
very minor ideas for how to possibly improve the new version:

> +/* PAGE_SHIFT determines the page size */
> +#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
> +
> +#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
> +
> +#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
> +#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
> +#else
> +#define PAGE_MASK	(~(PAGE_SIZE-1))
> +#endif

I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
and PAGE_MASK, just to avoid the extra indirection in the
preprocessor. This mainly has the benefit of slightly
shorter compiler warnings when all the macros get
traced back but can also slightly improve compile speed
in case this is used in deeply nested macros.

Without a comment, the special case for CONFIG_X86_64
not very clear, and probably not needed. If you are
worried about introducing an architecture specific
regression, I would suggest instead explaining the
possible issue in the patch description but using the
more generic and simpler #ifdef check.

      Arnd

Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

[Vincenzo Frascino]

On 23/09/2024 17:54, Arnd Bergmann wrote:
> On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
>> The VDSO implementation includes headers from outside of the
>> vdso/ namespace.
>>
>> Introduce vdso/page.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>
> 
> Thanks for the new version. This looks all good, just some
> very minor ideas for how to possibly improve the new version:
> 

Thanks Arnd.

>> +/* PAGE_SHIFT determines the page size */
>> +#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
>> +
>> +#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
>> +
>> +#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
>> +#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
>> +#else
>> +#define PAGE_MASK	(~(PAGE_SIZE-1))
>> +#endif
> 
> I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
> and PAGE_MASK, just to avoid the extra indirection in the
> preprocessor. This mainly has the benefit of slightly
> shorter compiler warnings when all the macros get
> traced back but can also slightly improve compile speed
> in case this is used in deeply nested macros.
> 

I will fix it in the next iteration.

> Without a comment, the special case for CONFIG_X86_64
> not very clear, and probably not needed. If you are
> worried about introducing an architecture specific
> regression, I would suggest instead explaining the
> possible issue in the patch description but using the
> more generic and simpler #ifdef check.
> 

If I do not add the #ifdef, it does not build. But you are right, I should have
put a comment in the commit message.

Regression below:

drivers/gpu/drm/i915/gt/intel_gt_print.h:29:36: error: format ‘%lx’ expects
argument of type ‘long unsigned int’, but argument 6 has type ‘u32’ {aka
‘unsigned int’} [-Werror=format=]
   29 |         drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
      |                                    ^~~~~~~~
include/drm/drm_print.h:424:39: note: in definition of macro ‘drm_dev_dbg’
  424 |         __drm_dev_dbg(NULL, dev, cat, fmt, ##__VA_ARGS__)
      |                                       ^~~
include/drm/drm_print.h:524:33: note: in expansion of macro ‘drm_dbg_driver’
  524 | #define drm_dbg(drm, fmt, ...)  drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
      |                                 ^~~~~~~~~~~~~~
linux/drivers/gpu/drm/i915/gt/intel_gt_print.h:29:9: note: in expansion of macro
‘drm_dbg’
   29 |         drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
      |         ^~~~~~~
drivers/gpu/drm/i915/gt/intel_gt.c:310:25: note: in expansion of macro ‘gt_dbg’
  310 |                         gt_dbg(gt, "Unexpected fault\n"
      |                         ^~~~~~

I am open to alternative suggestions.

>       Arnd

-- 
Regards,
Vincenzo

Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

[Christophe Leroy]

Le 24/09/2024 à 16:10, Vincenzo Frascino a écrit :
> 
> 
> On 23/09/2024 17:54, Arnd Bergmann wrote:
>> On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
>>> The VDSO implementation includes headers from outside of the
>>> vdso/ namespace.
>>>
>>> Introduce vdso/page.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>
>>
>> Thanks for the new version. This looks all good, just some
>> very minor ideas for how to possibly improve the new version:
>>
> 
> Thanks Arnd.
> 
>>> +/* PAGE_SHIFT determines the page size */
>>> +#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
>>> +
>>> +#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
>>> +
>>> +#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
>>> +#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
>>> +#else
>>> +#define PAGE_MASK	(~(PAGE_SIZE-1))
>>> +#endif
>>
>> I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
>> and PAGE_MASK, just to avoid the extra indirection in the
>> preprocessor. This mainly has the benefit of slightly
>> shorter compiler warnings when all the macros get
>> traced back but can also slightly improve compile speed
>> in case this is used in deeply nested macros.
>>
> 
> I will fix it in the next iteration.
> 
>> Without a comment, the special case for CONFIG_X86_64
>> not very clear, and probably not needed. If you are
>> worried about introducing an architecture specific
>> regression, I would suggest instead explaining the
>> possible issue in the patch description but using the
>> more generic and simpler #ifdef check.
>>
> 
> If I do not add the #ifdef, it does not build. But you are right, I should have
> put a comment in the commit message.
> 
> Regression below:
> 
> drivers/gpu/drm/i915/gt/intel_gt_print.h:29:36: error: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 6 has type ‘u32’ {aka
> ‘unsigned int’} [-Werror=format=]
>     29 |         drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
> ##__VA_ARGS__)
>        |                                    ^~~~~~~~
> include/drm/drm_print.h:424:39: note: in definition of macro ‘drm_dev_dbg’
>    424 |         __drm_dev_dbg(NULL, dev, cat, fmt, ##__VA_ARGS__)
>        |                                       ^~~
> include/drm/drm_print.h:524:33: note: in expansion of macro ‘drm_dbg_driver’
>    524 | #define drm_dbg(drm, fmt, ...)  drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
>        |                                 ^~~~~~~~~~~~~~
> linux/drivers/gpu/drm/i915/gt/intel_gt_print.h:29:9: note: in expansion of macro
> ‘drm_dbg’
>     29 |         drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
> ##__VA_ARGS__)
>        |         ^~~~~~~
> drivers/gpu/drm/i915/gt/intel_gt.c:310:25: note: in expansion of macro ‘gt_dbg’
>    310 |                         gt_dbg(gt, "Unexpected fault\n"
>        |                         ^~~~~~
> 
> I am open to alternative suggestions.
> 


'fault' is an 'u32' and 'mask' should be agnostic so the format should 
be %x not %lx I think:

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index a6c69a706fd7..352ef5e1c615 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -308,7 +308,7 @@ static void gen6_check_faults(struct intel_gt *gt)
  		fault = GEN6_RING_FAULT_REG_READ(engine);
  		if (fault & RING_FAULT_VALID) {
  			gt_dbg(gt, "Unexpected fault\n"
-			       "\tAddr: 0x%08lx\n"
+			       "\tAddr: 0x%08x\n"
  			       "\tAddress space: %s\n"
  			       "\tSource ID: %d\n"
  			       "\tType: %d\n",


Christophe

Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

[Vincenzo Frascino]

On 24/09/2024 15:28, Christophe Leroy wrote:
> 'fault' is an 'u32' and 'mask' should be agnostic so the format should 
> be %x not %lx I think:
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index a6c69a706fd7..352ef5e1c615 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -308,7 +308,7 @@ static void gen6_check_faults(struct intel_gt *gt)
>   		fault = GEN6_RING_FAULT_REG_READ(engine);
>   		if (fault & RING_FAULT_VALID) {
>   			gt_dbg(gt, "Unexpected fault\n"
> - "\tAddr: 0x%08lx\n"
> + "\tAddr: 0x%08x\n"
>   			       "\tAddress space: %s\n"
>   			       "\tSource ID: %d\n"
>   			       "\tType: %d\n",

Good catch Christoph. It makes sense, I did not notice the "l". I will add it to
my series.

-- 
Regards,
Vincenzo

Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Introduce vdso/page.h to make sure that the generic library
> uses only the allowed namespace.

This patch looks good to me, maybe it is worth some more description, 
for instance to explain why PAGE_MASK has two different definitions.

Christophe

[PATCH v2 5/8] x86: vdso: Modify asm/vdso/getrandom.h to include datapage

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Modify asm/vdso/getrandom.h to include datapage.

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/getrandom.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
index ff5334ad32a0..4597d5a6f094 100644
--- a/arch/x86/include/asm/vdso/getrandom.h
+++ b/arch/x86/include/asm/vdso/getrandom.h
@@ -7,6 +7,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <vdso/datapage.h>
+
 #include <asm/unistd.h>
 #include <asm/vvar.h>
 
-- 
2.34.1

Re: [PATCH v2 5/8] x86: vdso: Modify asm/vdso/getrandom.h to include datapage

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Modify asm/vdso/getrandom.h to include datapage.

I may be fine with that patch but it should explain why this is needed.

> 
> 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/getrandom.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
> index ff5334ad32a0..4597d5a6f094 100644
> --- a/arch/x86/include/asm/vdso/getrandom.h
> +++ b/arch/x86/include/asm/vdso/getrandom.h
> @@ -7,6 +7,8 @@
>   
>   #ifndef __ASSEMBLY__
>   
> +#include <vdso/datapage.h>
> +
>   #include <asm/unistd.h>
>   #include <asm/vvar.h>
>   

[PATCH v2 6/8] vdso: Modify vdso/getrandom.h to include the asm header

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Modify vdso/getrandom.h to include the getrandom asm header.

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>
---
 include/vdso/getrandom.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
index 6ca4d6de9e46..5cf3f75d6fb6 100644
--- a/include/vdso/getrandom.h
+++ b/include/vdso/getrandom.h
@@ -7,6 +7,7 @@
 #define _VDSO_GETRANDOM_H
 
 #include <linux/types.h>
+#include <asm/vdso/getrandom.h>
 
 #define CHACHA_KEY_SIZE         32
 #define CHACHA_BLOCK_SIZE       64
-- 
2.34.1

Re: [PATCH v2 6/8] vdso: Modify vdso/getrandom.h to include the asm header

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Modify vdso/getrandom.h to include the getrandom asm header.

You should explain why this is needed, i.e. what item in 
vdso/getrandom.h requires asm/vdso/getrandom.h

> 
> 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>
> ---
>   include/vdso/getrandom.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
> index 6ca4d6de9e46..5cf3f75d6fb6 100644
> --- a/include/vdso/getrandom.h
> +++ b/include/vdso/getrandom.h
> @@ -7,6 +7,7 @@
>   #define _VDSO_GETRANDOM_H
>   
>   #include <linux/types.h>
> +#include <asm/vdso/getrandom.h>
>   
>   #define CHACHA_KEY_SIZE         32
>   #define CHACHA_BLOCK_SIZE       64

[PATCH v2 7/8] vdso: Introduce uapi/vdso/random.h

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Introduce uapi/vdso/random.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>
---
 include/uapi/linux/random.h | 26 +------------------------
 include/uapi/vdso/random.h  | 38 +++++++++++++++++++++++++++++++++++++
 include/vdso/datapage.h     |  1 +
 3 files changed, 40 insertions(+), 25 deletions(-)
 create mode 100644 include/uapi/vdso/random.h

diff --git a/include/uapi/linux/random.h b/include/uapi/linux/random.h
index 1dd047ec98a1..dc43ed800212 100644
--- a/include/uapi/linux/random.h
+++ b/include/uapi/linux/random.h
@@ -44,30 +44,6 @@ struct rand_pool_info {
 	__u32	buf[];
 };
 
-/*
- * Flags for getrandom(2)
- *
- * GRND_NONBLOCK	Don't block and return EAGAIN instead
- * GRND_RANDOM		No effect
- * GRND_INSECURE	Return non-cryptographic random bytes
- */
-#define GRND_NONBLOCK	0x0001
-#define GRND_RANDOM	0x0002
-#define GRND_INSECURE	0x0004
-
-/**
- * struct vgetrandom_opaque_params - arguments for allocating memory for vgetrandom
- *
- * @size_per_opaque_state:	Size of each state that is to be passed to vgetrandom().
- * @mmap_prot:			Value of the prot argument in mmap(2).
- * @mmap_flags:			Value of the flags argument in mmap(2).
- * @reserved:			Reserved for future use.
- */
-struct vgetrandom_opaque_params {
-	__u32 size_of_opaque_state;
-	__u32 mmap_prot;
-	__u32 mmap_flags;
-	__u32 reserved[13];
-};
+#include <vdso/random.h>
 
 #endif /* _UAPI_LINUX_RANDOM_H */
diff --git a/include/uapi/vdso/random.h b/include/uapi/vdso/random.h
new file mode 100644
index 000000000000..5c80995129c2
--- /dev/null
+++ b/include/uapi/vdso/random.h
@@ -0,0 +1,38 @@
+
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * include/vdso/random.h
+ *
+ * Include file for the random number generator.
+ */
+
+#ifndef _UAPI_VDSO_RANDOM_H
+#define _UAPI_VDSO_RANDOM_H
+
+/*
+ * Flags for getrandom(2)
+ *
+ * GRND_NONBLOCK	Don't block and return EAGAIN instead
+ * GRND_RANDOM		No effect
+ * GRND_INSECURE	Return non-cryptographic random bytes
+ */
+#define GRND_NONBLOCK	0x0001
+#define GRND_RANDOM	0x0002
+#define GRND_INSECURE	0x0004
+
+/**
+ * struct vgetrandom_opaque_params - arguments for allocating memory for vgetrandom
+ *
+ * @size_per_opaque_state:	Size of each state that is to be passed to vgetrandom().
+ * @mmap_prot:			Value of the prot argument in mmap(2).
+ * @mmap_flags:			Value of the flags argument in mmap(2).
+ * @reserved:			Reserved for future use.
+ */
+struct vgetrandom_opaque_params {
+	__u32 size_of_opaque_state;
+	__u32 mmap_prot;
+	__u32 mmap_flags;
+	__u32 reserved[13];
+};
+
+#endif /* _UAPI_VDSO_RANDOM_H */
diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index b85f24cac3f5..b7d6c71f20c1 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -8,6 +8,7 @@
 #include <uapi/linux/time.h>
 #include <uapi/linux/types.h>
 #include <uapi/asm-generic/errno-base.h>
+#include <uapi/vdso/random.h>
 
 #include <vdso/bits.h>
 #include <vdso/clocksource.h>
-- 
2.34.1

Re: [PATCH v2 7/8] vdso: Introduce uapi/vdso/random.h

[Jason A. Donenfeld]

On Mon, Sep 23, 2024 at 03:19:42PM +0100, Vincenzo Frascino wrote:
> --- a/include/uapi/linux/random.h
> +++ b/include/uapi/linux/random.h
> @@ -44,30 +44,6 @@ struct rand_pool_info {
>  	__u32	buf[];
>  };
>  
> -/*
> - * Flags for getrandom(2)
> - *
> - * GRND_NONBLOCK	Don't block and return EAGAIN instead
> - * GRND_RANDOM		No effect
> - * GRND_INSECURE	Return non-cryptographic random bytes
> - */
> -#define GRND_NONBLOCK	0x0001
> -#define GRND_RANDOM	0x0002
> -#define GRND_INSECURE	0x0004
> -
> -/**
> - * struct vgetrandom_opaque_params - arguments for allocating memory for vgetrandom
> - *
> - * @size_per_opaque_state:	Size of each state that is to be passed to vgetrandom().
> - * @mmap_prot:			Value of the prot argument in mmap(2).
> - * @mmap_flags:			Value of the flags argument in mmap(2).
> - * @reserved:			Reserved for future use.
> - */
> -struct vgetrandom_opaque_params {
> -	__u32 size_of_opaque_state;
> -	__u32 mmap_prot;
> -	__u32 mmap_flags;
> -	__u32 reserved[13];
> -};
> +#include <vdso/random.h>
>  
>  #endif /* _UAPI_LINUX_RANDOM_H */
> diff --git a/include/uapi/vdso/random.h b/include/uapi/vdso/random.h
> new file mode 100644
> index 000000000000..5c80995129c2
> --- /dev/null
> +++ b/include/uapi/vdso/random.h
> @@ -0,0 +1,38 @@
> +

I really do not like this. This is UAPI, and it's linux/something.h
style of UAPI. What does moving it to vdso/ accomplish except confusion
for people looking where the code is and then polluting users'
/usr/include with extra directories that aren't meaningful?

A change like this makes me think the approach taken by this patchset
might not be the right one.

Re: [PATCH v2 7/8] vdso: Introduce uapi/vdso/random.h

[Vincenzo Frascino]

On 24/09/2024 00:09, Jason A. Donenfeld wrote:
> On Mon, Sep 23, 2024 at 03:19:42PM +0100, Vincenzo Frascino wrote:
>> --- a/include/uapi/linux/random.h
>> +++ b/include/uapi/linux/random.h
>> @@ -44,30 +44,6 @@ struct rand_pool_info {
>>  	__u32	buf[];
>>  };
>>  
>> -/*
>> - * Flags for getrandom(2)
>> - *
>> - * GRND_NONBLOCK	Don't block and return EAGAIN instead
>> - * GRND_RANDOM		No effect
>> - * GRND_INSECURE	Return non-cryptographic random bytes
>> - */
>> -#define GRND_NONBLOCK	0x0001
>> -#define GRND_RANDOM	0x0002
>> -#define GRND_INSECURE	0x0004
>> -
>> -/**
>> - * struct vgetrandom_opaque_params - arguments for allocating memory for vgetrandom
>> - *
>> - * @size_per_opaque_state:	Size of each state that is to be passed to vgetrandom().
>> - * @mmap_prot:			Value of the prot argument in mmap(2).
>> - * @mmap_flags:			Value of the flags argument in mmap(2).
>> - * @reserved:			Reserved for future use.
>> - */
>> -struct vgetrandom_opaque_params {
>> -	__u32 size_of_opaque_state;
>> -	__u32 mmap_prot;
>> -	__u32 mmap_flags;
>> -	__u32 reserved[13];
>> -};
>> +#include <vdso/random.h>
>>  
>>  #endif /* _UAPI_LINUX_RANDOM_H */
>> diff --git a/include/uapi/vdso/random.h b/include/uapi/vdso/random.h
>> new file mode 100644
>> index 000000000000..5c80995129c2
>> --- /dev/null
>> +++ b/include/uapi/vdso/random.h
>> @@ -0,0 +1,38 @@
>> +
> 
> I really do not like this. This is UAPI, and it's linux/something.h
> style of UAPI. What does moving it to vdso/ accomplish except confusion
> for people looking where the code is and then polluting users'
> /usr/include with extra directories that aren't meaningful?
> 
> A change like this makes me think the approach taken by this patchset
> might not be the right one.

The rationale was explained in my comment on patch 1/8. If you do not like the
vdso/ namespace in uapi/ could you please let me know what is your preference is
isolating the parts needed by the vdso library?

-- 
Regards,
Vincenzo

Re: [PATCH v2 7/8] vdso: Introduce uapi/vdso/random.h

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Introduce uapi/vdso/random.h to make sure that the generic
> library uses only the allowed namespace.

I can't see the reason for this change.

VDSO is userland code and should be safe to include any UAPI header.

Christophe

[PATCH v2 8/8] vdso: Modify getrandom to include the correct namespace.

[Vincenzo Frascino]

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Modify getrandom to take advantage of the refactoring done in the
previous patches and to include only the vdso/ 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>
---
 include/vdso/datapage.h |  1 +
 lib/vdso/getrandom.c    | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index b7d6c71f20c1..127f0c51bf01 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -5,6 +5,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/compiler.h>
+#include <linux/build_bug.h>
 #include <uapi/linux/time.h>
 #include <uapi/linux/types.h>
 #include <uapi/asm-generic/errno-base.h>
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 938ca539aaa6..e15d3cf768c9 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -3,19 +3,19 @@
  * Copyright (C) 2022-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
  */
 
-#include <linux/array_size.h>
-#include <linux/minmax.h>
 #include <vdso/datapage.h>
 #include <vdso/getrandom.h>
 #include <vdso/unaligned.h>
-#include <asm/vdso/getrandom.h>
-#include <uapi/linux/mman.h>
-#include <uapi/linux/random.h>
+#include <vdso/mman.h>
+#include <vdso/page.h>
 
-#undef PAGE_SIZE
-#undef PAGE_MASK
-#define PAGE_SIZE (1UL << CONFIG_PAGE_SHIFT)
-#define PAGE_MASK (~(PAGE_SIZE - 1))
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x)	(sizeof(x) / sizeof(*x))
+#endif
+
+#ifndef min_t
+#define min_t(type,a,b)	((type)(a) < (type)(b) ? (type)(a) : (type)(b))
+#endif
 
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
@@ -79,8 +79,8 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 	if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags)) {
 		struct vgetrandom_opaque_params *params = opaque_state;
 		params->size_of_opaque_state = sizeof(*state);
-		params->mmap_prot = PROT_READ | PROT_WRITE;
-		params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
+		params->mmap_prot = VDSO_MMAP_PROT;
+		params->mmap_flags = VDSO_MMAP_FLAGS;
 		for (size_t i = 0; i < ARRAY_SIZE(params->reserved); ++i)
 			params->reserved[i] = 0;
 		return 0;
-- 
2.34.1

Re: [PATCH v2 8/8] vdso: Modify getrandom to include the correct namespace.

[Jason A. Donenfeld]

On Mon, Sep 23, 2024 at 03:19:43PM +0100, Vincenzo Frascino wrote:
> -		params->mmap_prot = PROT_READ | PROT_WRITE;
> -		params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
> +		params->mmap_prot = VDSO_MMAP_PROT;
> +		params->mmap_flags = VDSO_MMAP_FLAGS;

The code that's being deleted is meaningful and descriptive. The code
that's being added is confusing. What on earth is a vdso mmap flag? Not
only is it indirection, which makes it harder to understand, but its
indirection through a meaninglessly generic name that suggests to the
user there's some additional property of the vdso or mmap or both that
would imply a specific flag for these general things. In reality, the
thing in question is about what getrandom.c uses.

Re: [PATCH v2 8/8] vdso: Modify getrandom to include the correct namespace.

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Modify getrandom to take advantage of the refactoring done in the
> previous patches and to include only the vdso/ 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>
> ---
>   include/vdso/datapage.h |  1 +
>   lib/vdso/getrandom.c    | 22 +++++++++++-----------
>   2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
> index b7d6c71f20c1..127f0c51bf01 100644
> --- a/include/vdso/datapage.h
> +++ b/include/vdso/datapage.h
> @@ -5,6 +5,7 @@
>   #ifndef __ASSEMBLY__
>   
>   #include <linux/compiler.h>
> +#include <linux/build_bug.h>

What in this datapage.h requires this build_bug header ?

>   #include <uapi/linux/time.h>
>   #include <uapi/linux/types.h>
>   #include <uapi/asm-generic/errno-base.h>
> diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
> index 938ca539aaa6..e15d3cf768c9 100644
> --- a/lib/vdso/getrandom.c
> +++ b/lib/vdso/getrandom.c
> @@ -3,19 +3,19 @@
>    * Copyright (C) 2022-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>    */
>   
> -#include <linux/array_size.h>
> -#include <linux/minmax.h>
>   #include <vdso/datapage.h>
>   #include <vdso/getrandom.h>
>   #include <vdso/unaligned.h>
> -#include <asm/vdso/getrandom.h>
> -#include <uapi/linux/mman.h>
> -#include <uapi/linux/random.h>
> +#include <vdso/mman.h>

This change is not needed, asm/vdso/getrandom.h is in VDSO namespace, 
and the other two are UAPI headers which must be safe to include in VDSO 
code as VDSO code in userland code.

> +#include <vdso/page.h>
>   
> -#undef PAGE_SIZE
> -#undef PAGE_MASK
> -#define PAGE_SIZE (1UL << CONFIG_PAGE_SHIFT)
> -#define PAGE_MASK (~(PAGE_SIZE - 1))
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x)	(sizeof(x) / sizeof(*x))
> +#endif
> +
> +#ifndef min_t
> +#define min_t(type,a,b)	((type)(a) < (type)(b) ? (type)(a) : (type)(b))
> +#endif

Would be better to force undefine/redefine ARRAY_SIZE and min_t instead 
of defining them only when they don't exist already.

>   
>   #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
>   	while (len >= sizeof(type)) {						\
> @@ -79,8 +79,8 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
>   	if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags)) {
>   		struct vgetrandom_opaque_params *params = opaque_state;
>   		params->size_of_opaque_state = sizeof(*state);
> -		params->mmap_prot = PROT_READ | PROT_WRITE;
> -		params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
> +		params->mmap_prot = VDSO_MMAP_PROT;
> +		params->mmap_flags = VDSO_MMAP_FLAGS;

At the time being the flags and prot are the same for all architectures, 
there is no point in introducing VDSO_MMAP_PROT and VDSO_MMAP_FLAGS. 
Maybe one day that may be needed, but until that day nothing should be 
changed, unless you already have in mind and describe an architecture 
that will need that.

Christophe

>   		for (size_t i = 0; i < ARRAY_SIZE(params->reserved); ++i)
>   			params->reserved[i] = 0;
>   		return 0;

Re: [PATCH v2 0/8] vdso: Use only headers from the vdso/ namespace

[Christophe Leroy]

Le 23/09/2024 à 16:19, Vincenzo Frascino a écrit :
> The recent implementation of getrandom in the generic vdso library,
> includes headers from outside of the vdso/ namespace.
> 
> The purpose of this patch series is to refactor the code to make sure
> that the library uses only the allowed namespace.
> 
> The series has been rebased on [1] to simplify the testing.
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git master

This tree includes random support for s390, x86, arm64 and loongarch but 
your series only handles x86 and arm64 it seems.

Christophe