Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation

[Jason A. Donenfeld]

On Thu, Jun 06, 2024 at 12:10:00AM +0200, Thomas Gleixner wrote:
> Jason!
> 
> On Wed, Jun 05 2024 at 23:03, Thomas Gleixner wrote:
> > On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> >> + */
> >> +#ifdef CONFIG_64BIT
> >> +typedef u64 vdso_kernel_ulong;
> >> +#else
> >> +typedef u32 vdso_kernel_ulong;
> >> +#endif
> >
> > All of this is pointless because if a 32-bit application runs on a
> > 64-bit kernel it has to use the 64-bit 'generation'. So why on earth do
> > we need magic here for a 32-bit kernel?
> >
> > Just use u64 for both and spare all this voodoo. We're seriously not
> > "optimizing" for 32-bit kernels.
> 
> All what happens on a 32-bit kernel is that the RNG will store the
> unsigned long (32bit) generation into a 64bit variable:
> 
> 	smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
> 
> As the upper 32bit are always zero, there is no issue vs. load store
> tearing at all. So there is zero benefit for this aside of slightly
> "better" user space code when running on a 32-bit kernel. Who cares?

Oh yea. Okay, great. I was concerned about the tearing, but I guess it's
really a non issue. So I'll just make it a u64 and all of this
complexity can just go away. Thanks for thinking about it in a less
convoluted way than me.

> While staring at this I wonder where the corresponding
> smp_load_acquire() is. I haven't found one in the VDSO code.
> READ_ONCE() is only equivalent on a few architectures.
> 
> But, what does that store_release() buy at all? There is zero ordering
> vs. anything in the kernel and neither against user space.
> 
> If that smp_store_release() serves a purpose then it really has to be
> extensively documented especially as the kernel itself simply uses
> WRITE/READ_ONCE() for base_rng.generation.

This came up here too: https://lore.kernel.org/all/Y3l6ocn1dTN0+1GK@xxxxxxxxx/

It's to order the writes to the generation counter and is_ready.

Jason