Re: [PATCH v6 09/10] stdlib: Add TLS optimization to arc4random

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

> On 28 Jun 2022, at 09:19, Florian Weimer <fweimer@redhat.com> wrote:
> 
> * Adhemerval Zanella via Libc-alpha:
> 
>> The arc4random state is moved to TCB, so there is no allocation
>> failure. It adds about 592 bytes struct pthread.
>> 
>> Now that the state is thread private within a shared struct, the
>> MADV_WIPEONFORK usage is removed. The cipher state reset is done
>> solely by the atfork internal handler.
>> 
>> The state is also cleared on thread exit iff it was initialized (so if
>> arc4random is not called it is not touched).
>> 
>> Although it is lock-free, arc4random is still not async-signal-safe
>> (the per thread state is not updated
>> 
>> On x86_64 using AVX2 it shows a slight better performance:
> 
> Should this be merged with the first patch?

I only split it to make it easier to drop if the strategy would not be
desirable, but I think make sense to merge.

> 
> Can we allocate the memory dynamically, and fall back to direct kernel
> randomness based on allocation failure?
> 
> Those 512 bytes of TCB space could be better used for increasing the
> static TLS reserve, I think.

Alright, it should be feasible since it already has to handle getentropy
failure on normal code path.


>> diff --git a/sysdeps/unix/sysv/linux/tls-internal.c b/sysdeps/unix/sysv/linux/tls-internal.c
>> index 6e25b021ab..75bc4b3b48 100644
>> --- a/sysdeps/unix/sysv/linux/tls-internal.c
>> +++ b/sysdeps/unix/sysv/linux/tls-internal.c
>> @@ -1 +1,32 @@
> 
>> +void
>> +__glibc_tls_internal_free (void)
>> +{
>> + struct pthread *self = THREAD_SELF;
>> + free (self->tls_state.strsignal_buf);
>> + free (self->tls_state.strerror_l_buf);
>> +
>> + if (self->tls_state.rnd_state.count != -1)
>> + /* Clear any lingering random state prior so if the thread stack is
>> + cached it won't leak any data. */
>> + memset (&self->tls_state.rnd_state, 0, sizeof self->tls_state.rnd_state);
>> +}
> 
> I think this should be explicit_bzero (currently for documentation
> purposes only).

Indeed, it would be good to clear it even when it would be dynamically
allocated.

> 
> Thanks,
> Florian