Re: [PATCH] Async signal safe TLS accesses

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/06/2013 01:15 PM, Alexandre Oliva wrote:
> On Dec  5, 2013, Andrew Hunter <ahh@google.com> wrote:
> 
>> On Thu, Nov 28, 2013 at 5:03 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
>>> The lock is recursive, but I don't see anything in the patch besides
>>> the CAS loops that avoids inconsistent state, particularly in the
>>> case of lazy TLS relocation (introduced with GNU2 TLS).  Was that not
>>> taken into account?
> 
>> glibc recursive locks aren't async-signal-safe.  We avoid problems
>> with reentrancy via dl_mask_all_signals.
> 
> But that doesn't avoid *concurrency* problems, does it?
> 
> Besides, GNU2 TLS provides for lazy TLS relocation (as mentioned above),
> and that most certainly takes the DL lock.  I don't see anything in the
> patch that would avoid signal-safety problems in that path.
> 
> GNU2 will probably be made default on x86 and ARM eventually, and it
> already is the default on a number of other recent architectures;
> AArch64 is one of them IIRC.  So, if this issue was not addressed by
> this patch, I would recommend being careful with the messaging: it may
> fail to make TLS AS-safe in general.  Some qualifier would be necessary
> to make the statement correct.

Alex is correct here.

I had not considered TLS descriptors in my initial review.

Echoing Alex's comment though in that this is not yet the default,
you have to explicitly request `-mtls-dialect=gnu2', but the benefits
of the descriptor are make it worth it.

I just looked at the TLSDESC code, which applies to x86, x86_64, arm,
and aarch64, and it looks like you will take dl_load_lock and that
could cause a deadlock in the signal handler if the TLS variable also
uses TLSDESC.

e.g.
DSO function
-> Signal handler
 -> var@plt
  -> _dl_tlsdesc_resolve_rela_fixup (sysdeps/x86_64/tlsdesc.c)
   -> _dl_tlsdesc_resolve_early_return_p (elf/tlsdeschtab.h)
    -> __rtld_lock_lock_recursive (GL(dl_load_lock));

I think it's entirely possible to rewrite this to be AS-safe.

I'm not requiring it, but I agree again with Alex that it must be
explicitly stated that GNU2 dialect TLS is not yet signal-safe with
your patch.

I assume __rtld_lock_lock_recursive is not AS-safe because there
a space of time between taking the lock and recording the ownership
where if the signal arrived would cause the code to attempt to
retake the lock and deadlock.

Cheers,
Carlos.