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

[PATCH 0/9] 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

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 (9):
  x86: vdso: Introduce asm/vdso/mman.h
  vdso: Introduce vdso/mman.h
  x86: vdso: Introduce asm/vdso/page.h
  vdso: Introduce vdso/page.h
  vdso: Split linux/minmax.h
  vdso: Split linux/array_size.h
  x86: vdso: Modify asm/vdso/getrandom.h to include datapage
  vdso: Modify vdso/getrandom.h to include the asm header
  vdso: Modify getrandom to include the correct namespace

 arch/x86/include/asm/vdso/getrandom.h |  2 ++
 arch/x86/include/asm/vdso/mman.h      | 15 +++++++++++
 arch/x86/include/asm/vdso/page.h      | 15 +++++++++++
 include/linux/array_size.h            |  8 +-----
 include/linux/minmax.h                | 28 +-------------------
 include/vdso/array_size.h             | 13 +++++++++
 include/vdso/getrandom.h              |  1 +
 include/vdso/minmax.h                 | 38 +++++++++++++++++++++++++++
 include/vdso/mman.h                   |  7 +++++
 include/vdso/page.h                   |  7 +++++
 lib/vdso/getrandom.c                  | 22 ++++++----------
 11 files changed, 108 insertions(+), 48 deletions(-)
 create mode 100644 arch/x86/include/asm/vdso/mman.h
 create mode 100644 arch/x86/include/asm/vdso/page.h
 create mode 100644 include/vdso/array_size.h
 create mode 100644 include/vdso/minmax.h
 create mode 100644 include/vdso/mman.h
 create mode 100644 include/vdso/page.h

-- 
2.34.1

[PATCH 1/9] 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

[PATCH 2/9] 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

[PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Vincenzo Frascino]

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

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

diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h
new file mode 100644
index 000000000000..b0af8fbef27c
--- /dev/null
+++ b/arch/x86/include/asm/vdso/page.h
@@ -0,0 +1,15 @@
+
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_VDSO_PAGE_H
+#define __ASM_VDSO_PAGE_H
+
+#ifndef __ASSEMBLY__
+
+#include <asm/page_types.h>
+
+#define VDSO_PAGE_MASK	PAGE_MASK
+#define VDSO_PAGE_SIZE	PAGE_SIZE
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_PAGE_H */
-- 
2.34.1

[PATCH 4/9] 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>
---
 include/vdso/page.h | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 include/vdso/page.h

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

[PATCH 5/9] vdso: Split linux/minmax.h

[Vincenzo Frascino]

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

Split linux/minmax.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/linux/minmax.h | 28 +---------------------------
 include/vdso/minmax.h  | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 27 deletions(-)
 create mode 100644 include/vdso/minmax.h

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 98008dd92153..846e3fa65c96 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -6,6 +6,7 @@
 #include <linux/compiler.h>
 #include <linux/const.h>
 #include <linux/types.h>
+#include <vdso/minmax.h>
 
 /*
  * min()/max()/clamp() macros must accomplish three things:
@@ -84,17 +85,6 @@
 #define __types_ok3(x,y,z,ux,uy,uz) \
 	(__sign_use(x,ux) & __sign_use(y,uy) & __sign_use(z,uz))
 
-#define __cmp_op_min <
-#define __cmp_op_max >
-
-#define __cmp(op, x, y)	((x) __cmp_op_##op (y) ? (x) : (y))
-
-#define __cmp_once_unique(op, type, x, y, ux, uy) \
-	({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
-
-#define __cmp_once(op, type, x, y) \
-	__cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
-
 #define __careful_cmp_once(op, x, y, ux, uy) ({		\
 	__auto_type ux = (x); __auto_type uy = (y);	\
 	BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy),	\
@@ -204,22 +194,6 @@
  * Or not use min/max/clamp at all, of course.
  */
 
-/**
- * min_t - return minimum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
- */
-#define min_t(type, x, y) __cmp_once(min, type, x, y)
-
-/**
- * max_t - return maximum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
- */
-#define max_t(type, x, y) __cmp_once(max, type, x, y)
-
 /*
  * Do not check the array parameter using __must_be_array().
  * In the following legit use-case where the "array" passed is a simple pointer,
diff --git a/include/vdso/minmax.h b/include/vdso/minmax.h
new file mode 100644
index 000000000000..26724f34c513
--- /dev/null
+++ b/include/vdso/minmax.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_MINMAX_H
+#define __VDSO_MINMAX_H
+
+#ifndef __ASSEMBLY__
+
+#include <linux/compiler.h>
+
+#define __cmp_op_min <
+#define __cmp_op_max >
+
+#define __cmp(op, x, y)	((x) __cmp_op_##op (y) ? (x) : (y))
+
+#define __cmp_once_unique(op, type, x, y, ux, uy) \
+	({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
+
+#define __cmp_once(op, type, x, y) \
+	__cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
+
+/**
+ * min_t - return minimum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define min_t(type, x, y) __cmp_once(min, type, x, y)
+
+/**
+ * max_t - return maximum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define max_t(type, x, y) __cmp_once(max, type, x, y)
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __VDSO_MINMAX_H */
-- 
2.34.1

[PATCH 6/9] vdso: Split linux/array_size.h

[Vincenzo Frascino]

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

Split linux/array_size.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/linux/array_size.h |  8 +-------
 include/vdso/array_size.h  | 13 +++++++++++++
 2 files changed, 14 insertions(+), 7 deletions(-)
 create mode 100644 include/vdso/array_size.h

diff --git a/include/linux/array_size.h b/include/linux/array_size.h
index 06d7d83196ca..ca9e63b419c4 100644
--- a/include/linux/array_size.h
+++ b/include/linux/array_size.h
@@ -2,12 +2,6 @@
 #ifndef _LINUX_ARRAY_SIZE_H
 #define _LINUX_ARRAY_SIZE_H
 
-#include <linux/compiler.h>
-
-/**
- * ARRAY_SIZE - get the number of elements in array @arr
- * @arr: array to be sized
- */
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+#include <vdso/array_size.h>
 
 #endif  /* _LINUX_ARRAY_SIZE_H */
diff --git a/include/vdso/array_size.h b/include/vdso/array_size.h
new file mode 100644
index 000000000000..4079f7a5f86e
--- /dev/null
+++ b/include/vdso/array_size.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _VDSO_ARRAY_SIZE_H
+#define _VDSO_ARRAY_SIZE_H
+
+#include <linux/compiler.h>
+
+/**
+ * ARRAY_SIZE - get the number of elements in array @arr
+ * @arr: array to be sized
+ */
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+
+#endif  /* _VDSO_ARRAY_SIZE_H */
-- 
2.34.1

[PATCH 7/9] 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

[PATCH 8/9] 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

