malloc: Use __getrandom_nocancel during tcache initiailization

Message ID 87fsigp7y9.fsf@oldenburg.str.redhat.com
State Committed
Commit 7187efd0aa270c83c428ea6cd0e1cffc34b41a74
Headers
Series malloc: Use __getrandom_nocancel during tcache initiailization |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Florian Weimer Aug. 1, 2022, 10:35 a.m. UTC
  Cancellation currently cannot happen at this point because dlopen
as used by the unwind link always performs additional allocations
for libgcc_s.so.1, even if it has been loaded already as a dependency
of the main executable.  But it seems prudent not to rely on this
quirk.

---
 malloc/malloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Adhemerval Zanella Netto Aug. 1, 2022, 1:42 p.m. UTC | #1
On 01/08/22 07:35, Florian Weimer via Libc-alpha wrote:
> Cancellation currently cannot happen at this point because dlopen
> as used by the unwind link always performs additional allocations
> for libgcc_s.so.1, even if it has been loaded already as a dependency
> of the main executable.  But it seems prudent not to rely on this
> quirk.
> 


LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  malloc/malloc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index bd3c76ed31..430d204156 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -254,6 +254,7 @@
>  /* For tcache double-free check.  */
>  #include <random-bits.h>
>  #include <sys/random.h>
> +#include <not-cancel.h>
>  
>  /*
>    Debugging:
> @@ -3153,7 +3154,7 @@ static uintptr_t tcache_key;
>  static void
>  tcache_key_initialize (void)
>  {
> -  if (__getrandom (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
> +  if (__getrandom_nocancel (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
>        != sizeof (tcache_key))
>      {
>        tcache_key = random_bits ();
>
  
Yann Droneaud Aug. 2, 2022, 9:04 a.m. UTC | #2
Hi,

Le 01/08/2022 à 12:35, Florian Weimer via Libc-alpha a écrit :
> Cancellation currently cannot happen at this point because dlopen
> as used by the unwind link always performs additional allocations
> for libgcc_s.so.1, even if it has been loaded already as a dependency
> of the main executable.  But it seems prudent not to rely on this
> quirk.
>
> ---
>   malloc/malloc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index bd3c76ed31..430d204156 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -254,6 +254,7 @@
>   /* For tcache double-free check.  */
>   #include <random-bits.h>
>   #include <sys/random.h>
> +#include <not-cancel.h>
>   
>   /*
>     Debugging:
> @@ -3153,7 +3154,7 @@ static uintptr_t tcache_key;
>   static void
>   tcache_key_initialize (void)
>   {
> -  if (__getrandom (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
> +  if (__getrandom_nocancel (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)


Is this a place where arc4random() could be used in the future ?

aka. is there a policy on using arc4random() instead of getrandom() in 
the library ?
  
Florian Weimer Aug. 2, 2022, 9:44 a.m. UTC | #3
* Yann Droneaud:

> Hi,
>
> Le 01/08/2022 à 12:35, Florian Weimer via Libc-alpha a écrit :
>> Cancellation currently cannot happen at this point because dlopen
>> as used by the unwind link always performs additional allocations
>> for libgcc_s.so.1, even if it has been loaded already as a dependency
>> of the main executable.  But it seems prudent not to rely on this
>> quirk.
>>
>> ---
>>   malloc/malloc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index bd3c76ed31..430d204156 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -254,6 +254,7 @@
>>   /* For tcache double-free check.  */
>>   #include <random-bits.h>
>>   #include <sys/random.h>
>> +#include <not-cancel.h>
>>     /*
>>     Debugging:
>> @@ -3153,7 +3154,7 @@ static uintptr_t tcache_key;
>>   static void
>>   tcache_key_initialize (void)
>>   {
>> -  if (__getrandom (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
>> +  if (__getrandom_nocancel (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
>
>
> Is this a place where arc4random() could be used in the future ?
>
> aka. is there a policy on using arc4random() instead of getrandom() in
> the library ?

Currently there is not much of a difference between arc4random_buf and
getrandom on current kernels.  We'll see where getrandom is heading (and
if vDSO acceleration will end up in the kernel), and if it will involve
malloc in some way.  If it does, we can't use arc4random from malloc, we
have to use the system call.

Thanks,
Florian
  
Cristian Rodríguez Aug. 3, 2022, 1:50 p.m. UTC | #4
On Tue, Aug 2, 2022 at 5:44 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:

> Currently there is not much of a difference between arc4random_buf and
> getrandom on current kernels.  We'll see where getrandom is heading (and
> if vDSO acceleration will end up in the kernel), and if it will involve
> malloc in some way.

The current experimental interface between vdso an userspace does not
involve malloc, state has to be allocated by calling
void *getrandom_alloc([inout] size_t *num, [out] size_t *size_per_each);
  

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index bd3c76ed31..430d204156 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -254,6 +254,7 @@ 
 /* For tcache double-free check.  */
 #include <random-bits.h>
 #include <sys/random.h>
+#include <not-cancel.h>
 
 /*
   Debugging:
@@ -3153,7 +3154,7 @@  static uintptr_t tcache_key;
 static void
 tcache_key_initialize (void)
 {
-  if (__getrandom (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
+  if (__getrandom_nocancel (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK)
       != sizeof (tcache_key))
     {
       tcache_key = random_bits ();