[Bug libc/18034] New: [aarch64] Lazy TLSDESC relocation has data race

[Bug libc/18034] New: [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

            Bug ID: 18034
           Summary: [aarch64] Lazy TLSDESC relocation has data race
           Product: glibc
           Version: 2.21
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: nszabolcs at gmail dot com
                CC: drepper.fsp at gmail dot com

With lazy TLSDESC relocation the first TLS access writes memory: the TLS
descriptor function pointer (td->entry) and its argument (td->arg), but these
writes are not synchronized with concurrent reader threads. Some threads can
observe the new entry, but not the new arg so wrong TLS offset is used by the
TLS access. All the descriptor functions must have barriers or other load-load
ordering guarantees before they access the TLSDESC argument in case of lazy
initialization.

The race is easy to trigger using a shared object that has a lot of static
TLSDESC relocations and then accessing the TLS objects from several threads.
(The bug was found by running glibc intl/tst-gettext6 test where threads access
libc internal static TLS at thread exit.)

This issue affects multi-threaded programs that access TLS through TLSDESC in a
shared object loaded with lazy binding on any architecture with "weak memory
model" (currently aarch64 and 32bit arm with -mtls-dialect=gnu2).

Lazy binding and TLSDESC are the default on aarch64 and libc.so itself does TLS
access, so essentially any multi-threaded program may crash at concurrent TLS
access (which may happen in glibc at thread exit).

(I reported this against aarch64 only because it is more critical there and the
32bit case will be easier to deal with separately once aarch64 is fixed.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

--- Comment #1 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
Created attachment 8146
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8146&action=edit
module with static tls for test case

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

--- Comment #2 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
Created attachment 8147
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8147&action=edit
main code of test case

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

--- Comment #3 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
Created attachment 8148
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8148&action=edit
shell script to build the test

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

--- Comment #4 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
patch and more discussion at
https://sourceware.org/ml/libc-alpha/2015-04/msg00266.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

--- Comment #5 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  c71c89e5c72baf43fd44d08dda8ab846eec5b1d6 (commit)
       via  08325735c2efb0257b8c07ac0ff91e44c27ecbf8 (commit)
      from  2a8c2c7b335ed07f63c246077fa672d8eaed23e4 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c71c89e5c72baf43fd44d08dda8ab846eec5b1d6

commit c71c89e5c72baf43fd44d08dda8ab846eec5b1d6
Author: Szabolcs Nagy <nsz@port70.net>
Date:   Wed Jun 17 12:44:53 2015 +0100

    [AArch64] Fix cfi_adjust_cfa_offset usage in dl-tlsdesc.S

    Some of the cfi annotations used incorrect sign.

        * sysdeps/aarch64/dl-tlsdesc.S (_dl_tlsdesc_return_lazy): Fix
        cfi_adjust_cfa_offset argument.
        (_dl_tlsdesc_undefweak, _dl_tlsdesc_dynamic): Likewise.
        (_dl_tlsdesc_resolve_rela, _dl_tlsdesc_resolve_hold): Likewise.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=08325735c2efb0257b8c07ac0ff91e44c27ecbf8

commit 08325735c2efb0257b8c07ac0ff91e44c27ecbf8
Author: Szabolcs Nagy <nsz@port70.net>
Date:   Wed Jun 17 12:37:49 2015 +0100

    [BZ 18034][AArch64] Lazy TLSDESC relocation data race fix

    Lazy TLSDESC initialization needs to be synchronized with concurrent TLS
    accesses.  The TLS descriptor contains a function pointer (entry) and an
    argument that is accessed from the entry function.  With lazy
initialization
    the first call to the entry function updates the entry and the argument to
    their final value.  A final entry function must make sure that it accesses
an
    initialized argument, this needs synchronization on systems with weak
memory
    ordering otherwise the writes of the first call can be observed out of
order.

    There are at least two issues with the current code:

    tlsdesc.c (i386, x86_64, arm, aarch64) uses volatile memory accesses on the
    write side (in the initial entry function) instead of C11 atomics.

    And on systems with weak memory ordering (arm, aarch64) the read side
    synchronization is missing from the final entry functions (dl-tlsdesc.S).

    This patch only deals with aarch64.

    * Write side:

    Volatile accesses were replaced with C11 relaxed atomics, and a release
    store was used for the initialization of entry so the read side can
    synchronize with it.

    * Read side:

    TLS access generated by the compiler and an entry function code is roughly

      ldr x1, [x0]    // load the entry
      blr x1          // call it

    entryfunc:
      ldr x0, [x0,#8] // load the arg
      ret

    Various alternatives were considered to force the ordering in the entry
    function between the two loads:

    (1) barrier

    entryfunc:
      dmb ishld
      ldr x0, [x0,#8]

    (2) address dependency (if the address of the second load depends on the
    result of the first one the ordering is guaranteed):

    entryfunc:
      ldr x1,[x0]
      and x1,x1,#8
      orr x1,x1,#8
      ldr x0,[x0,x1]

    (3) load-acquire (ARMv8 instruction that is ordered before subsequent
    loads and stores)

    entryfunc:
      ldar xzr,[x0]
      ldr x0,[x0,#8]

    Option (1) is the simplest but slowest (note: this runs at every TLS
    access), options (2) and (3) do one extra load from [x0] (same address
    loads are ordered so it happens-after the load on the call site),
    option (2) clobbers x1 which is problematic because existing gcc does
    not expect that, so approach (3) was chosen.

    A new _dl_tlsdesc_return_lazy entry function was introduced for lazily
    relocated static TLS, so non-lazy static TLS can avoid the synchronization
    cost.

        [BZ #18034]
        * sysdeps/aarch64/dl-tlsdesc.h (_dl_tlsdesc_return_lazy): Declare.
        * sysdeps/aarch64/dl-tlsdesc.S (_dl_tlsdesc_return_lazy): Define.
        (_dl_tlsdesc_undefweak): Guarantee TLSDESC entry and argument load-load
        ordering using ldar.
        (_dl_tlsdesc_dynamic): Likewise.
        (_dl_tlsdesc_return_lazy): Likewise.
        * sysdeps/aarch64/tlsdesc.c (_dl_tlsdesc_resolve_rela_fixup): Use
        relaxed atomics instead of volatile and synchronize with release store.
        (_dl_tlsdesc_resolve_hold_fixup): Use relaxed atomics instead of
        volatile.
        * elf/tlsdeschtab.h (_dl_tlsdesc_resolve_early_return_p): Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                    |   20 +++++++++++++++++
 NEWS                         |   14 ++++++------
 elf/tlsdeschtab.h            |    8 ++++--
 sysdeps/aarch64/dl-tlsdesc.S |   47 +++++++++++++++++++++++++++++++++++++----
 sysdeps/aarch64/dl-tlsdesc.h |    3 ++
 sysdeps/aarch64/tlsdesc.c    |   36 +++++++++++++++++++++----------
 6 files changed, 101 insertions(+), 27 deletions(-)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18034] [aarch64] Lazy TLSDESC relocation has data race

[nszabolcs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18034

Szabolcs Nagy <nszabolcs at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
commit 08325735c2efb0257b8c07ac0ff91e44c27ecbf8 fixed it for AArch64.

-- 
You are receiving this mail because:
You are on the CC list for the bug.