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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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://lore.kernel.org/all/20240906190655.2777023-1-Jason@zx2c4.com/