Re: Directions for arc4random

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

>>>>>> I would like to know if I should resubmit my last, very complex
>>>>>> approach, or if I should switch to the simple, lock-based approach.
>>>>>
>>>>> It was my backlog list for some time. The first problem I see is the 
>>>>> the huge size which makes reviewing quite complicate.
>>>>>
>>>>> The first step to try simplify patch submission is remove all x86
>>>>> optimizations for AES-NI/SSE2 as you already indicate in your initial
>>>>> submission.
>>>>
>>>> It's AES-NI and RDRAND, demonstrating the use of both CPU extension
>>>> hooks to verify that they are usable.  I think they do not substantially
>>>> complicate review of the rest of the patch.
>>>
>>> My point is the change is orthogonal to the arc4random and it could be
>>> send in a different patch.  The original patch is already quite
>>> complex and moving the arch-specific bits to a separate patch
>>> simplifies review on both changes.
>> 
>> Sure, but then you don't see how the arch hooks are to be used, and to
>> review those bits, you'll have to look at the arch patch anyway.
>
> But on generic version only the architecture would be reviewed and won't
> prematurely tied to an specific ABI. I will check how hard would be to
> support aarch64 on current proposal.

Okay.

>>>>> Another thing is I tend to agree with Rich, the lock-free fork/clone
>>>>> detection is quite complicated and possible troublesome for maintainability. 
>>>>> Since we have a long-standing issue of a no async-signal-safe fork 
>>>>> implementation (BZ#4737) and fork implementation still contain some 
>>>>> lock contention on multithread fork (due atfork list handling, libio 
>>>>> internal list, and malloc) I think such lock-free complexity would not 
>>>>> yield much gain.
>>>>
>>>> The lock is eliminated not from fork (which doesn't have to care if we
>>>> are willing to reseed in the new process), but from regular arc4random
>>>> calls.
>>>
>>> Right, my confusion here, this is indeed another topic.  
>>>
>>> Checking on other arc4random implementation (OpenBSD one which seems to 
>>> support Linux), its fork handling seems sketchy: it uses a atfork handler 
>>> and detect a fork by checking a global variable set by the handler plus 
>>> against current pid (get with getpid).  It incurs in an extra syscall, it 
>>> does not handle pid wrap around, and requires a global lock over arc4random 
>>> on fork detect operation.
>> 
>> The OpenBSD implementation on OpenBSD uses minhert(2), which gives a
>> zeroed pool after fork, and it should therefore reseed in the new
>> process.  This is really subtle, a few comments in the code certainly
>> wouldn't hurt.
>
> Agree, although the fork handling scheme I described the is the one
> used on Linux (lib/libcrypto/arc4random/arc4random_linux.h and it does
> not support MADV_WIPEONFORK).

Correct, that code predates MADV_WIPEONFORK.

>>> Right, but if I understood correct, at least for our case glibc will only
>>> pay for the extra page if the program actually calls arc4random,
>>> right?
>> 
>> Yes, but I want to use arc4random to expand the initial AT_RANDOM cookie
>> into more bytes, for more ASLR for mapping shared objects, and for the
>> heap protector.  So all programs would use it, unless we add further
>> complication.
>
> Right, so AT_RANDOM cookie extension might be potentially used on dlopen
> and heap protector on malloc.  I am not sure if dlopen should be considered
> a scenario for push for an optimized arc4random, but malloc most likely is.
> Do you plan to push the heap protector as default feature or as interposing
> malloc for debugging (I am not sure if you already discussed it on mailing
> list)?

I still plan to push the heap protector as a default feature, unless the
performance overhead proves prohibitive.

Thanks,
Florian