[PATCH 0/4] Fixups for random vDSO

This small series is an extract of fixups for generic part of random vDSO in
preparation of implementing vDSO getrandom for powerpc.

See last version of full series at:
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1724309198.git.christophe.leroy@csgroup.eu/

This series is based on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git master

Christophe Leroy (4):
  asm-generic/unaligned.h: Extract common header for vDSO
  random: vDSO: Don't use PAGE_SIZE and PAGE_MASK
  random: vDSO: Clean header inclusion in getrandom
  random: vDSO: don't use 64 bits atomics on 32 bits architectures

 arch/x86/include/asm/pvclock.h  |  1 +
 drivers/char/random.c           |  9 ++++++++-
 include/asm-generic/unaligned.h | 11 +----------
 include/vdso/helpers.h          |  1 +
 include/vdso/unaligned.h        | 15 +++++++++++++++
 lib/vdso/getrandom.c            | 16 ++++++++--------
 6 files changed, 34 insertions(+), 19 deletions(-)
 create mode 100644 include/vdso/unaligned.h

-- 
2.44.0

getrandom vDSO implementation requires __put_unaligned_t() and
__put_unaligned_t() but including asm-generic/unaligned.h pulls
too many other headers.

Follow the same approach as for most things in include/vdso/,
see for instance commit 8165b57bca21 ("linux/const.h: Extract
common header for vDSO"): Move __get_unaligned_t and __put_unaligned_t
into a new unaligned.h living in the vdso/ include directory.

Signed-off-by: Christophe Leroy <redacted>
---
 include/asm-generic/unaligned.h | 11 +----------
 include/vdso/unaligned.h        | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 10 deletions(-)
 create mode 100644 include/vdso/unaligned.h

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index a84c64e5f11e..95acdd70b3b2 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h

@@ -8,16 +8,7 @@
  */
 #include <linux/unaligned/packed_struct.h>
 #include <asm/byteorder.h>
-
-#define __get_unaligned_t(type, ptr) ({						\
-	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
-	__pptr->x;								\
-})
-
-#define __put_unaligned_t(type, val, ptr) do {					\
-	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
-	__pptr->x = (val);							\
-} while (0)
+#include <vdso/unaligned.h>
 
 #define get_unaligned(ptr)	__get_unaligned_t(typeof(*(ptr)), (ptr))
 #define put_unaligned(val, ptr) __put_unaligned_t(typeof(*(ptr)), (val), (ptr))

diff --git a/include/vdso/unaligned.h b/include/vdso/unaligned.h
new file mode 100644
index 000000000000..eee3d2a4dbe4
--- /dev/null
+++ b/include/vdso/unaligned.h

@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_UNALIGNED_H
+#define __VDSO_UNALIGNED_H
+
+#define __get_unaligned_t(type, ptr) ({						\
+	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
+	__pptr->x;								\
+})
+
+#define __put_unaligned_t(type, val, ptr) do {					\
+	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
+	__pptr->x = (val);							\
+} while (0)
+
+#endif /* __VDSO_UNALIGNED_H */

-- 
2.44.0

