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

[Thomas Gleixner <tglx@linutronix.de>]

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?

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.

Thanks,

         tglx