[PATCH 9/9] 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>
---
 lib/vdso/getrandom.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 938ca539aaa6..9c26b738e4a1 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -3,19 +3,13 @@
  * 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>
-
-#undef PAGE_SIZE
-#undef PAGE_MASK
-#define PAGE_SIZE (1UL << CONFIG_PAGE_SHIFT)
-#define PAGE_MASK (~(PAGE_SIZE - 1))
+#include <vdso/mman.h>
+#include <vdso/page.h>
+#include <vdso/array_size.h>
+#include <vdso/minmax.h>
 
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
@@ -68,7 +62,7 @@ static __always_inline ssize_t
 __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
 		       unsigned int flags, void *opaque_state, size_t opaque_len)
 {
-	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
+	ssize_t ret = min_t(size_t, INT_MAX & VDSO_PAGE_MASK /* = MAX_RW_COUNT */, len);
 	struct vgetrandom_state *state = opaque_state;
 	size_t batch_len, nblocks, orig_len = len;
 	bool in_use, have_retried = false;
@@ -79,15 +73,15 @@ __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;
 	}
 
 	/* The state must not straddle a page, since pages can be zeroed at any time. */
-	if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
+	if (unlikely(((unsigned long)opaque_state & ~VDSO_PAGE_MASK) + sizeof(*state) > VDSO_PAGE_SIZE))
 		return -EFAULT;
 
 	/* Handle unexpected flags by falling back to the kernel. */
-- 
2.34.1

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

[Jason A. Donenfeld]

Christophe explained the issue with this in
https://lore.kernel.org/all/85efc7c5-40c8-4c89-b65f-dd13536fb8c7@cs-soprasteria.com/

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

[Vincenzo Frascino]

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?

-- 
Regards,
Vincenzo

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Arnd Bergmann]

On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:

> diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h
> new file mode 100644
> index 000000000000..b0af8fbef27c
> --- /dev/null
> +++ b/arch/x86/include/asm/vdso/page.h
> @@ -0,0 +1,15 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_VDSO_PAGE_H
> +#define __ASM_VDSO_PAGE_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <asm/page_types.h>
> +
> +#define VDSO_PAGE_MASK	PAGE_MASK
> +#define VDSO_PAGE_SIZE	PAGE_SIZE
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __ASM_VDSO_PAGE_H */

I don't get this one: the x86 asm/page_types.h still includes other
headers outside of the vdso namespace, but you seem to only need these
two definitions that are the same across everything.

Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h
header? I did spend a lot of time a few months ago ensuring that
we can have a single definition for all architectures based on
CONFIG_PAGE_SHIFT, so all the extra copies should just go away.

       Arnd

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Christophe Leroy]

Le 04/09/2024 à 16:52, Arnd Bergmann a écrit :
> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:
> 
>> diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h
>> new file mode 100644
>> index 000000000000..b0af8fbef27c
>> --- /dev/null
>> +++ b/arch/x86/include/asm/vdso/page.h
>> @@ -0,0 +1,15 @@
>> +
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __ASM_VDSO_PAGE_H
>> +#define __ASM_VDSO_PAGE_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <asm/page_types.h>
>> +
>> +#define VDSO_PAGE_MASK	PAGE_MASK
>> +#define VDSO_PAGE_SIZE	PAGE_SIZE
>> +
>> +#endif /* !__ASSEMBLY__ */
>> +
>> +#endif /* __ASM_VDSO_PAGE_H */
> 
> I don't get this one: the x86 asm/page_types.h still includes other
> headers outside of the vdso namespace, but you seem to only need these
> two definitions that are the same across everything.
> 
> Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h
> header? I did spend a lot of time a few months ago ensuring that
> we can have a single definition for all architectures based on
> CONFIG_PAGE_SHIFT, so all the extra copies should just go away.
> 

Just wondering, after looking at x86, powerpc and arm64, is there any 
difference between:

X86,ARM64:
#define PAGE_SIZE		(_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK		(~(PAGE_SIZE-1))

POWERPC:
#define PAGE_SIZE		(ASM_CONST(1) << PAGE_SHIFT)
/*
  * 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))


Which one should be taken in vdso/page.h ?

Christophe

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

[Christophe Leroy]

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

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Christophe Leroy]

Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Introduce asm/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/x86/include/asm/vdso/page.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>   create mode 100644 arch/x86/include/asm/vdso/page.h
> 
> diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h
> new file mode 100644
> index 000000000000..b0af8fbef27c
> --- /dev/null
> +++ b/arch/x86/include/asm/vdso/page.h
> @@ -0,0 +1,15 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_VDSO_PAGE_H
> +#define __ASM_VDSO_PAGE_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <asm/page_types.h>
> +
> +#define VDSO_PAGE_MASK	PAGE_MASK
> +#define VDSO_PAGE_SIZE	PAGE_SIZE

Same, I don't think we need those macros to duplicate PAGE_SIZE and 
PAGE_MASK in each and every architecture.

The best would be to use CONFIG_PAGE_SHIFT which is defined the same way 
on every architecture and refactor PAGE_SIZE and PAGE_MASK and get rid 
of all arch specific definitions of PAGE_SIZE and PAGE_MASK.

> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __ASM_VDSO_PAGE_H */

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

[Christophe Leroy]

Le 03/09/2024 à 17:14, 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.
> 
> 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/page.h | 7 +++++++
>   1 file changed, 7 insertions(+)
>   create mode 100644 include/vdso/page.h
> 
> diff --git a/include/vdso/page.h b/include/vdso/page.h
> new file mode 100644
> index 000000000000..f18e304941cb
> --- /dev/null
> +++ b/include/vdso/page.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __VDSO_PAGE_H
> +#define __VDSO_PAGE_H
> +
> +#include <asm/vdso/page.h>

I can't see the benefit of that, the generic library can directly 
include asm/vdso/page.h

> +
> +#endif	/* __VDSO_PAGE_H */

Re: [PATCH 5/9] vdso: Split linux/minmax.h

[Christophe Leroy]

Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Split linux/minmax.h to make sure that the generic library
> uses only the allowed namespace.

It is probably easier to just don't use min_t() in VDSO. Can be open 
coded without impeeding readability.

