Re: [PATCH v10 2/3] arm64: random: Add data to pool from setup_arch()

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Brown <broonie@kernel.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v10 2/3] arm64: random: Add data to pool from setup_arch()
Date: Wed, 15 Jan 2020 14:36:32 +0100	[thread overview]
Message-ID: <CAKv+Gu-eUkAtft9d+=zvnHiqQUBRGSJX9M03zF1i9Ms+oMstAA@mail.gmail.com> (raw)
In-Reply-To: <20200115124238.GF21692@willie-the-truck>

On Wed, 15 Jan 2020 at 13:42, Will Deacon <will@kernel.org> wrote:
>
> On Wed, Jan 15, 2020 at 12:07:03PM +0000, Mark Brown wrote:
> > On Wed, Jan 15, 2020 at 09:16:16AM +0000, Will Deacon wrote:
> > > On Wed, Jan 15, 2020 at 08:48:46AM +0100, Ard Biesheuvel wrote:
> >
> > > > > Note that we are only adding data here, it will be mixed into the pool
> > > > > but won't be credited as entropy. There are currently no suitable
> > > > > interfaces for that at present - extending the random code to provide
> >
> > > > This is slightly unfortunate, as this way, we lose the ability to use
> > > > random.trust_cpu=1 to get the entropy credited and initialize CRNG
> > > > early.
> >
> > Right.  OTOH that's a bit of a mess to do, I do have some
> > thoughts but it's a bit of a mess trying to do it tastefully,
> > especially when considering that you probably don't want an
> > interface that it's easy for something to misuse.  The effort
> > involved certainly seems large enough to handle separately.
>
> Maybe, but see below...
>
> > > Agreed. Do you think we should wait for that support before merging the
> > > series? Given that I don't know of any CPUs implementing this extension,
> > > we can probably afford not to rush this in.
> >
> > It's implemented in at least the fast models already, not checked
> > any of the other emulators, so there's some possibility of people
> > using it while developing other things and hopefully at least
> > some of the various CI systems will be including emulated
> > platforms with newer extensions in their coverage so might gain
> > some benefit from it.  Frankly the only reason I'm looking at
> > this at all is that I'd written patch 3 because I was getting fed
> > up with KASLR initialization being easily disabled when I was
> > trying to test E0PD on the models (especially before I added the
> > status print at boot to KASLR so this happened silently), having
> > this in mainline would've helped considerably when working on
> > that.
>
> I was thinking specifically about users on silicon rather than developers
> on simulators. (I could stick this on a branch for developers if necessary).
>
> > I don't see any downside to having the code in mainline as is,
> > even though it's not ideal it does make things better since if
> > for some reason anyone does end up running this code on a system
> > that has the feature they'll get at least some benefit from it
> > even if nothing else happens.  The bulk of the code isn't going
> > to change when the early init stuff gets improved and includes
> > tables like cpufeature.h that make it annoying to hold out of
> > tree, the bits that are going to change can just as well be
> > worked on incrementally as held out of tree entirely and having
> > the rest in means there's less friction doing that.
>
> The usual downside that comes from merging patches with promises of fixing
> them up later is that the motivating task gets marked as "done" somewhere,
> the developer gets given something else to do and the updates never
> materialise. That's not a dig at you; it's just the way these things tend
> to work (I've certainly been on both sides of that coin).
>
> If there was an urgency to this, I'd suggest merging a form of Richard's
> code, as it appears to solve the technical issue of credited entropy whilst
> leaving some room for subsequent cleanup. However, I think that makes it
> even less likely that anybody will come back to do the cleanup because the
> code will be perfectly functional, so I'd prefer to wait for a complete
> solution unless you think it's not achievable for 5.7.
>
> I'd also really like Ard's ack on anything relating to RNGs.
>

Patches #1 and #3 are fine with me, modulo the HWCAP bit which I don't
deeply care about.

But the way this patch works around our workaround for mismatched RNG
caps between cores doesn't make sense to me.
arch_get_random_seed_long() should just have some out of line __init
path that gets invoked only during early boot, exactly how we are
using it in patch #3 to seed KASLR, where we don't care about whether
or not other CPUs have the extension. (Note that rand_initialize() is
called very early, way before the point where we have to care about
being scheduled from a CPU with RNG to one without)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel