Received-SPF: pass (google.com: domain of linux-crypto+bounces-4850-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 7 Jun 2024 17:59:42 +0200
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org, patches@lists.linux.dev,
	linux-crypto@vger.kernel.org, linux-api@vger.kernel.org,
	x86@kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
	Carlos O'Donell <carlos@redhat.com>,
	Florian Weimer <fweimer@redhat.com>, Arnd Bergmann <arnd@arndb.de>,
	Jann Horn <jannh@google.com>,
	Christian Brauner <brauner@kernel.org>,
	David Hildenbrand <dhildenb@redhat.com>
Subject: Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom()
 implementation
Message-ID: <ZmMubnTe6m_ET-9w@zx2c4.com>
References: <874ja73xx7.ffs@tglx>
 <87v82n2g93.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <87v82n2g93.ffs@tglx>

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

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

Jason