> 
> 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/linux/minmax.h | 28 +---------------------------
>   include/vdso/minmax.h  | 38 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 27 deletions(-)
>   create mode 100644 include/vdso/minmax.h
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 98008dd92153..846e3fa65c96 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -6,6 +6,7 @@
>   #include <linux/compiler.h>
>   #include <linux/const.h>
>   #include <linux/types.h>
> +#include <vdso/minmax.h>
>   
>   /*
>    * min()/max()/clamp() macros must accomplish three things:
> @@ -84,17 +85,6 @@
>   #define __types_ok3(x,y,z,ux,uy,uz) \
>   	(__sign_use(x,ux) & __sign_use(y,uy) & __sign_use(z,uz))
>   
> -#define __cmp_op_min <
> -#define __cmp_op_max >
> -
> -#define __cmp(op, x, y)	((x) __cmp_op_##op (y) ? (x) : (y))
> -
> -#define __cmp_once_unique(op, type, x, y, ux, uy) \
> -	({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
> -
> -#define __cmp_once(op, type, x, y) \
> -	__cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
> -
>   #define __careful_cmp_once(op, x, y, ux, uy) ({		\
>   	__auto_type ux = (x); __auto_type uy = (y);	\
>   	BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy),	\
> @@ -204,22 +194,6 @@
>    * Or not use min/max/clamp at all, of course.
>    */
>   
> -/**
> - * min_t - return minimum of two values, using the specified type
> - * @type: data type to use
> - * @x: first value
> - * @y: second value
> - */
> -#define min_t(type, x, y) __cmp_once(min, type, x, y)
> -
> -/**
> - * max_t - return maximum of two values, using the specified type
> - * @type: data type to use
> - * @x: first value
> - * @y: second value
> - */
> -#define max_t(type, x, y) __cmp_once(max, type, x, y)
> -
>   /*
>    * Do not check the array parameter using __must_be_array().
>    * In the following legit use-case where the "array" passed is a simple pointer,
> diff --git a/include/vdso/minmax.h b/include/vdso/minmax.h
> new file mode 100644
> index 000000000000..26724f34c513
> --- /dev/null
> +++ b/include/vdso/minmax.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __VDSO_MINMAX_H
> +#define __VDSO_MINMAX_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <linux/compiler.h>
> +
> +#define __cmp_op_min <
> +#define __cmp_op_max >
> +
> +#define __cmp(op, x, y)	((x) __cmp_op_##op (y) ? (x) : (y))
> +
> +#define __cmp_once_unique(op, type, x, y, ux, uy) \
> +	({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
> +
> +#define __cmp_once(op, type, x, y) \
> +	__cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
> +
> +/**
> + * min_t - return minimum of two values, using the specified type
> + * @type: data type to use
> + * @x: first value
> + * @y: second value
> + */
> +#define min_t(type, x, y) __cmp_once(min, type, x, y)
> +
> +/**
> + * max_t - return maximum of two values, using the specified type
> + * @type: data type to use
> + * @x: first value
> + * @y: second value
> + */
> +#define max_t(type, x, y) __cmp_once(max, type, x, y)
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __VDSO_MINMAX_H */

Re: [PATCH 6/9] vdso: Split linux/array_size.h

[Christophe Leroy]

Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
> 
> Split linux/array_size.h to make sure that the generic library
> uses only the allowed namespace.

There is only one place using ARRAY_SIZE(x), can be open coded as 
sizeof(x)/sizeof(*x) instead.

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/linux/array_size.h |  8 +-------
>   include/vdso/array_size.h  | 13 +++++++++++++
>   2 files changed, 14 insertions(+), 7 deletions(-)
>   create mode 100644 include/vdso/array_size.h
> 
> diff --git a/include/linux/array_size.h b/include/linux/array_size.h
> index 06d7d83196ca..ca9e63b419c4 100644
> --- a/include/linux/array_size.h
> +++ b/include/linux/array_size.h
> @@ -2,12 +2,6 @@
>   #ifndef _LINUX_ARRAY_SIZE_H
>   #define _LINUX_ARRAY_SIZE_H
>   
> -#include <linux/compiler.h>
> -
> -/**
> - * ARRAY_SIZE - get the number of elements in array @arr
> - * @arr: array to be sized
> - */
> -#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> +#include <vdso/array_size.h>
>   
>   #endif  /* _LINUX_ARRAY_SIZE_H */
> diff --git a/include/vdso/array_size.h b/include/vdso/array_size.h
> new file mode 100644
> index 000000000000..4079f7a5f86e
> --- /dev/null
> +++ b/include/vdso/array_size.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _VDSO_ARRAY_SIZE_H
> +#define _VDSO_ARRAY_SIZE_H
> +
> +#include <linux/compiler.h>
> +
> +/**
> + * ARRAY_SIZE - get the number of elements in array @arr
> + * @arr: array to be sized
> + */
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> +
> +#endif  /* _VDSO_ARRAY_SIZE_H */

Re: [PATCH 7/9] x86: vdso: Modify asm/vdso/getrandom.h to include datapage

[Christophe Leroy]

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

Does  asm/vdso/getrandom.h need datapage ? If not it is the ones that 
need it that should include it.

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

Re: [PATCH 5/9] vdso: Split linux/minmax.h

[Arnd Bergmann]

On Wed, Sep 4, 2024, at 17:17, Christophe Leroy wrote:
> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>> The VDSO implementation includes headers from outside of the
>> vdso/ namespace.
>> 
>> Split linux/minmax.h to make sure that the generic library
>> uses only the allowed namespace.
>
> It is probably easier to just don't use min_t() in VDSO. Can be open 
> coded without impeeding readability.

Right, or possibly the even simpler MIN()/MAX() if the arguments
have no side-effects.

       Arnd

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Christophe Leroy]

Le 03/09/2024 à 17:14, 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>
> ---
>   lib/vdso/getrandom.c | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
> index 938ca539aaa6..9c26b738e4a1 100644
> --- a/lib/vdso/getrandom.c
> +++ b/lib/vdso/getrandom.c
> @@ -3,19 +3,13 @@
>    * 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>

Now build fails on powerpc because struct vgetrandom_opaque_params is 
unknown.

x86 get it by chance via the following header inclusion chain:

In file included from ./include/linux/random.h:10,
                  from ./include/linux/nodemask.h:98,
                  from ./include/linux/mmzone.h:18,
                  from ./include/linux/gfp.h:7,
                  from ./include/linux/xarray.h:16,
                  from ./include/linux/radix-tree.h:21,
                  from ./include/linux/idr.h:15,
                  from ./include/linux/kernfs.h:12,
                  from ./include/linux/sysfs.h:16,
                  from ./include/linux/kobject.h:20,
                  from ./include/linux/of.h:18,
                  from ./include/linux/clocksource.h:19,
                  from ./include/clocksource/hyperv_timer.h:16,
                  from ./arch/x86/include/asm/vdso/gettimeofday.h:21,
                  from ./include/vdso/datapage.h:164,
                  from 
arch/x86/entry/vdso/../../../../lib/vdso/getrandom.c:7,
                  from arch/x86/entry/vdso/vgetrandom.c:7:




