Message-Id: <cb66b582-ba63-4a5a-9df8-b07288f1f66d@app.fastmail.com>
Date: Tue, 27 Aug 2024 11:59:25 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Jason A . Donenfeld" <Jason@...c4.com>,
 "Christophe Leroy" <christophe.leroy@...roup.eu>
Cc: "Theodore Ts'o" <tytso@....edu>, "Andy Lutomirski" <luto@...nel.org>,
 "Thomas Gleixner" <tglx@...utronix.de>,
 "Vincenzo Frascino" <vincenzo.frascino@....com>,
 "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

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@...roup.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@...roup.eu>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.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?

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

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.

     Arnd

Powered by blists - more mailing lists