[PATCH 0/4] Fixups for random vDSO

[PATCH 0/4] Fixups for random vDSO

[Christophe Leroy]

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

[PATCH 1/4] asm-generic/unaligned.h: Extract common header for vDSO

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
 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

[PATCH 2/4] random: vDSO: Don't use PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
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

Re: [PATCH 2/4] random: vDSO: Don't use PAGE_SIZE and PAGE_MASK

[Jason A. Donenfeld]

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

Re: [PATCH 2/4] random: vDSO: Don't use PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

Le 27/08/2024 à 09:49, Jason A. Donenfeld a écrit :
> 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...

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

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

Re: [PATCH 2/4] random: vDSO: Don't use PAGE_SIZE and PAGE_MASK

[Jason A. Donenfeld]

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.

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

[PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Jason A. Donenfeld]

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 <christophe.leroy@csgroup.eu>
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 <christophe.leroy@csgroup.eu>
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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

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

> 
>  From 0d9a3d68cd6222395a605abd0ac625c41d4cabfa Mon Sep 17 00:00:00 2001
> From: Christophe Leroy <christophe.leroy@csgroup.eu>
> 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 <christophe.leroy@csgroup.eu>
> 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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Arnd Bergmann]

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 <christophe.leroy@csgroup.eu>
> 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 <christophe.leroy@csgroup.eu>
> 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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

Le 27/08/2024 à 11:59, Arnd Bergmann a écrit :
> 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 <christophe.leroy@csgroup.eu>
>> 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 <christophe.leroy@csgroup.eu>
>> 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?

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


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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

Hi Christophe,

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

...


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

...

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

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

[-- Attachment #2: 0001-random-VDSO-Use-only-headers-from-the-vdso-namespace.patch --]
[-- Type: text/x-patch, Size: 5622 bytes --]

From 1933028eaeebc5c053309379c43ddea5b460c25e Mon Sep 17 00:00:00 2001
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
Date: Tue, 27 Aug 2024 16:44:32 +0100
Subject: [PATCH] random: VDSO: Use only headers from the vdso/ namespace

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

Refactor the code to make sure that the generic library uses only the
allowed namespace.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 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/vdso/getrandom.h              |  1 +
 include/vdso/mman.h                   |  7 +++++++
 include/vdso/page.h                   |  7 +++++++
 lib/vdso/getrandom.c                  | 20 ++++++--------------
 7 files changed, 53 insertions(+), 14 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/mman.h
 create mode 100644 include/vdso/page.h

diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
index b96e674cafde..457f237bd602 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>
 
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 */
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 */
diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
index a8b7c14b0ae0..9cf65252ee88 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
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 */
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 */
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 938ca539aaa6..0936fd1ca6ce 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -3,19 +3,11 @@
  * 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>
 
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
@@ -68,7 +60,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 +71,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] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :
> Hi Christophe,
> 
> On 27/08/2024 11:49, Christophe Leroy wrote:
> 
> ...
> 
> 
>>>
>>> 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


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

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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino]

Hi Christophe,

On 27/08/2024 18:14, Christophe Leroy wrote:
> 
> 
> Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :
>> Hi Christophe,
>>
>> On 27/08/2024 11:49, Christophe Leroy wrote:
>>
>> ...

...

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

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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Christophe Leroy]

Hi Vincenzo,

Le 29/08/2024 à 14:01, Vincenzo Frascino a écrit :
> Hi Christophe,
> 
> On 27/08/2024 18:14, Christophe Leroy wrote:
>>
>>
>> Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :
>>> Hi Christophe,
>>>
>>> On 27/08/2024 11:49, Christophe Leroy wrote:
>>>
>>> ...
> 
> ...
> 
>>>
>>> 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>:
> ...
> 
>>
>>
>>>
>>>> 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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino]

...


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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[LEROY Christophe]

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

Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino]

Hi Christophe,

On 27/08/2024 18:38, LEROY Christophe wrote:
> 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.
> 

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

[PATCH 3/4] random: vDSO: Clean header inclusion in getrandom

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
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

[PATCH 4/4] random: vDSO: don't use 64 bits atomics on 32 bits architectures

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
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

Re: [PATCH 4/4] random: vDSO: don't use 64 bits atomics on 32 bits architectures

[Jason A. Donenfeld]

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