> -
> -#undef PAGE_SIZE
> -#undef PAGE_MASK
> -#define PAGE_SIZE (1UL << CONFIG_PAGE_SHIFT)
> -#define PAGE_MASK (~(PAGE_SIZE - 1))
> +#include <vdso/mman.h>
> +#include <vdso/page.h>
> +#include <vdso/array_size.h>
> +#include <vdso/minmax.h>
>   
>   #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
>   	while (len >= sizeof(type)) {						\
> @@ -68,7 +62,7 @@ static __always_inline ssize_t
>   __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
>   		       unsigned int flags, void *opaque_state, size_t opaque_len)
>   {
> -	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
> +	ssize_t ret = min_t(size_t, INT_MAX & VDSO_PAGE_MASK /* = MAX_RW_COUNT */, len);
>   	struct vgetrandom_state *state = opaque_state;
>   	size_t batch_len, nblocks, orig_len = len;
>   	bool in_use, have_retried = false;
> @@ -79,15 +73,15 @@ __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;
>   	}
>   
>   	/* The state must not straddle a page, since pages can be zeroed at any time. */
> -	if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
> +	if (unlikely(((unsigned long)opaque_state & ~VDSO_PAGE_MASK) + sizeof(*state) > VDSO_PAGE_SIZE))
>   		return -EFAULT;
>   
>   	/* Handle unexpected flags by falling back to the kernel. */

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

[Vincenzo Frascino]

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

-- 
Regards,
Vincenzo

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Vincenzo Frascino]

Hi Arnd,

On 04/09/2024 15:52, Arnd Bergmann wrote:
> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:
> 

...

>> +
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __ASM_VDSO_PAGE_H
>> +#define __ASM_VDSO_PAGE_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <asm/page_types.h>
>> +
>> +#define VDSO_PAGE_MASK	PAGE_MASK
>> +#define VDSO_PAGE_SIZE	PAGE_SIZE
>> +
>> +#endif /* !__ASSEMBLY__ */
>> +
>> +#endif /* __ASM_VDSO_PAGE_H */
> 
> I don't get this one: the x86 asm/page_types.h still includes other
> headers outside of the vdso namespace, but you seem to only need these
> two definitions that are the same across everything.
> > Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h
> header? I did spend a lot of time a few months ago ensuring that
> we can have a single definition for all architectures based on
> CONFIG_PAGE_SHIFT, so all the extra copies should just go away.
> 
>        Arnd
Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they
all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.:

x86:
#define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)

powerpc:
#define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)

hence I left to the architecture the responsibility of redefining the constants
for the VSDO.

As a long term plan I would like to simplify the code and have a single
definition as you are saying in vdso/page.h for both the macros. But my
preference would be to post it as a separate series so that I can focus more on
validating it properly.

-- 
Regards,
Vincenzo

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Vincenzo Frascino]

On 04/09/2024 16:05, Christophe Leroy wrote:
> 
> 
> Le 04/09/2024 à 16:52, Arnd Bergmann a écrit :
>> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:
>>
...

>>> +
>>> +#ifndef __ASSEMBLY__
>>> +
>>> +#include <asm/page_types.h>
>>> +
>>> +#define VDSO_PAGE_MASK    PAGE_MASK
>>> +#define VDSO_PAGE_SIZE    PAGE_SIZE
>>> +
>>> +#endif /* !__ASSEMBLY__ */
>>> +
>>> +#endif /* __ASM_VDSO_PAGE_H */
>>
>> I don't get this one: the x86 asm/page_types.h still includes other
>> headers outside of the vdso namespace, but you seem to only need these
>> two definitions that are the same across everything.
>>
>> Why not put PAGE_MASK and PAGE_SIZE into a global vdso/page.h
>> header? I did spend a lot of time a few months ago ensuring that
>> we can have a single definition for all architectures based on
>> CONFIG_PAGE_SHIFT, so all the extra copies should just go away.
>>
> 
> Just wondering, after looking at x86, powerpc and arm64, is there any difference
> between:
> 
> X86,ARM64:
> #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)
> #define PAGE_MASK        (~(PAGE_SIZE-1))
> 
> POWERPC:
> #define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)
> /*
>  * 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))
> 
> 
> Which one should be taken in vdso/page.h ?
>

I am not sure either on this point. That's the main reason why I proposed an
indirection for the definitions.

> Christophe

-- 
Regards,
Vincenzo

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

[Vincenzo Frascino]

Hi Christophe,

On 04/09/2024 18:16, Christophe Leroy wrote:
> 
> 
> Le 03/09/2024 à 17:14, 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.
>>
>> 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/page.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>   create mode 100644 include/vdso/page.h
>>
>> diff --git a/include/vdso/page.h b/include/vdso/page.h
>> new file mode 100644
>> index 000000000000..f18e304941cb
>> --- /dev/null
>> +++ b/include/vdso/page.h
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __VDSO_PAGE_H
>> +#define __VDSO_PAGE_H
>> +
>> +#include <asm/vdso/page.h>
> 
> I can't see the benefit of that, the generic library can directly include
> asm/vdso/page.h
> 

I think you agree that any discussion we can have on this point will be made
obsolete by the fact that we will end up defining PAGE_SIZE/PAGE_MASK in
vdso/page.h.

>> +
>> +#endif    /* __VDSO_PAGE_H */

-- 
Regards,
Vincenzo

Re: [PATCH 5/9] vdso: Split linux/minmax.h

[Vincenzo Frascino]

On 04/09/2024 18:23, Arnd Bergmann wrote:
> On Wed, Sep 4, 2024, at 17:17, Christophe Leroy wrote:
>> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>>> The VDSO implementation includes headers from outside of the
>>> vdso/ namespace.
>>>
>>> Split linux/minmax.h to make sure that the generic library
>>> uses only the allowed namespace.
>>
>> It is probably easier to just don't use min_t() in VDSO. Can be open 
>> coded without impeeding readability.
> 
> Right, or possibly the even simpler MIN()/MAX() if the arguments
> have no side-effects.
> 

Agreed, generally I do not like open-coding since it tends to introduce
duplication, but these cases are simple especially if we can use MIN()/MAX().

>        Arnd

-- 
Regards,
Vincenzo

Re: [PATCH 6/9] vdso: Split linux/array_size.h

[Vincenzo Frascino]

On 04/09/2024 18:18, Christophe Leroy wrote:
> 
> 
> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>> The VDSO implementation includes headers from outside of the
>> vdso/ namespace.
>>
>> Split linux/array_size.h to make sure that the generic library
>> uses only the allowed namespace.
> 
> There is only one place using ARRAY_SIZE(x), can be open coded as
> sizeof(x)/sizeof(*x) instead.
> 

Agreed, as per previous comment on MIN()/MAX(). I will refactor my code accordingly.

