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-ja...@zx2c4.com/
