Re: Patch: CPU RNG framework with stub x86 implementation

[Thor Lancelot Simon <tls%panix.com@localhost>]

On Mon, Jan 11, 2016 at 06:36:56AM +0000, Taylor R Campbell wrote:
> 
> All the conditionally initialized softint business keeps tripping me
> up.  It seems that we ought not to install the cpurng source until we
> have established the softint.  Can we not softint_establish before we
> attach the cpurng rndsource?

I got tripped up by the initialization happening in the backwards order
from what I'd remembered, and I left the conditional in from an abundance
of caution.  It can probably go.  I'll test.

> Also, why does this need to happen in softint context anyway?  Isn't
> the point of the CPU RNG that it is super-cheap and never requires
> asynchronous access?  The previous patch didn't do that -- why do it
> now?

It's the locking used to protect the recordkeeping and the pool add.
The same issue that afflicts the rpi RNG and a number of others, I
think.  I'm not happy about it.  This is what I meant when I said our
existing source framework is awfully heavy for this kind of source.
My very first try at this -- for all its flaws -- did avoid these
issues and was much more efficient.  At a price, of course.

We can deadlock as things request entropy at boot and we're called
repeatedly to feed the pool.  The calls to get entropy come from
somewhere in the pool machinery that's got the pool lock held, and
try to take the source's lock that protects its queue and recorkeeping;
the source's add function tries to take the pool lock, and... boom.

Entertainingly (ha!) I didn't notice this initially because I was
testing with a VM with 4 CPUs, and in that case we don't deadlock.  When
I reduced the CPU count to 2, I found the code deadlocked every time.
I know (and I read your comments below as well) that you reorganized
the code so it should not deadlock like this, but I'm still seeing it
happen.

To eliminate the deadlock without deferring from the context in which
we call the get function from the pool code, one lock or the other's
got to go, I think.  The source lock's the obvious target.

The queues feeding the pool could be replaced with pcq, which would
avoid one of the datastructures requiring protection by the source's
lock.  The recordkeeping could be done with atomics (of course, the
resulting locked bus transactions may cost far more than the actual
RNG instruction, but... sigh).  That leaves the entropy estimators.
The obvious solution is for sources that don't use them to not have
them.  There's also the buffer that accumulates bits for the statistical
test, but we can probably come up with a hack for that.

That's a bunch of work, though.

> I'm asking because I would like to know whether you intend for it to
> generically makes sense to attach one cpurng(9) rndsource per CPU.  If
> so, then maybe we should just do that, and in the callback, skip if
> it's not our CPU, rather than using locks.

It would; I didn't bother because I figured there was no reason it
mattered which CPU we were on; is it good enough to skip, I wonder,
or do we need to disable interrupts and/or preemption in the body
of the function as well?  The VIA source already does (not super
happy about that).

Thanks for the close reading!

-- 
  Thor Lancelot Simon	                                     tls%panix.com@localhost

  "We cannot usually in social life pursue a single value or a single moral
   aim, untroubled by the need to compromise with others."      - H.L.A. Hart