Using PAGE_SIZE and PAGE_MASK in VDSO requires inclusion of
page.h and it creates several problems, see
commit 8b3843ae3634 ("vdso/datapage: Quick fix - use asm/page-def.h
for ARM64") and commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in
vdso/datapage.h").

An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO
when they do not exist already, but this can be misleading.

So follow the same approach as commit cffaefd15a8f ("vdso: Use
CONFIG_PAGE_SHIFT in vdso/datapage.h") and exclusively use
CONFIG_PAGE_SHIFT. To avoid too much ugliness, define local consts
that constains the calculated page size and page mask.

Signed-off-by: Christophe Leroy <redacted>
---
v3: Use local consts instead of _PAGE_SIZE and _PAGE_MASK macros that are already defined by some architectures.
---
 lib/vdso/getrandom.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index f1643656d0b0..5874e3072bfe 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c

@@ -65,7 +65,9 @@ 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);
+	const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT;
+	const unsigned long page_mask = ~(page_size - 1);
+	ssize_t ret = min_t(size_t, INT_MAX & 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;

@@ -84,7 +86,7 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 	}
 
 	/* 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 & ~page_mask) + sizeof(*state) > page_size))
 		return -EFAULT;
 
 	/* Handle unexpected flags by falling back to the kernel. */

-- 
2.44.0

On Tue, Aug 27, 2024 at 09:31:48AM +0200, Christophe Leroy wrote:

-	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
+	const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT;
+	const unsigned long page_mask = ~(page_size - 1);
+	ssize_t ret = min_t(size_t, INT_MAX & page_mask /* = MAX_RW_COUNT */, len);

I'm really not a fan of making the code less idiomatic...

An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO
when they do not exist already, but this can be misleading.

Why would what tglx and I suggested be misleading? That seems pretty
normal... Are you worried they might mismatch somehow? (If so, why?) 

Jason

Le 27/08/2024 à 09:49, Jason A. Donenfeld a écrit :

On Tue, Aug 27, 2024 at 09:31:48AM +0200, Christophe Leroy wrote:

quoted

-	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
+	const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT;
+	const unsigned long page_mask = ~(page_size - 1);
+	ssize_t ret = min_t(size_t, INT_MAX & page_mask /* = MAX_RW_COUNT */, len);

I'm really not a fan of making the code less idiomatic...

Ok, I have another idea, let's give it a try.

quoted

An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO
when they do not exist already, but this can be misleading.

Why would what tglx and I suggested be misleading? That seems pretty
normal... Are you worried they might mismatch somehow? (If so, why?)

All architectures have their own definition, they are all based on 
CONFIG_PAGE_SHIFT and should give the same value but with some 
subtleties. The best would be to have an asm-generic definition of 
PAGE_SIZE and PAGE_MASK that all architectures use, but that's another 
level of work.

tglx or yourself suggested to put in a one of the vdso headers instead 
of directly in getrandom.c. This is too fragile because PAGE_SIZE might 
be absent in that header but arrive in getrandom.c through another header.

Christophe

On Tue, Aug 27, 2024 at 10:16:18AM +0200, Christophe Leroy wrote:

tglx or yourself suggested to put in a one of the vdso headers instead 
of directly in getrandom.c. This is too fragile because PAGE_SIZE might 
be absent in that header but arrive in getrandom.c through another header.

Oh jeeze, yea.

FYI, _PAGE_SIZE is defined on s390, so that might not be such a good
idea (from the previous version).

The best would be to have an asm-generic definition of
PAGE_SIZE and PAGE_MASK that all architectures use, but that's another
level of work.

Yea that seems far too large of an operation to do here.

quoted

I'm really not a fan of making the code less idiomatic...

Ok, I have another idea, let's give it a try.

What's the other idea?

Using PAGE_SIZE and PAGE_MASK in VDSO requires inclusion of
page.h and it creates several problems, see
commit 8b3843ae3634 ("vdso/datapage: Quick fix - use asm/page-def.h
for ARM64") and commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in
vdso/datapage.h").

Redefine PAGE_SIZE and PAGE_MASK based on CONFIG_PAGE_SHIFT.

Signed-off-by: Christophe Leroy <redacted>
---
v3: Use local consts instead of _PAGE_SIZE and _PAGE_MASK macros that are already defined by some architectures.

v4: undefine and redefine PAGE_SIZE and PAGE_MASK
---
 lib/vdso/getrandom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index f1643656d0b0..e5968ed141cb 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c

@@ -14,6 +14,11 @@
 #include <asm/unaligned.h>
 #include <uapi/linux/mman.h>
 
+#undef PAGE_SIZE
+#undef PAGE_MASK
+#define PAGE_SIZE	(1UL << CONFIG_PAGE_SHIFT)
+#define PAGE_MASK	(~(PAGE_SIZE - 1))
+
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
 		__put_unaligned_t(type, __get_unaligned_t(type, src), dst);	\

-- 
2.44.0

I don't love this, but it might be the lesser of evils, so sure, let's
do it.

I think I'll combine these header fixups so that the whole operation is
a bit more clear. The commit is still pretty small. Something like
below:

From 0d9a3d68cd6222395a605abd0ac625c41d4cabfa Mon Sep 17 00:00:00 2001
From: Christophe Leroy <redacted>
Date: Tue, 27 Aug 2024 09:31:47 +0200
Subject: [PATCH] random: vDSO: clean header inclusion in getrandom

Depending on the architecture, building a 32-bit vDSO on a 64-bit kernel
is problematic when some system headers are included.

Minimise the amount of headers by moving needed items, such as
__{get,put}_unaligned_t, into dedicated common headers and in general
use more specific headers, similar to what was done in commit
8165b57bca21 ("linux/const.h: Extract common header for vDSO") and
commit 8c59ab839f52 ("lib/vdso: Enable common headers").

On some architectures this results in missing PAGE_SIZE, as was
described by commit 8b3843ae3634 ("vdso/datapage: Quick fix - use
asm/page-def.h for ARM64"), so define this if necessary, in the same way
as done prior by commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in
vdso/datapage.h").

Removing linux/time64.h leads to missing 'struct timespec64' in
x86's asm/pvclock.h. Add a forward declaration of that struct in
that file.

Signed-off-by: Christophe Leroy <redacted>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/x86/include/asm/pvclock.h  |  1 +
 include/asm-generic/unaligned.h | 11 +----------
 include/vdso/helpers.h          |  1 +
 include/vdso/unaligned.h        | 15 +++++++++++++++
 lib/vdso/getrandom.c            | 13 ++++++++-----
 5 files changed, 26 insertions(+), 15 deletions(-)
 create mode 100644 include/vdso/unaligned.h

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 0c92db84469d..6e4f8fae3ce9 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h

@@ -5,6 +5,7 @@
 #include <asm/clocksource.h>
 #include <asm/pvclock-abi.h>

+struct timespec64;
 /* some helper functions for xen and kvm pv clock sources */
 u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
 u64 pvclock_clocksource_read_nowd(struct pvclock_vcpu_time_info *src);

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index a84c64e5f11e..95acdd70b3b2 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h

@@ -8,16 +8,7 @@
  */
 #include <linux/unaligned/packed_struct.h>
 #include <asm/byteorder.h>
-
-#define __get_unaligned_t(type, ptr) ({						\
-	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
-	__pptr->x;								\
-})
-
-#define __put_unaligned_t(type, val, ptr) do {					\
-	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
-	__pptr->x = (val);							\
-} while (0)
+#include <vdso/unaligned.h>

 #define get_unaligned(ptr)	__get_unaligned_t(typeof(*(ptr)), (ptr))
 #define put_unaligned(val, ptr) __put_unaligned_t(typeof(*(ptr)), (val), (ptr))

