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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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
>