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 04:26:32PM +0200, Christophe Leroy wrote:
> And thanks for playing up with it while I was sleeping and getting ideas 
> too.
> 
> Did you learn powerpc assembly during the night or did you know it already ?

I don't really know ppc assembly. I had perused the tree over the last
week and gotten some feel for it when reviewing patches, but I don't
have anything memorized (except, perhaps, the eieio instruction [1,2]).
Last night after sending the first broken patch I went out to play (I
play jazz guitar ~every night these days), and the whole time I kept
thinking about the problem. So first thing I did when I got home was try
to fake my way through some ppc asm. A fun mini project for me.

[1] https://lore.kernel.org/lkml/Pine.LNX.4.33.0110120919130.31677-100000@penguin.transmeta.com/
[2] https://lore.kernel.org/lkml/alpine.LFD.2.00.0904141006170.18124@localhost.localdomain/

> At the end I ended up with something which I think is simple enough for 
> a backport to stable.

It seems like a good patch indeed, and hopefully small enough that
Michael will let me carry in my tree for 6.12, per the plan.

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

Jason