diff --git a/include/vdso/helpers.h b/include/vdso/helpers.h
index 73501149439d..3ddb03bb05cb 100644
--- a/include/vdso/helpers.h
+++ b/include/vdso/helpers.h

@@ -4,6 +4,7 @@

 #ifndef __ASSEMBLY__

+#include <asm/barrier.h>
 #include <vdso/datapage.h>

 static __always_inline u32 vdso_read_begin(const struct vdso_data *vd)

diff --git a/include/vdso/unaligned.h b/include/vdso/unaligned.h
new file mode 100644
index 000000000000..eee3d2a4dbe4
--- /dev/null
+++ b/include/vdso/unaligned.h

@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_UNALIGNED_H
+#define __VDSO_UNALIGNED_H
+
+#define __get_unaligned_t(type, ptr) ({						\
+	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
+	__pptr->x;								\
+})
+
+#define __put_unaligned_t(type, val, ptr) do {					\
+	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
+	__pptr->x = (val);							\
+} while (0)
+
+#endif /* __VDSO_UNALIGNED_H */

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 1281fa3546c2..938ca539aaa6 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c

@@ -4,15 +4,18 @@
  */

 #include <linux/array_size.h>
-#include <linux/cache.h>
-#include <linux/kernel.h>
-#include <linux/time64.h>
+#include <linux/minmax.h>
 #include <vdso/datapage.h>
 #include <vdso/getrandom.h>
+#include <vdso/unaligned.h>
 #include <asm/vdso/getrandom.h>
-#include <asm/vdso/vsyscall.h>
-#include <asm/unaligned.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))

 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
--

2.46.0

Le 27/08/2024 à 10:40, Jason A. Donenfeld a écrit :

I don't love this, but it might be the lesser of evils, so sure, let's
do it.

I don't love it either but I still prefer it to:

#ifndef PAGE_SIZE
#define PAGE_SIZE
#define PAGE_MASK
#endif

At least we are sure that every architecture get the same , independant 
of the selected options, and we know what we get.

For instance, on x86, when you select CONFIG_HYPERV_TIMER you get 
asm/hyperv_timer.h which indirectly pulls page.h. But when 
CONFIG_HYPERV_TIMER is not selected you don't get asm/hyperv_timer.h

I think I'll combine these header fixups so that the whole operation is
a bit more clear. The commit is still pretty small. Something like
below:

Looks good to me.

Thanks
Christophe

On Tue, Aug 27, 2024, at 10:40, Jason A. Donenfeld wrote:

I don't love this, but it might be the lesser of evils, so sure, let's
do it.

I think I'll combine these header fixups so that the whole operation is
a bit more clear. The commit is still pretty small. Something like
below:

From 0d9a3d68cd6222395a605abd0ac625c41d4cabfa Mon Sep 17 00:00:00 2001
From: Christophe Leroy <redacted>
Date: Tue, 27 Aug 2024 09:31:47 +0200
Subject: [PATCH] random: vDSO: clean header inclusion in getrandom

Depending on the architecture, building a 32-bit vDSO on a 64-bit kernel
is problematic when some system headers are included.

Minimise the amount of headers by moving needed items, such as
__{get,put}_unaligned_t, into dedicated common headers and in general
use more specific headers, similar to what was done in commit
8165b57bca21 ("linux/const.h: Extract common header for vDSO") and
commit 8c59ab839f52 ("lib/vdso: Enable common headers").

