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

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

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