Message-ID: <c187369e-a034-4d32-bbed-cacb41ddd65e@csgroup.eu>
Date: Sat, 7 Sep 2024 17:15:17 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "Jason A. Donenfeld" <Jason@...c4.com>
Cc: linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
 Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>,
 Naveen N Rao <naveen@...nel.org>,
 Vincenzo Frascino <vincenzo.frascino@....com>,
 Andrei Vagin <avagin@...il.com>
Subject: Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up
 getrandom() vDSO implementation on VDSO32")



Le 07/09/2024 à 16:35, Jason A. Donenfeld a écrit :
> On Fri, Sep 06, 2024 at 08:54:49PM +0200, Jason A. Donenfeld wrote:
>> On Fri, Sep 06, 2024 at 05:14:43PM +0200, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
>>>> On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
>>>>
>>>>> On the long run I wonder if we should try to find a more generic
>>>>> solution for getrandom instead of requiring each architecture to handle
>>>>> it. On gettimeofday the selection of the right page is embeded in the
>>>>> generic part, see for instance :
>>>>>
>>>>> static __maybe_unused __kernel_old_time_t
>>>>> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
>>>>> {
>>>>> 	__kernel_old_time_t t;
>>>>>
>>>>> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
>>>>> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>>>> 		vd = __arch_get_timens_vdso_data(vd);
>>>>>
>>>>> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
>>>>>
>>>>> 	if (time)
>>>>> 		*time = t;
>>>>>
>>>>> 	return t;
>>>>> }
>>>>>
>>>>> and powerpc just provides:
>>>>>
>>>>> static __always_inline
>>>>> const struct vdso_data *__arch_get_timens_vdso_data(const struct
>>>>> vdso_data *vd)
>>>>> {
>>>>> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
>>>>> }
>>>>
>>>> It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
>>>> have this problem at all, because the layout of their vvars doesn't
>>>> require it. So the vd->clock_mode access is unnecessary.
>>>>
>>>>> Or another solution could be to put random data in a third page that is
>>>>> always at the same place regardless of timens ?
>>>>
>>>> Maybe that's the easier way, yea. Potentially wasteful, though.
>>>>
>>>
>>> Indeed I just looked at Loongarch and that's exactly what they do: they
>>> have a third page after the two pages dedicated to TIME for arch
>>> specific data, and they have added getrandom data there.
>>>
>>> The third page is common to every process so it won't waste more than a
>>> few bytes. It doesn't worry me even on the older boards that only have
>>> 32 Mbytes of RAM.
>>>
>>> So yes, I may have a look at that in the future, what we have at the
>>> moment is good enough to move forward.
>>
>> My x86 code is kind of icky for this:
>>
>> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
>> {
>>          if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>                  return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
>>          return &__vdso_rng_data;
>> }
>>
>> Doing the subtraction like that means that this is more clearly correct.
>> But it also makes the compiler insert two jumps for the branch, and then
>> reads the addresses of those variables and such.
>>
>> If I change it to:
>>
>> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
>> {
>>          if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>                  return (void *)&__vdso_rng_data + (3UL << CONFIG_PAGE_SHIFT);
>>          return &__vdso_rng_data;
>> }
>>
>> Then there's a much nicer single `cmov` with no branching.
>>
>> But if I want to do that for real, I'll have to figure out what set of
>> nice compile-time constants I can use. I haven't looked into this yet.
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20240906190655.2777023-1-Jason%40zx2c4.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C3ee8b35fe848434e72fd08dccf4a67ff%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638613165688600378%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=g4zcMonjJNYhwrUWeCDoL5Ri7Mbg5hVQJyZNU2zH4Pc%3D&reserved=0

Looks good.

Allthough other architectures don't use defines but enums for that:

arch/arm64/kernel/vdso.c-36-
arch/arm64/kernel/vdso.c-37-enum vvar_pages {
arch/arm64/kernel/vdso.c:38:	VVAR_DATA_PAGE_OFFSET,
arch/arm64/kernel/vdso.c:39:	VVAR_TIMENS_PAGE_OFFSET,
arch/arm64/kernel/vdso.c-40-	VVAR_NR_PAGES,
arch/arm64/kernel/vdso.c-41-};
--
arch/loongarch/include/asm/vdso/vdso.h-36-
arch/loongarch/include/asm/vdso/vdso.h-37-enum vvar_pages {
arch/loongarch/include/asm/vdso/vdso.h:38:	VVAR_GENERIC_PAGE_OFFSET,
arch/loongarch/include/asm/vdso/vdso.h:39:	VVAR_TIMENS_PAGE_OFFSET,
arch/loongarch/include/asm/vdso/vdso.h-40-	VVAR_LOONGARCH_PAGES_START,
arch/loongarch/include/asm/vdso/vdso.h-41-	VVAR_LOONGARCH_PAGES_END = 
VVAR_LOONGARCH_PAGES_START + LOONGARCH_VDSO_DATA_PAGES - 1,
--
arch/powerpc/kernel/vdso.c-54-
arch/powerpc/kernel/vdso.c-55-enum vvar_pages {
arch/powerpc/kernel/vdso.c:56:	VVAR_DATA_PAGE_OFFSET,
arch/powerpc/kernel/vdso.c:57:	VVAR_TIMENS_PAGE_OFFSET,
arch/powerpc/kernel/vdso.c-58-	VVAR_NR_PAGES,
arch/powerpc/kernel/vdso.c-59-};
--
arch/riscv/kernel/vdso.c-19-
arch/riscv/kernel/vdso.c-20-enum vvar_pages {
arch/riscv/kernel/vdso.c:21:	VVAR_DATA_PAGE_OFFSET,
arch/riscv/kernel/vdso.c:22:	VVAR_TIMENS_PAGE_OFFSET,
arch/riscv/kernel/vdso.c-23-	VVAR_NR_PAGES,
arch/riscv/kernel/vdso.c-24-};
--
arch/s390/kernel/vdso.c-31-
arch/s390/kernel/vdso.c-32-enum vvar_pages {
arch/s390/kernel/vdso.c:33:	VVAR_DATA_PAGE_OFFSET,
arch/s390/kernel/vdso.c:34:	VVAR_TIMENS_PAGE_OFFSET,
arch/s390/kernel/vdso.c-35-	VVAR_NR_PAGES,
arch/s390/kernel/vdso.c-36-};

Christophe

Powered by blists - more mailing lists