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