# Re: \[PATCH\] random: vDSO: Redefine PAGE\_SIZE and PAGE\_MASK - From: Christophe Leroy \ - Date: 2024-08-27 17:14:47 - List: linuxppc-dev - Page: https://ratatoskr.run/linuxppc-dev/2024/08/2724144 - Thread: https://ratatoskr.run/linuxppc-dev/2024/08/2723152/t.md ```text 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 : ./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 : /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 : /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 '' 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 >> >> -#ifdef CONFIG_PPC64 >> +#if defined(CONFIG_PPC64) && !defined(BUILD_VDSO) >> >>  #include >>  #include >> > > 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 > ```