> 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/linux/array_size.h |  8 +-------
>>   include/vdso/array_size.h  | 13 +++++++++++++
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>   create mode 100644 include/vdso/array_size.h
>>
>> diff --git a/include/linux/array_size.h b/include/linux/array_size.h
>> index 06d7d83196ca..ca9e63b419c4 100644
>> --- a/include/linux/array_size.h
>> +++ b/include/linux/array_size.h
>> @@ -2,12 +2,6 @@
>>   #ifndef _LINUX_ARRAY_SIZE_H
>>   #define _LINUX_ARRAY_SIZE_H
>>   -#include <linux/compiler.h>
>> -
>> -/**
>> - * ARRAY_SIZE - get the number of elements in array @arr
>> - * @arr: array to be sized
>> - */
>> -#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>> +#include <vdso/array_size.h>
>>     #endif  /* _LINUX_ARRAY_SIZE_H */
>> diff --git a/include/vdso/array_size.h b/include/vdso/array_size.h
>> new file mode 100644
>> index 000000000000..4079f7a5f86e
>> --- /dev/null
>> +++ b/include/vdso/array_size.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _VDSO_ARRAY_SIZE_H
>> +#define _VDSO_ARRAY_SIZE_H
>> +
>> +#include <linux/compiler.h>
>> +
>> +/**
>> + * ARRAY_SIZE - get the number of elements in array @arr
>> + * @arr: array to be sized
>> + */
>> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>> +
>> +#endif  /* _VDSO_ARRAY_SIZE_H */

-- 
Regards,
Vincenzo

Re: [PATCH 7/9] x86: vdso: Modify asm/vdso/getrandom.h to include datapage

[Vincenzo Frascino]

On 04/09/2024 18:19, Christophe Leroy wrote:
> 
> 
> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>> The VDSO implementation includes headers from outside of the
>> vdso/ namespace.
>>
>> Modify asm/vdso/getrandom.h to include datapage.
> 
> Does  asm/vdso/getrandom.h need datapage ? If not it is the ones that need it
> that should include it.
>

Why do you think it does not need it?

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

-- 
Regards,
Vincenzo

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Vincenzo Frascino]

On 04/09/2024 18:26, Christophe Leroy wrote:
> 
> 
> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
...

> 
> Now build fails on powerpc because struct vgetrandom_opaque_params is unknown.
> 
> x86 get it by chance via the following header inclusion chain:
> 
> In file included from ./include/linux/random.h:10,
>                  from ./include/linux/nodemask.h:98,
>                  from ./include/linux/mmzone.h:18,
>                  from ./include/linux/gfp.h:7,
>                  from ./include/linux/xarray.h:16,
>                  from ./include/linux/radix-tree.h:21,
>                  from ./include/linux/idr.h:15,
>                  from ./include/linux/kernfs.h:12,
>                  from ./include/linux/sysfs.h:16,
>                  from ./include/linux/kobject.h:20,
>                  from ./include/linux/of.h:18,
>                  from ./include/linux/clocksource.h:19,
>                  from ./include/clocksource/hyperv_timer.h:16,
>                  from ./arch/x86/include/asm/vdso/gettimeofday.h:21,
>                  from ./include/vdso/datapage.h:164,
>                  from arch/x86/entry/vdso/../../../../lib/vdso/getrandom.c:7,
>                  from arch/x86/entry/vdso/vgetrandom.c:7:
> 
> 
> 
> 

This tells me very little ;)

Can you please provide more details? e.g. What is the error you are getting? How
do I reproduce it?

I am happy to include the required change as part of this series.

Overall, the reason why I am doing this exercise it to sanitize the headers for
all the architectures so that in future we do not have issues. It is good we
find problems now.

-- 
Regards,
Vincenzo

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Christophe Leroy]

Le 06/09/2024 à 13:52, Vincenzo Frascino a écrit :
> 
> 
> On 04/09/2024 18:26, Christophe Leroy wrote:
>>
>>
>> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
> ...
> 
>>
>> Now build fails on powerpc because struct vgetrandom_opaque_params is unknown.
>>
>> x86 get it by chance via the following header inclusion chain:
>>
>> In file included from ./include/linux/random.h:10,
>>                   from ./include/linux/nodemask.h:98,
>>                   from ./include/linux/mmzone.h:18,
>>                   from ./include/linux/gfp.h:7,
>>                   from ./include/linux/xarray.h:16,
>>                   from ./include/linux/radix-tree.h:21,
>>                   from ./include/linux/idr.h:15,
>>                   from ./include/linux/kernfs.h:12,
>>                   from ./include/linux/sysfs.h:16,
>>                   from ./include/linux/kobject.h:20,
>>                   from ./include/linux/of.h:18,
>>                   from ./include/linux/clocksource.h:19,
>>                   from ./include/clocksource/hyperv_timer.h:16,
>>                   from ./arch/x86/include/asm/vdso/gettimeofday.h:21,
>>                   from ./include/vdso/datapage.h:164,
>>                   from arch/x86/entry/vdso/../../../../lib/vdso/getrandom.c:7,
>>                   from arch/x86/entry/vdso/vgetrandom.c:7:
>>
>>
>>
>>
> 
> This tells me very little ;)
> 
> Can you please provide more details? e.g. What is the error you are getting? How
> do I reproduce it?
> 
> I am happy to include the required change as part of this series.
> 
> Overall, the reason why I am doing this exercise it to sanitize the headers for
> all the architectures so that in future we do not have issues. It is good we
> find problems now.
> 

More details:

$ make ARCH=powerpc CROSS_COMPILE=ppc-linux- mpc885_ads_defconfig

