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