On some architectures this results in missing PAGE_SIZE, as was
described by commit 8b3843ae3634 ("vdso/datapage: Quick fix - use
asm/page-def.h for ARM64"), so define this if necessary, in the same way
as done prior by commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in
vdso/datapage.h").

Removing linux/time64.h leads to missing 'struct timespec64' in
x86's asm/pvclock.h. Add a forward declaration of that struct in
that file.

Signed-off-by: Christophe Leroy <redacted>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

This is clearly better, but there are still a couple of inaccuracies
that may end up biting us again later. Not sure whether it's worth
trying to fix it all at once or if we want to address them when that
happens:

 #include <linux/array_size.h>
-#include <linux/cache.h>
-#include <linux/kernel.h>
-#include <linux/time64.h>
+#include <linux/minmax.h>

These are still two headers outside of the vdso/ namespace. For arm64
we had concluded that this is never safe, and any vdso header should
only include other vdso headers so we never pull in anything that
e.g. depends on memory management headers that are in turn broken
for the compat vdso.

The array_size.h header is really small, so that one could
probably just be moved into the vdso/ namespace. The minmax.h
header is already rather complex, so it may be better to just
open-code the usage of MIN/MAX where needed?

 #include <vdso/datapage.h>
 #include <vdso/getrandom.h>
+#include <vdso/unaligned.h>
 #include <asm/vdso/getrandom.h>
-#include <asm/vdso/vsyscall.h>
-#include <asm/unaligned.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))

Since these are now the same across all architectures, maybe we
can just have the PAGE_SIZE definitions a vdso header instead
and include that from asm/page.h.

Including uapi/linux/mman.h may still be problematic on
some architectures if they change it in a way that is
incompatible with compat vdso, but at least that can't
accidentally rely on CONFIG_64BIT or something else that
would be wrong there.

     Arnd

Le 27/08/2024 à 11:59, Arnd Bergmann a écrit :

On Tue, Aug 27, 2024, at 10:40, Jason A. Donenfeld wrote:

quoted

I don't love this, but it might be the lesser of evils, so sure, let's
do it.

I think I'll combine these header fixups so that the whole operation is
a bit more clear. The commit is still pretty small. Something like
below:

 From 0d9a3d68cd6222395a605abd0ac625c41d4cabfa Mon Sep 17 00:00:00 2001
From: Christophe Leroy <redacted>
Date: Tue, 27 Aug 2024 09:31:47 +0200
Subject: [PATCH] random: vDSO: clean header inclusion in getrandom

Depending on the architecture, building a 32-bit vDSO on a 64-bit kernel
is problematic when some system headers are included.

Minimise the amount of headers by moving needed items, such as
__{get,put}_unaligned_t, into dedicated common headers and in general
use more specific headers, similar to what was done in commit
8165b57bca21 ("linux/const.h: Extract common header for vDSO") and
commit 8c59ab839f52 ("lib/vdso: Enable common headers").

On some architectures this results in missing PAGE_SIZE, as was
described by commit 8b3843ae3634 ("vdso/datapage: Quick fix - use
asm/page-def.h for ARM64"), so define this if necessary, in the same way
as done prior by commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in
vdso/datapage.h").

Removing linux/time64.h leads to missing 'struct timespec64' in
x86's asm/pvclock.h. Add a forward declaration of that struct in
that file.

Signed-off-by: Christophe Leroy <redacted>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

This is clearly better, but there are still a couple of inaccuracies
that may end up biting us again later. Not sure whether it's worth
trying to fix it all at once or if we want to address them when that
happens:

quoted

  #include <linux/array_size.h>
-#include <linux/cache.h>
-#include <linux/kernel.h>
-#include <linux/time64.h>
+#include <linux/minmax.h>

These are still two headers outside of the vdso/ namespace. For arm64
we had concluded that this is never safe, and any vdso header should
only include other vdso headers so we never pull in anything that
e.g. depends on memory management headers that are in turn broken
for the compat vdso.

The array_size.h header is really small, so that one could
probably just be moved into the vdso/ namespace. The minmax.h
header is already rather complex, so it may be better to just
open-code the usage of MIN/MAX where needed?

It is used at two places only so yes can to that.

Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also 
have it only once

quoted

  #include <vdso/datapage.h>
  #include <vdso/getrandom.h>
+#include <vdso/unaligned.h>
  #include <asm/vdso/getrandom.h>
-#include <asm/vdso/vsyscall.h>
-#include <asm/unaligned.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))

Since these are now the same across all architectures, maybe we
can just have the PAGE_SIZE definitions a vdso header instead
and include that from asm/page.h.

I gave it a quick look yesterday, there are still some subtleties 
between architectures.

For instance, most architectures use 1UL for the shift but powerpc use 1 
and has the following comment:

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

So we'll have to look at all this carefully when we want something 
common, or am I missing something ?

Including uapi/linux/mman.h may still be problematic on
some architectures if they change it in a way that is
incompatible with compat vdso, but at least that can't
accidentally rely on CONFIG_64BIT or something else that
would be wrong there.

Yes that one is tricky. Because uapi/linux/mman.h includes asm/mman.h 
with the intention to include uapi/asm/mman.h but when built from the 
kernel in reality you get arch/powerpc/include/asm/mman.h and I had to 
add some ifdefery to kick-out kernel oddities it contains that pull 
additional kernel headers.

diff --git a/arch/powerpc/include/asm/mman.h 

b/arch/powerpc/include/asm/mman.h
index 17a77d47ed6d..42a51a993d94 100644

--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h

@@ -6,7 +6,7 @@

  #include <uapi/asm/mman.h>

-#ifdef CONFIG_PPC64
+#if defined(CONFIG_PPC64) && !defined(BUILD_VDSO)

  #include <asm/cputable.h>
  #include <linux/mm.h>

Christophe

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...

quoted

These are still two headers outside of the vdso/ namespace. For arm64
we had concluded that this is never safe, and any vdso header should
only include other vdso headers so we never pull in anything that
e.g. depends on memory management headers that are in turn broken
for the compat vdso.

The array_size.h header is really small, so that one could
probably just be moved into the vdso/ namespace. The minmax.h
header is already rather complex, so it may be better to just
open-code the usage of MIN/MAX where needed?

It is used at two places only so yes can to that.

Could you please clarify where minmax is needed? I tried to build Jason's master
tree for x86, commenting the header and it seems building fine. I might be
missing something.

Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
only once

I have a similar issue to figure out why linux/array_size.h and
uapi/linux/random.h are needed. It seems that I can build the object without
them. Could you please explain?

In general, the philosophy adopted to split the headers is to extract the set of
functions used by vDSOs from the linux headers and place them in the vdso headers.
Consequently the linux header includes to vdso one. E.g.: linux/time64.h
includes vdso/time64.h.

IMHO we should follow the same approach, if at all for consistency, unless there
is a valid reason.

...

I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.

I am attaching a patch to provide my view on how to minimize the headers
included and use only the vdso/ namespace. Please, before using the code,
consider that I conducted very limited testing.

Note: It should apply clean on Jason's tree.

Let me know your thoughts.

Christophe

-- 
Regards,
Vincenzo

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...

quoted

quoted

These are still two headers outside of the vdso/ namespace. For arm64
we had concluded that this is never safe, and any vdso header should
only include other vdso headers so we never pull in anything that
e.g. depends on memory management headers that are in turn broken
for the compat vdso.

The array_size.h header is really small, so that one could
probably just be moved into the vdso/ namespace. The minmax.h
header is already rather complex, so it may be better to just
open-code the usage of MIN/MAX where needed?

It is used at two places only so yes can to that.

Could you please clarify where minmax is needed? I tried to build Jason's master
tree for x86, commenting the header and it seems building fine. I might be
missing something.

Without it:

   VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:11,
                  from <command-line>:
./arch/powerpc/include/asm/vdso/getrandom.h: In function 
'__arch_get_vdso_rng_data':
./arch/powerpc/include/asm/vdso/getrandom.h:46:9: error: implicit 
declaration of function 'BUILD_BUG' [-Werror=implicit-function-declaration]
    46 |         BUILD_BUG();
       |         ^~~~~~~~~
./arch/powerpc/include/asm/vdso/getrandom.h:47:1: error: no return 
statement in function returning non-void [-Werror=return-type]
    47 | }
       | ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c: In function 
'__cvdso_getrandom_data':
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:76:23: error: implicit 
declaration of function 'min_t' [-Werror=implicit-function-declaration]
    76 |         ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = 
MAX_RW_COUNT */, len);
       |                       ^~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:76:29: error: expected 
