Re: [RFC][PATCH] Make cryptoapi non-optional?

[Matt Mackall <mpm@selenic.com>]

On Fri, Aug 15, 2003 at 10:03:24PM -0700, Andrew Morton wrote:
> Matt Mackall <mpm@selenic.com> wrote:
> >
> > a) extract_entropy (pool->lock)
> > 
> > ...
> > 
> >  [There was also a cute sleeping problem here with random_read.
> >  random_read used old-style open-coded sleeping, marked itself
> >  TASK_INTERRUPTIBLE, then called extract_entropy, which would do a
> >  conditional reschedule, and fall asleep until the next wake up,
> >  despite having enough entropy to fulfill the request.]
> 
> OK.  There was no description of all this in your initial patch, so I don't
> know which patches solve these problems.

Almost all of this was in the first locking patch, which was probably
shorter than this description.

> > ...
> >  [By the way, whoever did the workqueue conversion for 2.5 changed this
> >  code to wakeup the processing worker when the sample pool was half
> >  full rather on every sample but got the test not quite right.
> 
> That would have been me.  The context switch rate due to
> add_disk_randomness() was much too high (1000/sec iirc), so that was an
> attempt to reduce the wakeup rate.
> 
> What we really should try to do is to not pass the work to keventd at all:
> just do the entropy addition right there in interrupt context, with
> appropriate locking.

Agreed, will add to my list.

> > f) change_poolsize (queued for resend)
> 
> Queued for resend because I just didn't know what to do with the patches.
> 
> I do not understand the random driver, and judging from the past couple of
> days discussions I'm not likely to.  I don't know how to test it and I
> doubt if anyone else is testing it in sufficient depth.

Ok, well this piece remains oopsable by inspection, though it is root
only. See free_entropy_store:

        if (r->pool)
                kfree(r->pool);
        kfree(r);

..while r and r->pool are potentially being used concurrently.

> And, to add to my dilemma, the random driver is security-related and
> something which scary propellor-headed types get all emotional about :)
>
> So getting these changes in will be hard.  It would help a lot if they
> could be presented individually and that Ted, Jamie and/or other interested
> parties were to review and ack them.

Ok. I've significantly improved the instrumentation and will be happy
to post test cases along with my next fixes too. All the controversial
things are at the end of my queue, as well (the present controversy
was not about something I'd submitted for inclusion). 

If anyone's interested, I have a preliminary Debian package for NIST's
RNG statistical test suite too (though this is only good as a sanity
check).
 
> > g) urandom starves/races random (queued for resend)
> 
> hrm.  That's one which I actually understood.  There is yet hope.

I can probably break this into a bunch of pieces if the two its
already in don't seem obvious enough on their own.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon