Re: [PATCH v2] aarch64: vdso: Wire up getrandom() vDSO implementation

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 02/09/24 10:25, Jason A. Donenfeld wrote:
> On Mon, Sep 02, 2024 at 03:19:56PM +0200, Christophe Leroy wrote:
>>
>>
>> Le 02/09/2024 à 15:11, Jason A. Donenfeld a écrit :
>>> Hey Christophe (for header logic) & Will (for arm64 stuff),
>>>
>>> On Fri, Aug 30, 2024 at 09:28:29AM -0300, Adhemerval Zanella Netto wrote:
>>>>>> diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
>>>>>> index 938ca539aaa6..7c9711248d9b 100644
>>>>>> --- a/lib/vdso/getrandom.c
>>>>>> +++ b/lib/vdso/getrandom.c
>>>>>> @@ -5,6 +5,7 @@
>>>>>>   
>>>>>>   #include <linux/array_size.h>
>>>>>>   #include <linux/minmax.h>
>>>>>> +#include <linux/mm.h>
>>>>>>   #include <vdso/datapage.h>
>>>>>>   #include <vdso/getrandom.h>
>>>>>>   #include <vdso/unaligned.h>
>>>>>
>>>>> Looks like this should be a separate change?
>>>>
>>>>
>>>> It is required so arm64 can use  c-getrandom-y, otherwise vgetrandom.o build
>>>> fails:
>>>>
>>>> CC      arch/arm64/kernel/vdso/vgetrandom.o
>>>> In file included from ./include/uapi/linux/mman.h:5,
>>>>                   from /mnt/projects/linux/linux-git/lib/vdso/getrandom.c:13,
>>>>                   from <command-line>:
>>>> ./arch/arm64/include/asm/mman.h: In function ‘arch_calc_vm_prot_bits’:
>>>> ./arch/arm64/include/asm/mman.h:14:13: error: implicit declaration of function ‘system_supports_bti’ [-Werror=implicit-function-declaration]
>>>>     14 |         if (system_supports_bti() && (prot & PROT_BTI))
>>>>        |             ^~~~~~~~~~~~~~~~~~~
>>>> ./arch/arm64/include/asm/mman.h:15:24: error: ‘VM_ARM64_BTI’ undeclared (first use in this function); did you mean ‘ARM64_BTI’?
>>>>     15 |                 ret |= VM_ARM64_BTI;
>>>>        |                        ^~~~~~~~~~~~
>>>>        |                        ARM64_BTI
>>>> ./arch/arm64/include/asm/mman.h:15:24: note: each undeclared identifier is reported only once for each function it appears in
>>>> ./arch/arm64/include/asm/mman.h:17:13: error: implicit declaration of function ‘system_supports_mte’ [-Werror=implicit-function-declaration]
>>>>     17 |         if (system_supports_mte() && (prot & PROT_MTE))
>>>>        |             ^~~~~~~~~~~~~~~~~~~
>>>> ./arch/arm64/include/asm/mman.h:18:24: error: ‘VM_MTE’ undeclared (first use in this function)
>>>>     18 |                 ret |= VM_MTE;
>>>>        |                        ^~~~~~
>>>> ./arch/arm64/include/asm/mman.h: In function ‘arch_calc_vm_flag_bits’:
>>>> ./arch/arm64/include/asm/mman.h:32:24: error: ‘VM_MTE_ALLOWED’ undeclared (first use in this function)
>>>>     32 |                 return VM_MTE_ALLOWED;
>>>>        |                        ^~~~~~~~~~~~~~
>>>> ./arch/arm64/include/asm/mman.h: In function ‘arch_validate_flags’:
>>>> ./arch/arm64/include/asm/mman.h:59:29: error: ‘VM_MTE’ undeclared (first use in this function)
>>>>     59 |         return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED);
>>>>        |                             ^~~~~~
>>>> ./arch/arm64/include/asm/mman.h:59:52: error: ‘VM_MTE_ALLOWED’ undeclared (first use in this function)
>>>>     59 |         return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED);
>>>>        |                                                    ^~~~~~~~~~~~~~
>>>> arch/arm64/kernel/vdso/vgetrandom.c: In function ‘__kernel_getrandom’:
>>>> arch/arm64/kernel/vdso/vgetrandom.c:18:25: error: ‘ENOSYS’ undeclared (first use in this function); did you mean ‘ENOSPC’?
>>>>     18 |                 return -ENOSYS;
>>>>        |                         ^~~~~~
>>>>        |                         ENOSPC
>>>> cc1: some warnings being treated as errors
>>>>
>>>> I can move to a different patch, but this is really tied to this patch.
>>>
>>> Adhemerval kept this change in this patch for v3, which, if it's
>>> necessary, is fine with me. But I was looking to see if there was
>>> another way of doing it, because including linux/mm.h inside of vdso
>>> code is kind of contrary to your project with e379299fe0b3 ("random:
>>> vDSO: minimize and simplify header includes").
>>>
>>> getrandom.c includes uapi/linux/mman.h for the mmap constants. That
>>> seems fine; it's userspace code after all. But then uapi/linux/mman.h
>>> has this:
>>>
>>>     #include <asm/mman.h>
>>>     #include <asm-generic/hugetlb_encode.h>
>>>     #include <linux/types.h>
>>>
>>> The asm-generic/ one resolves to uapi/asm-generic. But the asm/ one
>>> resolves to arch code, which is where we then get in trouble on ARM,
>>> where arch/arm64/include/asm/mman.h has all sorts of kernel code in it.
>>>
>>> Maybe, instead, it should resolve to arch/arm64/include/uapi/asm/mman.h,
>>> which is the header that userspace actually uses in normal user code?
>>>
>>> Is this a makefile problem? What's going on here? Seems like this is
>>> something worth sorting out. Or I can take Adhemerval's v3 as-is and
>>> we'll grit our teeth and work it out later, as you prefer. But I thought
>>> I should mention it.
>>
>> That's a tricky problem, I also have it on powerpc, see patch 5, I 
>> solved it that way:
>>
>> In the Makefile:
>> -ccflags-y := -fno-common -fno-builtin
>> +ccflags-y := -fno-common -fno-builtin -DBUILD_VDSO
>>
>> In arch/powerpc/include/asm/mman.h:
>>
>> 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>
>>
>> So that the only thing that remains in arch/powerpc/include/asm/mman.h 
>> when building a VDSO is #include <uapi/asm/mman.h>
>>
>> I got the idea from ARM64, they use something similar in their 
>> arch/arm64/include/asm/rwonce.h
> 
> That seems reasonable enough. Adhemerval - do you want to incorporate
> this solution for your v+1? And Will, is it okay to keep that as one
> patch, as Christophe has done, rather than splitting it, so the whole
> change is hermetic?

Sure, I will do it for v4.