expression before 'size_t'
    76 |         ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = 
MAX_RW_COUNT */, len);
       |                             ^~~~~~
In file included from ./include/linux/array_size.h:5,
                  from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:6:
./include/linux/compiler.h:243:33: error: implicit declaration of 
function 'BUILD_BUG_ON_ZERO' [-Werror=implicit-function-declaration]
   243 | #define __must_be_array(a) 
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
       |                                 ^~~~~~~~~~~~~~~~~
./include/linux/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:89:40: note: in 
expansion of macro 'ARRAY_SIZE'
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:196:27: error: expected 
expression before 'size_t'
   196 |         batch_len = min_t(size_t, sizeof(state->batch) - 
state->pos, len);
       |                           ^~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:247:9: error: implicit 
declaration of function 'BUILD_BUG_ON' 
[-Werror=implicit-function-declaration]
   247 |         BUILD_BUG_ON(sizeof(state->batch_key) % 
CHACHA_BLOCK_SIZE != 0);
       |         ^~~~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:93: 
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2

quoted

Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
only once

I have a similar issue to figure out why linux/array_size.h and
uapi/linux/random.h are needed. It seems that I can build the object without
them. Could you please explain?

Without linux/array_size.h:

   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:89:40: error: implicit 
declaration of function 'ARRAY_SIZE' [-Werror=implicit-function-declaration]
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:93: 
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2

Without uapi/linux/random.h:

   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:86:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    86 |                 params->size_of_opaque_state = sizeof(*state);
       |                       ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:87:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    87 |                 params->mmap_prot = PROT_READ | PROT_WRITE;
       |                       ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:23: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    88 |                 params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
       |                       ^~
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:6:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/linux/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:89:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                                         ^~
./include/linux/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/minmax.h:5,
                  from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:7:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    89 |                 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/linux/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:89:40: note: in 
expansion of macro 'ARRAY_SIZE'
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    89 |                 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/linux/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:89:40: note: in 
expansion of macro 'ARRAY_SIZE'
    89 |                 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/linux/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:89:40: note: in 
expansion of macro 'ARRAY_SIZE'
    89 |                 for (size_t i = 0; i < 
ARRAY_SIZE(params->reserved); ++i)
       |                                        ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:90:31: error: invalid 
use of undefined type 'struct vgetrandom_opaque_params'
    90 |                         params->reserved[i] = 0;
       |                               ^~
In file included from ./include/linux/array_size.h:5:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:99:32: error: 
'GRND_NONBLOCK' undeclared (first use in this function); did you mean 
'MAP_NONBLOCK'?
    99 |         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:99:32: note: each 
undeclared identifier is reported only once for each function it appears in
    99 |         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:99:48: error: 
'GRND_RANDOM' undeclared (first use in this function)
    99 |         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:99:62: error: 
'GRND_INSECURE' undeclared (first use in this function)
    99 |         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:93: 
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2

In general, the philosophy adopted to split the headers is to extract the set of
functions used by vDSOs from the linux headers and place them in the vdso headers.
Consequently the linux header includes to vdso one. E.g.: linux/time64.h
includes vdso/time64.h.