$ LANG= make ARCH=powerpc CROSS_COMPILE=ppc-linux- vmlinux
   SYNC    include/config/auto.conf
   SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_32.h
   SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_64.h
   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_32.h
   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_64.h
   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_spu.h
   HOSTCC  scripts/dtc/dtc.o
   HOSTCC  scripts/dtc/flattree.o
   HOSTCC  scripts/dtc/fstree.o
   HOSTCC  scripts/dtc/data.o
   HOSTCC  scripts/dtc/livetree.o
   HOSTCC  scripts/dtc/treesource.o
   HOSTCC  scripts/dtc/srcpos.o
   HOSTCC  scripts/dtc/checks.o
   HOSTCC  scripts/dtc/util.o
   LEX     scripts/dtc/dtc-lexer.lex.c
   YACC    scripts/dtc/dtc-parser.tab.[ch]
   HOSTCC  scripts/dtc/dtc-lexer.lex.o
   HOSTCC  scripts/dtc/dtc-parser.tab.o
   HOSTLD  scripts/dtc/dtc
   HOSTCC  scripts/dtc/libfdt/fdt.o
   HOSTCC  scripts/dtc/libfdt/fdt_ro.o
   HOSTCC  scripts/dtc/libfdt/fdt_wip.o
   HOSTCC  scripts/dtc/libfdt/fdt_sw.o
   HOSTCC  scripts/dtc/libfdt/fdt_rw.o
   HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
   HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
   HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
   HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
   HOSTCC  scripts/dtc/fdtoverlay.o
   HOSTLD  scripts/dtc/fdtoverlay
   HOSTCC  scripts/kallsyms
   HOSTCC  scripts/sorttable
   CC      scripts/mod/empty.o
   HOSTCC  scripts/mod/mk_elfconfig
   MKELF   scripts/mod/elfconfig.h
   HOSTCC  scripts/mod/modpost.o
   CC      scripts/mod/devicetable-offsets.s
   HOSTCC  scripts/mod/file2alias.o
   HOSTCC  scripts/mod/sumversion.o
   HOSTCC  scripts/mod/symsearch.o
   HOSTLD  scripts/mod/modpost
   CC      kernel/bounds.s
   CC      arch/powerpc/kernel/asm-offsets.s
   CALL    scripts/checksyscalls.sh
   CHKSHA1 include/linux/atomic/atomic-arch-fallback.h
   CHKSHA1 include/linux/atomic/atomic-instrumented.h
   CHKSHA1 include/linux/atomic/atomic-long.h
   LDS     arch/powerpc/kernel/vdso/vdso32.lds
   VDSO32A arch/powerpc/kernel/vdso/sigtramp32-32.o
   VDSO32A arch/powerpc/kernel/vdso/gettimeofday-32.o
   VDSO32A arch/powerpc/kernel/vdso/datapage-32.o
   VDSO32A arch/powerpc/kernel/vdso/cacheflush-32.o
   VDSO32A arch/powerpc/kernel/vdso/note-32.o
   VDSO32A arch/powerpc/kernel/vdso/getcpu-32.o
   VDSO32A arch/powerpc/kernel/vdso/getrandom-32.o
   VDSO32A arch/powerpc/kernel/vdso/vgetrandom-chacha-32.o
   VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o
   VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from <command-line>:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c: In function 
'__cvdso_getrandom_data':
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:75:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    75 |                 params->size_of_opaque_state = sizeof(*state);
       |                       ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:76:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    76 |                 params->mmap_prot = VDSO_MMAP_PROT;
       |                       ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:77:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    77 |                 params->mmap_flags = VDSO_MMAP_FLAGS;
       |                       ^~
In file included from ./include/linux/array_size.h:5,
                  from ./include/linux/kernel.h:16,
                  from ./arch/powerpc/include/asm/page.h:11,
                  from ./arch/powerpc/include/asm/vdso/page.h:8,
                  from ./include/vdso/page.h:5,
                  from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:10:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/vdso/array_size.h:11:33: note: in definition of macro 'ARRAY_SIZE'
    11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
       |                                 ^~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/vdso/array_size.h:11:48: note: in definition of macro 'ARRAY_SIZE'
    11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
       |                                                ^~~
In file included from ./include/linux/container_of.h:5,
                  from ./include/linux/kernel.h:22:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/linux/build_bug.h:16:62: note: in definition of macro 
'BUILD_BUG_ON_ZERO'
    16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { 
int:(-!!(e)); })))
       |                                                              ^
./include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
   243 | #define __must_be_array(a) 
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
       |                                                   ^~~~~~~~~~~
./include/vdso/array_size.h:11:59: note: in expansion of macro 
'__must_be_array'
    11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
       | 
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:40: note: in 
expansion of macro 'ARRAY_SIZE'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/linux/build_bug.h:16:62: note: in definition of macro 
'BUILD_BUG_ON_ZERO'
    16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { 
int:(-!!(e)); })))
       |                                                              ^
./include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
   243 | #define __must_be_array(a) 
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
       |                                                   ^~~~~~~~~~~
./include/vdso/array_size.h:11:59: note: in expansion of macro 
'__must_be_array'
    11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
       | 
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:40: note: in 
expansion of macro 'ARRAY_SIZE'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
./include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width 
not an integer constant
    16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { 
int:(-!!(e)); })))
       |                                                   ^
./include/linux/compiler.h:243:33: note: in expansion of macro 
'BUILD_BUG_ON_ZERO'
   243 | #define __must_be_array(a) 
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
       |                                 ^~~~~~~~~~~~~~~~~
./include/vdso/array_size.h:11:59: note: in expansion of macro 
'__must_be_array'
    11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
       | 
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:78:40: note: in 
expansion of macro 'ARRAY_SIZE'
    78 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:79:31: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    79 |                         params->reserved[i] = 0;
       |                               ^~
In file included from ./include/vdso/datapage.h:7,
                  from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:6:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:32: error: 
'GRND_NONBLOCK' undeclared (first use in this function); did you mean 
'MAP_NONBLOCK'?
    88 |         if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM | 
GRND_INSECURE)))
       |                                ^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
    77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
       |                                             ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:32: note: each 
undeclared identifier is reported only once for each function it appears in
    88 |         if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM | 
GRND_INSECURE)))
       |                                ^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
    77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
       |                                             ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:48: error: 
'GRND_RANDOM' undeclared (first use in this function)
    88 |         if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM | 
GRND_INSECURE)))
       |                                                ^~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
    77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
       |                                             ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:62: error: 
'GRND_INSECURE' undeclared (first use in this function)
    88 |         if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM | 
GRND_INSECURE)))
       | 
^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
    77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
       |                                             ^
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:87: 
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:227: __sub-make] Error 2



FWIW, here are the changes I added on top of your series:

diff --git a/arch/powerpc/include/asm/vdso/getrandom.h 
b/arch/powerpc/include/asm/vdso/getrandom.h
index 501d6bb14e8a..4af9efdbe296 100644
--- a/arch/powerpc/include/asm/vdso/getrandom.h
+++ b/arch/powerpc/include/asm/vdso/getrandom.h
@@ -5,6 +5,8 @@
  #ifndef _ASM_POWERPC_VDSO_GETRANDOM_H
  #define _ASM_POWERPC_VDSO_GETRANDOM_H

+#include <vdso/datapage.h>
+
  #ifndef __ASSEMBLY__

  static __always_inline int do_syscall_3(const unsigned long _r0, const 
unsigned long _r3,
diff --git a/arch/powerpc/include/asm/vdso/mman.h 
b/arch/powerpc/include/asm/vdso/mman.h
new file mode 100644
index 000000000000..4c936c9d11ab
--- /dev/null
+++ b/arch/powerpc/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 */
diff --git a/arch/powerpc/include/asm/vdso/page.h 
b/arch/powerpc/include/asm/vdso/page.h
new file mode 100644
index 000000000000..c41fd44aca5b
--- /dev/null
+++ b/arch/powerpc/include/asm/vdso/page.h
@@ -0,0 +1,15 @@
+
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_VDSO_PAGE_H
+#define __ASM_VDSO_PAGE_H
+
+#ifndef __ASSEMBLY__
+
+#include <asm/page.h>
+
+#define VDSO_PAGE_MASK	PAGE_MASK
+#define VDSO_PAGE_SIZE	PAGE_SIZE
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_PAGE_H */


Christophe

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Vincenzo Frascino]

