Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 1 Jan 2023 16:53:22 +0100
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        tglx@linutronix.de, 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>
Subject: Re: [PATCH v13 6/7] random: introduce generic vDSO getrandom()
 implementation
Message-ID: <Y7GschU1qZPFZAhm@zx2c4.com>
References: <20221221142327.126451-1-Jason@zx2c4.com>
 <20221221142327.126451-7-Jason@zx2c4.com>
 <Y6OHa196S8e1mImg@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y6OHa196S8e1mImg@sol.localdomain>
Precedence: bulk

On Wed, Dec 21, 2022 at 02:23:39PM -0800, Eric Biggers wrote:
> On Wed, Dec 21, 2022 at 03:23:26PM +0100, Jason A. Donenfeld wrote:
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index 6425f5f838e0..660cd15b6228 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -60,6 +60,7 @@
> >  #include <crypto/blake2s.h>
> >  #ifdef CONFIG_VDSO_GETRANDOM
> >  #include <vdso/getrandom.h>
> > +#include <vdso/datapage.h>
> >  #endif
> >  #include <asm/archrandom.h>
> >  #include <asm/processor.h>
> > @@ -407,6 +408,9 @@ static void crng_reseed(struct work_struct *work)
> >	/*
> >	 * We copy the new key into the base_crng, overwriting the old one,
> >	 * and update the generation counter. We avoid hitting ULONG_MAX,
> >	 * because the per-cpu crngs are initialized to ULONG_MAX, so this
> >	 * forces new CPUs that come online to always initialize.
> >	 */
> >	spin_lock_irqsave(&base_crng.lock, flags);
> >	memcpy(base_crng.key, key, sizeof(base_crng.key));
> >	next_gen = base_crng.generation + 1;
> >  	if (next_gen == ULONG_MAX)
> >  		++next_gen;
> >  	WRITE_ONCE(base_crng.generation, next_gen);
> > +#ifdef CONFIG_VDSO_GETRANDOM
> > +	smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
> > +#endif
> 
> It's confusing that "uninitialized generation" is ULONG_MAX in the per-cpu
> crngs, but 0 in the vdso_rng_data.  That results in a weird off-by one thing,
> where the vdso_rng_data generation number has to be 1 higher.
> 
> Would it be possible to use 0 for both?

It might be, but this will involve some changes to how the batching
works too, so I think I'd like to do that separately, if at all.
However, I'll add a comment there noting what's happening so it's a bit
less confusing.

Jason