IMHO we should follow the same approach, if at all for consistency, unless there
is a valid reason.

Indeed I started with something that didn't build and I did the simplest 
I could to get it build. I agree with you at the end that would be a 
best, can be done in follow-up patches I guess.

...

quoted

quoted

Including uapi/linux/mman.h may still be problematic on
some architectures if they change it in a way that is
incompatible with compat vdso, but at least that can't
accidentally rely on CONFIG_64BIT or something else that
would be wrong there.

Yes that one is tricky. Because uapi/linux/mman.h includes asm/mman.h with the
intention to include uapi/asm/mman.h but when built from the kernel in reality
you get arch/powerpc/include/asm/mman.h and I had to add some ifdefery to
kick-out kernel oddities it contains that pull additional kernel headers.

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 17a77d47ed6d..42a51a993d94 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h

@@ -6,7 +6,7 @@

  #include <uapi/asm/mman.h>

-#ifdef CONFIG_PPC64
+#if defined(CONFIG_PPC64) && !defined(BUILD_VDSO)

  #include <asm/cputable.h>
  #include <linux/mm.h>

I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.

Fully agree.

I am attaching a patch to provide my view on how to minimize the headers
included and use only the vdso/ namespace. Please, before using the code,
consider that I conducted very limited testing.

Note: It should apply clean on Jason's tree.

Let me know your thoughts.

quoted

Christophe

Hi Christophe,

On 27/08/2024 18:14, Christophe Leroy wrote:

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :

quoted

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...

...

quoted

Could you please clarify where minmax is needed? I tried to build Jason's master
tree for x86, commenting the header and it seems building fine. I might be
missing something.

Without it:

  VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:11,
                 from <command-line>:

...

quoted

quoted

Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
only once

I have a similar issue to figure out why linux/array_size.h and
uapi/linux/random.h are needed. It seems that I can build the object without
them. Could you please explain?

Without linux/array_size.h:

  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:89:40: error: implicit

If this is the case, those headers should be defined for the powerpc
implementation only. The generic implementation should be interpreted as the
minimum common denominator in between all the architectures for what concerns
the headers.

-- 
Regards,
Vincenzo

Hi Vincenzo,

Le 29/08/2024 à 14:01, Vincenzo Frascino a écrit :

Hi Christophe,

On 27/08/2024 18:14, Christophe Leroy wrote:

quoted

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :

quoted

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...

...

quoted

quoted

Could you please clarify where minmax is needed? I tried to build Jason's master
tree for x86, commenting the header and it seems building fine. I might be
missing something.

Without it:

   VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:11,
                  from <command-line>:

...

quoted

quoted

quoted

Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
only once

I have a similar issue to figure out why linux/array_size.h and
uapi/linux/random.h are needed. It seems that I can build the object without
them. Could you please explain?

Without linux/array_size.h:

   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:89:40: error: implicit

If this is the case, those headers should be defined for the powerpc
implementation only. The generic implementation should be interpreted as the
minimum common denominator in between all the architectures for what concerns
the headers.

Sorry, I disagree. You can't rely on necessary headers being included 
indirectly by other arch specific headers. getrandom.c uses 
ARRAY_SIZE(), it must include the header that defines ARRAY_SIZE().

At the moment, on x86 you get linux/array.h by change through the 
following chain, that the reason why the build doesn't break:

In file included from ./include/linux/kernel.h:16,
                  from ./include/linux/cpumask.h:11,
                  from ./arch/x86/include/asm/cpumask.h:5,
                  from ./arch/x86/include/asm/msr.h:11,
                  from ./arch/x86/include/asm/vdso/gettimeofday.h:19,
                  from ./include/vdso/datapage.h:164,
                  from 
arch/x86/entry/vdso/../../../../lib/vdso/getrandom.c:9,

 From my point of view you can't expect such a chain from each architecture.

Christophe

...

quoted

quoted

Without linux/array_size.h:

   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:89:40: error: implicit

If this is the case, those headers should be defined for the powerpc
implementation only. The generic implementation should be interpreted as the
minimum common denominator in between all the architectures for what concerns
the headers.

Sorry, I disagree. You can't rely on necessary headers being included indirectly
by other arch specific headers. getrandom.c uses ARRAY_SIZE(), it must include
the header that defines ARRAY_SIZE().

At the moment, on x86 you get linux/array.h by change through the following
chain, that the reason why the build doesn't break:

In file included from ./include/linux/kernel.h:16,
                 from ./include/linux/cpumask.h:11,
                 from ./arch/x86/include/asm/cpumask.h:5,
                 from ./arch/x86/include/asm/msr.h:11,
                 from ./arch/x86/include/asm/vdso/gettimeofday.h:19,
                 from ./include/vdso/datapage.h:164,
                 from arch/x86/entry/vdso/../../../../lib/vdso/getrandom.c:9,

From my point of view you can't expect such a chain from each architecture.

--->8---

I can't see which header provides you with min_t() or ARRAY_SIZE().

Good point, this needs to be addressed by my patch, I will extend it, do some
more testing and post it again next week.

--->8---


