Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Christophe Leroy]

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