Re: the random driver

["Theodore Ts'o" <tytso@mit.edu>]

On Tue, Nov 19, 2002 at 11:46:53PM -0800, Andrew Morton wrote:
> a) It's racy.  The head and tail pointers have no SMP protection
>    and a race will cause it to dump 128 already-processed items
>    back into the entropy pool.

Yeah, that's a real problem.  The random driver was never adequately
or locked for SMP case.  We also have a problem on the output side;
two processes that read from /dev/random at the same time can get the
exact same value.  This is **bad**, especially if it is being used for
UUID generation or for session key generation.

The output side SMP locking is on my todo queue, but this week I'm in
Atlanta dealing with IPSE Cat the the IETF meeting....  when I get
back in Boston next week, I'll look at fixing this, but if someone
wants to beat me to it, feel free....

> b) It's weird.  What's up with this?
> 
>         batch_entropy_pool[2*batch_head] = a;
>         batch_entropy_pool[(2*batch_head) + 1] = b;
> 
>    It should be an array of 2-element structures.

The entropy returned by the drivers is essentially just an arbitrary
64 bit value.  It's treated as two 32 bit values so that we don't lose
horribly given GCC's pathetic 64-bit code generator for the ia32
platform. 

> d) It's punting work up to process context which could be performed
>    right there in interrupt context.

The idea was to trying to pacify the soft realtime nazi's that are
stressing out over every single microsecond of interrupt latency.
Realistically, it's about dozen memory memory cache misses, so it's
not *that* bad.  Originally though the batched work was being done in
a bottom-half handler, so there wasn't a process context switch
overhead.  So perhaps we should rethink the design decision of
deffering the work in the interests of reducing interrupt latency.

> My suggestion, if anyone cares, is to convert the entropy pool
> into smaller per-cpu buffers, protected by local_irq_save() only.
> This way the global lock (which isn't there yet) only needs to
> be taken when a CPU is actually dumping its buffer.

Yeah, that's probably what we should do.

						- Ted