On 06/09/2024 13:04, Christophe Leroy wrote:
> 
> 
> Le 06/09/2024 à 13:52, Vincenzo Frascino a écrit :
>>
>>
>> On 04/09/2024 18:26, Christophe Leroy wrote:
>>>
>>>
>>> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>> ...
>>

...

> 
> More details:
> 
> $ make ARCH=powerpc CROSS_COMPILE=ppc-linux- mpc885_ads_defconfig
> 
> $ LANG= make ARCH=powerpc CROSS_COMPILE=ppc-linux- vmlinux
>   SYNC    include/config/auto.conf
>   SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_32.h
>   SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_64.h
>   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_32.h
>   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_64.h
>   SYSTBL  arch/powerpc/include/generated/asm/syscall_table_spu.h

Thank you Christophe, is upstream code sufficient to reproduce the issue? Or do
I need specific patches?

> 
> 
> Christophe
> 

-- 
Regards,
Vincenzo

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Christophe Leroy]

Le 06/09/2024 à 14:40, Vincenzo Frascino a écrit :
> 
> 
> On 06/09/2024 13:04, Christophe Leroy wrote:
>>
>>
>> Le 06/09/2024 à 13:52, Vincenzo Frascino a écrit :
>>>
>>>
>>> On 04/09/2024 18:26, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>>> ...
>>>
> 
> ...
> 
>>
>> More details:
>>
>> $ make ARCH=powerpc CROSS_COMPILE=ppc-linux- mpc885_ads_defconfig
>>
>> $ LANG= make ARCH=powerpc CROSS_COMPILE=ppc-linux- vmlinux
>>    SYNC    include/config/auto.conf
>>    SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_32.h
>>    SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_64.h
>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_32.h
>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_64.h
>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_spu.h
> 
> Thank you Christophe, is upstream code sufficient to reproduce the issue? Or do
> I need specific patches?
> 

You need random git tree plus the patch "[PATCH] powerpc patches for 
Vincenzo's series" I have just sent to you.

Christophe

Re: [PATCH 9/9] vdso: Modify getrandom to include the correct namespace

[Vincenzo Frascino]

On 06/09/2024 13:49, Christophe Leroy wrote:
> 
> 
> Le 06/09/2024 à 14:40, Vincenzo Frascino a écrit :
>>
>>
>> On 06/09/2024 13:04, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/09/2024 à 13:52, Vincenzo Frascino a écrit :
>>>>
>>>>
>>>> On 04/09/2024 18:26, Christophe Leroy wrote:
>>>>>
>>>>>
>>>>> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
>>>> ...
>>>>
>>
>> ...
>>
>>>
>>> More details:
>>>
>>> $ make ARCH=powerpc CROSS_COMPILE=ppc-linux- mpc885_ads_defconfig
>>>
>>> $ LANG= make ARCH=powerpc CROSS_COMPILE=ppc-linux- vmlinux
>>>    SYNC    include/config/auto.conf
>>>    SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_32.h
>>>    SYSHDR  arch/powerpc/include/generated/uapi/asm/unistd_64.h
>>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_32.h
>>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_64.h
>>>    SYSTBL  arch/powerpc/include/generated/asm/syscall_table_spu.h
>>
>> Thank you Christophe, is upstream code sufficient to reproduce the issue? Or do
>> I need specific patches?
>>
> 
> You need random git tree plus the patch "[PATCH] powerpc patches for Vincenzo's
> series" I have just sent to you.
>

Thank you!

> Christophe

-- 
Regards,
Vincenzo

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Christophe Leroy]

Hi Arnd,

Le 06/09/2024 à 21:19, Arnd Bergmann a écrit :
> On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote:
>> On 04/09/2024 15:52, Arnd Bergmann wrote:
>>> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:
>> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they
>> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.:
>>
>> x86:
>> #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)
>>
>> powerpc:
>> #define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)
>>
>> hence I left to the architecture the responsibility of redefining the constants
>> for the VSDO.
> 
> ASM_CONST() is a powerpc-specific macro that is defined the
> same way as _AC(). We could probably just replace all ASM_CONST()
> as a cleanup, but for this purpose, just remove the custom PAGE_SIZE
> and PAGE_SHIFT macros. This can be a single patch fro all
> architectures.
> 

I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1.

The two functions below don't provide the same result:

#define PAGE_SIZE (1 << 12)
#define PAGE_MASK (~(PAGE_SIZE - 1))

unsigned long long f(unsigned long long x)
{
	return x & PAGE_MASK;
}

#define PAGE_SIZE_2 (1UL << 12)
#define PAGE_MASK_2 (~(PAGE_SIZE_2 - 1))

unsigned long long g(unsigned long long x)
{
	return x & PAGE_MASK_2;
}

00000000 <f>:
    0:	54 84 00 26 	clrrwi  r4,r4,12
    4:	4e 80 00 20 	blr

00000008 <g>:
    8:	54 84 00 26 	clrrwi  r4,r4,12
    c:	38 60 00 00 	li      r3,0
   10:	4e 80 00 20 	blr


This can be a problem on 32 bits platforms with 64 bits phys_addr_t

Christophe

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Arnd Bergmann]

On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote:
> On 04/09/2024 15:52, Arnd Bergmann wrote:
>> On Tue, Sep 3, 2024, at 15:14, Vincenzo Frascino wrote:
> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they
> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.:
>
> x86:
> #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)
>
> powerpc:
> #define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)
>
> hence I left to the architecture the responsibility of redefining the constants
> for the VSDO.

ASM_CONST() is a powerpc-specific macro that is defined the
same way as _AC(). We could probably just replace all ASM_CONST()
as a cleanup, but for this purpose, just remove the custom PAGE_SIZE
and PAGE_SHIFT macros. This can be a single patch fro all
architectures.

    Arnd

RE: [PATCH 5/9] vdso: Split linux/minmax.h

[David Laight]

From: Vincenzo Frascino
> Sent: 06 September 2024 12:41
> 
> On 04/09/2024 18:23, Arnd Bergmann wrote:
> > On Wed, Sep 4, 2024, at 17:17, Christophe Leroy wrote:
> >> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
> >>> The VDSO implementation includes headers from outside of the
> >>> vdso/ namespace.
> >>>
> >>> Split linux/minmax.h to make sure that the generic library
> >>> uses only the allowed namespace.
> >>
> >> It is probably easier to just don't use min_t() in VDSO. Can be open
> >> coded without impeeding readability.
> >
> > Right, or possibly the even simpler MIN()/MAX() if the arguments
> > have no side-effects.
> >
> 
> Agreed, generally I do not like open-coding since it tends to introduce
> duplication, but these cases are simple especially if we can use MIN()/MAX().

