Message-ID: <17437f43-9d1f-4263-888e-573a355cb0b5@arm.com>
Date: Tue, 27 Aug 2024 17:05:01 +0100
From: Vincenzo Frascino <vincenzo.frascino@....com>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
 Arnd Bergmann <arnd@...db.de>, "Jason A . Donenfeld" <Jason@...c4.com>
Cc: Theodore Ts'o <tytso@....edu>, Andy Lutomirski <luto@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
 linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
 Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

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.

> 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?

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.

...

>>
>> 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.

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

-- 
Regards,
Vincenzo
View attachment "0001-random-VDSO-Use-only-headers-from-the-vdso-namespace.patch" of type "text/x-patch" (5622 bytes)

Powered by blists - more mailing lists