As I explained in my other email (snippet above), my patch should address the
cases of ARRAY_SIZE() and min_t() which are directly used by getrandom.c. My
comment here refers to the cases not directly used by getrandom.c or the Generic
vDSO library more in general (if any).

Hope this clarifies.

Christophe

-- 
Regards,
Vincenzo

Hi Vicenzo,

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...



I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.

I am attaching a patch to provide my view on how to minimize the headers
included and use only the vdso/ namespace. Please, before using the code,
consider that I conducted very limited testing.

Note: It should apply clean on Jason's tree.

Let me know your thoughts.

Your patch looks nice, maybe a bit too much. For instance getrandom.c 
can include directly asm/vdso/page.h instead of creating vdso/page.h

Or create a vdso/page.h that only use CONFIG_PAGE_SHIFT and doesn't 
include anything from architectures.

We should also keep PROT_READ and PROT_WRITE in getrandom.c , that's 
better for readability. Same for MAP_DROPPABLE | MAP_ANONYMOUS. I can't 
see the benefit of hiding them in a header.

I can't see which header provides you with min_t() or ARRAY_SIZE().

I think you should also work on removing headers included by 
arch/x86/include/asm/vdso/gettimeofday.h which is included by 
include/vdso/datapage.h :

   #include <uapi/linux/time.h>
   #include <asm/vgtod.h>
   #include <asm/vvar.h>
   #include <asm/unistd.h>
   #include <asm/msr.h>
   #include <asm/pvclock.h>
   #include <clocksource/hyperv_timer.h>

As a comparison, the one from powerpc only includes the following one so 
it pulls a lot less non-vdso headers:

   #include <asm/vdso/timebase.h>
   #include <asm/barrier.h>
   #include <asm/unistd.h>
   #include <uapi/linux/time.h>

Christophe

Hi Christophe,

On 27/08/2024 18:38, LEROY Christophe wrote:

Hi Vicenzo,

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :

quoted

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...



I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.

I am attaching a patch to provide my view on how to minimize the headers
included and use only the vdso/ namespace. Please, before using the code,
consider that I conducted very limited testing.

Note: It should apply clean on Jason's tree.

Let me know your thoughts.

Your patch looks nice, maybe a bit too much. For instance getrandom.c 
can include directly asm/vdso/page.h instead of creating vdso/page.h

Or create a vdso/page.h that only use CONFIG_PAGE_SHIFT and doesn't 
include anything from architectures.

IMHO there should be only one place per architecture where PAGE_SIZE and
PAGE_MASK are defined. This makes sure that if there is a problem, we do not
have multiple places to look into.

The indirection helps to keep consistent the namespace and allows for future
extension. Similar logic has been used during my original vDSO headers
definition and implementation.

We should also keep PROT_READ and PROT_WRITE in getrandom.c , that's 
better for readability. Same for MAP_DROPPABLE | MAP_ANONYMOUS. I can't 
see the benefit of hiding them in a header.

The idea is not to make the code unreadable but to defer to the architecture the
decision of prot and flags avoiding the inclusion of headers coming from the
uapi namespace.

I can't see which header provides you with min_t() or ARRAY_SIZE().

Good point, this needs to be addressed by my patch, I will extend it, do some
more testing and post it again next week.

I think you should also work on removing headers included by 
arch/x86/include/asm/vdso/gettimeofday.h which is included by 
include/vdso/datapage.h :

   #include <uapi/linux/time.h>
   #include <asm/vgtod.h>
   #include <asm/vvar.h>
   #include <asm/unistd.h>
   #include <asm/msr.h>
   #include <asm/pvclock.h>
   #include <clocksource/hyperv_timer.h>

As a comparison, the one from powerpc only includes the following one so 
it pulls a lot less non-vdso headers:

   #include <asm/vdso/timebase.h>
   #include <asm/barrier.h>
   #include <asm/unistd.h>
   #include <uapi/linux/time.h>

Christophe

This does not seem a concern, in fact I believe that the generic vDSO library
should not mandate to the architecture how to organize headers. As far as the
requirements are satisfied each architecture should be able to define its own
naming and conventions independently.

-- 
Regards,
Vincenzo

Building a VDSO32 on a 64 bits kernel is problematic when some
system headers are included. See commit 8c59ab839f52 ("lib/vdso:
Enable common headers") for more details.

Minimise the amount of headers by moving needed items into
dedicated common headers.

Removing linux/time64.h leads to missing 'struct timespec64' in
x86's asm/pvclock.h. Add a forward declaration of that struct in
that file.

Signed-off-by: Christophe Leroy <redacted>
---
v3: Split PAGE_SIZE/PAGE_MASK subject in another patch and explained the forward declaration of 'struct timespec64' in commit message.
---
 arch/x86/include/asm/pvclock.h | 1 +
 include/vdso/helpers.h         | 1 +
 lib/vdso/getrandom.c           | 8 +++-----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 0c92db84469d..6e4f8fae3ce9 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h

@@ -5,6 +5,7 @@
 #include <asm/clocksource.h>
 #include <asm/pvclock-abi.h>
 
+struct timespec64;
 /* some helper functions for xen and kvm pv clock sources */
 u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src);
 u64 pvclock_clocksource_read_nowd(struct pvclock_vcpu_time_info *src);