Aren't MIN()/MAX() likely to get defined in minmax.h for cases where the
arguments are constants - and maybe have checks that they are constants.
So you don't want to define them in the VDSO header either.

Open coding simple cases is actually easier to read :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Arnd Bergmann]

On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote:
> Le 06/09/2024 à 21:19, Arnd Bergmann a écrit :
>> On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote:

>>> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they
>>> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.:
>>>
>>> x86:
>>> #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)
>>>
>>> powerpc:
>>> #define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)
>>>
>>> hence I left to the architecture the responsibility of redefining the constants
>>> for the VSDO.
>> 
>> ASM_CONST() is a powerpc-specific macro that is defined the
>> same way as _AC(). We could probably just replace all ASM_CONST()
>> as a cleanup, but for this purpose, just remove the custom PAGE_SIZE
>> and PAGE_SHIFT macros. This can be a single patch fro all
>> architectures.
>> 
>
> I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1.
>
>
> This can be a problem on 32 bits platforms with 64 bits phys_addr_t

But that would already be a bug if anything used this, however
none of them do. The only instance of an open-coded

#define PAGE_SIZE       (1 << PAGE_SHIFT)

is from openrisc, but that is only used inside of __ASSEMBLY__, for
the same effect as _AC().

    Arnd

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Christophe Leroy]

Le 08/09/2024 à 22:48, Arnd Bergmann a écrit :
> On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote:
>> Le 06/09/2024 à 21:19, Arnd Bergmann a écrit :
>>> On Fri, Sep 6, 2024, at 11:20, Vincenzo Frascino wrote:
> 
>>>> Looking at the definition of PAGE_SIZE and PAGE_MASK for each architecture they
>>>> all depend on CONFIG_PAGE_SHIFT but they are slightly different, e.g.:
>>>>
>>>> x86:
>>>> #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)
>>>>
>>>> powerpc:
>>>> #define PAGE_SIZE        (ASM_CONST(1) << PAGE_SHIFT)
>>>>
>>>> hence I left to the architecture the responsibility of redefining the constants
>>>> for the VSDO.
>>>
>>> ASM_CONST() is a powerpc-specific macro that is defined the
>>> same way as _AC(). We could probably just replace all ASM_CONST()
>>> as a cleanup, but for this purpose, just remove the custom PAGE_SIZE
>>> and PAGE_SHIFT macros. This can be a single patch fro all
>>> architectures.
>>>
>>
>> I'm not worried about _AC versus ASM_CONST, but I am by the 1UL versus 1.
>>
>>
>> This can be a problem on 32 bits platforms with 64 bits phys_addr_t
> 
> But that would already be a bug if anything used this, however
> none of them do. The only instance of an open-coded
> 
> #define PAGE_SIZE       (1 << PAGE_SHIFT)
> 
> is from openrisc, but that is only used inside of __ASSEMBLY__, for
> the same effect as _AC().

Maybe I was not clear enough. The problem is not with PAGE_SHIFT but 
with PAGE_MASK, and that's what I show with my exemple.

If take the definition from ARM64 (which is the same as several other 
artchitectures):

#define PAGE_SIZE		(_AC(1, UL) << PAGE_SHIFT)
#define PAGE_MASK		(~(PAGE_SIZE-1))

PAGE_SHIFT is 12
PAGE_SIZE is then 4096 UL
PAGE_MASK is then 0xfffff000 UL

So if I take the probe() in drivers/uio/uio_pci_generic.c, it has:

	uiomem->addr = r->start & PAGE_MASK;

uiomem->addr is a phys_addr_t
r->start is a ressource_size_t hence a phys_addr_t

And phys_addr_t is defined as:

	#ifdef CONFIG_PHYS_ADDR_T_64BIT
	typedef u64 phys_addr_t;
	#else
	typedef u32 phys_addr_t;
	#endif

On a 32 bits platform, UL is unsigned 32 bits, so the r->start & 
PAGE_MASK will and r->start with 0x00000000fffff000

That is wrong.


That's the reason why powerpc does not define PAGE_MASK like ARM64 but 
defines PAGE_MASK as:

	(~((1 << PAGE_SHIFT) - 1))

When using 1 instead of 1UL, PAGE_MASK is still 0xfffff000 but it is a 
signed constant, so when it is anded with an u64, it gets 
signed-extended to 0xfffffffffffff000 which gives the expected result.

That's what I wanted to illustrate with the exemple in my previous 
message. The function f() was using the signed PAGE_MASK while function 
g() was using the unsigned PAGE_MASK:

00000000 <f>:
    0:    54 84 00 26     clrrwi  r4,r4,12
    4:    4e 80 00 20     blr

00000008 <g>:
    8:    54 84 00 26     clrrwi  r4,r4,12
    c:    38 60 00 00     li      r3,0
   10:    4e 80 00 20     blr

Function f() returns the given u64 value with the 12 lowest bits cleared
Function g() returns the given u64 value with the 12 lowest bits cleared 
and the 32 highest bits cleared as well, which is unexpected.

Christophe

Re: [PATCH 3/9] x86: vdso: Introduce asm/vdso/page.h

[Arnd Bergmann]

On Tue, Sep 10, 2024, at 12:28, Christophe Leroy wrote:
> Le 08/09/2024 à 22:48, Arnd Bergmann a écrit :
>> On Fri, Sep 6, 2024, at 18:40, Christophe Leroy wrote:
>
> uiomem->addr is a phys_addr_t
> r->start is a ressource_size_t hence a phys_addr_t
>
> And phys_addr_t is defined as:
>
> 	#ifdef CONFIG_PHYS_ADDR_T_64BIT
> 	typedef u64 phys_addr_t;
> 	#else
> 	typedef u32 phys_addr_t;
> 	#endif
>
> On a 32 bits platform, UL is unsigned 32 bits, so the r->start & 
> PAGE_MASK will and r->start with 0x00000000fffff000
>
> That is wrong.

Right, I see. So out of the five 32-bit architectures with a
64-bit phys_addr_t, arc seems to ignore this problem, x86 has
a separate PHYSICAL_PAGE_MASK for its internal uses and
the other three have the definition you mention as

> 	(~((1 << PAGE_SHIFT) - 1))

And this is only documented properly on powerpc.

How about making the common definition this?

#ifdef CONFIG_PHYS_ADDR_T_64BIT
#define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
#else
#define PAGE_MASK (~(PAGE_SIZE-1))
#endif

That keeps it unchanged for everything other than arc
and x86-32, and hopefully fixes the currently behavior
on those two.

     Arnd