Re: [PATCH] Async signal safe TLS accesses

[OndÅej BÃlka <neleai at seznam dot cz>]

On Thu, Dec 05, 2013 at 12:01:11PM -0800, Andrew Hunter wrote:
>     TLS accesses from initial-exec variables are async-signal-safe.  Even
>     dynamic-type accesses from shared objects loaded by ld.so at startup
>     are.  But dynamic accesses from dlopen()ed objects are not, which
>     means a lot of trouble for any sort of per-thread state we want to
>     use from signal handlers since we can't rely on always having
>     initial-exec.  Make all TLS access always signal safe.
> 
>     Doing this has a few components to it:
> 
>      * We introduce a set of symbols __signal_safe_{malloc,free,memalign,&c}.
>        They do what it says on the box, but guarantee async-signal-safety.
>        We provide a minimal mmap-based implementation in ld.so.  (This may
>        prove useful elsewhere in libc.)
> 
>      * We use these throughout dl-tls.c in paths reachable from tls_get_addr
>        (and, importantly, for allocations on other paths that might be
>        freed/realloced from tls_get_addr.)
> 
>      * tls_get_addr synchronizes with dlopen() by dl_load_lock; this lock
>        is reentrant, but not, alas, signal safe.  Replace this with simple
>        CAS based synchronization.
> 
>      * Use signal masking in the slow path of tls_get_addr to prevent
>        reentrant TLS initialization.  The most complicated part here is
>        ensuring a dlopen which forces static TLS (and thus updates all
>        threads' DTVs) does not interfere with this.
> 
These components make patch needlessly bigger than it should be.

There are at least 4 parts that could be checked out independently.

1. Factoring _dl_clear_dtv out, this was already reviewed please post 
separate patch that could be checked in. 
Function is used once so why you just don't add it as static function?

2. Adding _dl_mask_all_signals / _dl_unmask_all_signals. These should
also come as separate patch. Also as these are useful at their own we
could drop _dl prefix and use them also in libc.

3. Changing to signal safe allocator, Again actual allocator is
implementation detail that will be changed but we need some api now, so
send patch with it.

4. Using atomics instead of locking, first three changes were mechanical
and should be reviewed pretty quickly, this is difficult part of patch.


Also I found following nits:

> +
> +  size_t pg = GLRO (dl_pagesize);
> +  size_t padded_size;
> +  if (boundary <= pg)
> +    {
> +      /* We'll get a pointer certainly aligned to boundary, so just
> +	 add one more boundary-sized chunk to hold the header.  */
> +      padded_size = roundup (size, boundary) + boundary;
> +    }
> +  else
> +    {
> +      /* If we want K pages aligned to a J-page boundary, K+J+1 pages
> +	 contains at least one such region that isn't directly at the start
> +	 (so we can place the header.)	This is wasteful, but you're the one
> +	 who wanted 64K-aligned TLS.  */
> +      padded_size = roundup (size, pg) + boundary + pg;
> +    }
> +
Is special casing here really needed?


snip

> +      /* busywork */
> +      free (malloc (128));

that gets optimized away.

snip

> +     pthread_sigmask in that we do not mask internal signals used for
> +     cancellation and setxid handling. This disables asyncrhonous

s/asynchronous