diff --git a/include/vdso/helpers.h b/include/vdso/helpers.h
index 73501149439d..3ddb03bb05cb 100644
--- a/include/vdso/helpers.h
+++ b/include/vdso/helpers.h

@@ -4,6 +4,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include <asm/barrier.h>
 #include <vdso/datapage.h>
 
 static __always_inline u32 vdso_read_begin(const struct vdso_data *vd)

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 5874e3072bfe..5d79663b026b 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c

@@ -4,15 +4,13 @@
  */
 
 #include <linux/array_size.h>
-#include <linux/cache.h>
-#include <linux/kernel.h>
-#include <linux/time64.h>
+#include <linux/minmax.h>
 #include <vdso/datapage.h>
 #include <vdso/getrandom.h>
+#include <vdso/unaligned.h>
 #include <asm/vdso/getrandom.h>
-#include <asm/vdso/vsyscall.h>
-#include <asm/unaligned.h>
 #include <uapi/linux/mman.h>
+#include <uapi/linux/random.h>
 
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\

-- 
2.44.0

Performing SMP atomic operations on u64 fails on powerpc32:

    CC      drivers/char/random.o
  In file included from <command-line>:
  drivers/char/random.c: In function 'crng_reseed':
  ././include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_391' declared with attribute error: Need native word sized stores/loads for atomicity.
    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
        |                                             ^
  ././include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert'
    491 |                         prefix ## suffix();                             \
        |                         ^~~~~~
  ././include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
        |         ^~~~~~~~~~~~~~~~~~~
  ././include/linux/compiler_types.h:513:9: note: in expansion of macro 'compiletime_assert'
    513 |         compiletime_assert(__native_word(t),                            \
        |         ^~~~~~~~~~~~~~~~~~
  ./arch/powerpc/include/asm/barrier.h:74:9: note: in expansion of macro 'compiletime_assert_atomic_type'
     74 |         compiletime_assert_atomic_type(*p);                             \
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/asm-generic/barrier.h:172:55: note: in expansion of macro '__smp_store_release'
    172 | #define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0)
        |                                                       ^~~~~~~~~~~~~~~~~~~
  drivers/char/random.c:286:9: note: in expansion of macro 'smp_store_release'
    286 |         smp_store_release(&__arch_get_k_vdso_rng_data()->generation, next_gen + 1);
        |         ^~~~~~~~~~~~~~~~~

Random driver generation is handled as unsigned long not u64, see for
instance base_cnrg or struct crng.

But on vDSO it needs to be an u64 not just an unsigned long because of
32 bits VDSO being used with 64 bits kernels.

On random side however it is an unsigned long hence a 32 bits value on
32 bits architectures, so just cast it to unsigned long for the
smp_store_release(). A side effect is that on big endian architectures
the store will be performed in the upper 32 bits. It is not an issue
on its own because the vDSO site doesn't mind the value, it only
checks differences. Just make sure that the vDSO side checks the full
64 bits, for that the local current_generation has to be u64 as well.

Signed-off-by: Christophe Leroy <redacted>
---
v3: Cast to unsigned long in random and use u64 in vDSO instead of changing generation field to unsigned long
---
 drivers/char/random.c | 9 ++++++++-
 lib/vdso/getrandom.c  | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 77968309e2c2..dc9bab51e74d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c

@@ -282,8 +282,15 @@ static void crng_reseed(struct work_struct *work)
 	 * former to arrive at the latter. Use smp_store_release so that this
 	 * is ordered with the write above to base_crng.generation. Pairs with
 	 * the smp_rmb() before the syscall in the vDSO code.
+	 *
+	 * Cast to unsigned long for 32 bits architectures as atomic 64 bits
+	 * operations are not supported on those architectures. Anyway
+	 * base_crng.generation is a 32 bits value so it is ok. On big endian
+	 * architectures it will be stored in the upper 32 bits but that's ok
+	 * because the vDSO side only checks whether the value changed, it
+	 * doesn't use or interpret the value.
 	 */
-	smp_store_release(&__arch_get_k_vdso_rng_data()->generation, next_gen + 1);
+	smp_store_release((unsigned long *)&__arch_get_k_vdso_rng_data()->generation, next_gen + 1);
 #endif
 	if (!static_branch_likely(&crng_is_ready))
 		crng_init = CRNG_READY;

diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 5d79663b026b..8027b2711b69 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c

@@ -69,7 +69,7 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 	struct vgetrandom_state *state = opaque_state;
 	size_t batch_len, nblocks, orig_len = len;
 	bool in_use, have_retried = false;
-	unsigned long current_generation;
+	u64 current_generation;
 	void *orig_buffer = buffer;
 	u32 counter[2] = { 0 };
 

-- 
2.44.0

On Tue, Aug 27, 2024 at 09:31:50AM +0200, Christophe Leroy wrote:

Performing SMP atomic operations on u64 fails on powerpc32:

Thanks for this, and nice catch on the vDSO side checking on big endian.
I've applied this, fixing up the commit message and the comment,
maintaining the reverse christmas tree in getrandom.c, and adding tglx's
suggested-by tag.

Jason