Re: fixing dynamic tls races (BZ 19329 and friends)

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 12/24/2020 16:55, Szabolcs Nagy via Libc-alpha wrote:
> i tried to fix BZ 19329 4 years ago without changing
> the design, just introducing atomics where necessary.
> the outcome is a bit hairy and does not solve the
> dlclose problem (BZ 27111). (one issue with this is
> that all accesses to the shared state has to be atomic
> even the ones that are under locks, but sometimes
> those can be relaxed atomic since there is no further

this likely needs to wait after the release, but to
avoid this fix to blow up in size i'd like to propose
a change to the glibc concurrency rules:

https://sourceware.org/glibc/wiki/Concurrency

currently it says:

- "All accesses to an object should use atomics if
  there is at least one atomic access."

- "Document at least [..] Why a certain memory order
  (MO) is sufficient. This especially applies to
  relaxed MO: why isn't a stronger MO required?"

i think we should allow non-racing memory accesses
to be non-atomic even if the same object may be
accessed atomically at some point. otherwise the
relaxed mo atomic annotations would make the dynamic
linker code harder to read and reason about.

=== c/c++ vs glibc memory model:

in c/c++ atomics is decided based on the object type.
(this allows different representation and alignment
for atomic and non-atomic types e.g. in case atomics
is implemented via locks.) as a consequence atomic
and non-atomic accesses cannot be mixed to an object.

glibc does not use atomic types, atomic is per access
(since there are abi types like pthread_spinlock_t
that are non-atomic historically but must be accessed
atomically). glibc only requires lock free atomic
access to naturally aligned int/unsigned/pointer.
this allows mixing atomic and non-atomic accesses.

=== problematic case:

a common pattern in the dynamic tls handling code is
to use shared variables as follows

(LR) many read accesses behind locks
(LW) few key write accesses behind locks
(NR) few key read accesses without locks

in such design (LW) and (NR) can be in a data-race
and need atomics for synchronization (acquire/release
mo pairing is usually enough to order the surrounding
memory accesses, but this requires careful analysis
on a case-by-case basis). however (LR) cannot be in a
data-race, c/c++ would allow non-atomic access, other
than the type based atomic requirement. in conforming
c/c++ code (LR) would typically be relaxed mo. i find
this problematic and something we can avoid in glibc:
relaxed mo is usually a big red flag due to unclear
and complicated semantics. however non-racing access
such as (LR) is an easy to check case that is common
and can be non-atomic. if we were to turn (LR) to
relaxed mo then glibc will be littered with red flags
and it will be hard to see which are the real tricky
relaxed mo accesses.

reading the list of identified valid uses of relaxed
mo atomics

http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2020/p2055r0.pdf

it seems relaxed atomics can even introduce problems
compared to non-atomic access when it is used for a
non-racing access (see 2.1 there).

=== data point:

in the dynamic tls code at least these objects need
to be atomic:

  GL(dl_tls_generation)
  GL(dl_tls_max_dtv_idx)
  dtv_slotinfo_list->next
  dtv_slotinfo_list->slotinfo[i].gen
  dtv_slotinfo_list->slotinfo[i].map

there are about 100 memory accesses to these objects
most of which are (LR) type: so fixing BZ 19329 would
need to add close to 100 relaxed mo atomic loads with
explanation.

=== proposal:

my proposal is to allow non-racing accesses to be
non-atomic in glibc even if the same object needs
atomics elsewhere (this seems to be standard practice
at least in the linux kernel). i think this rule also
covers early, single-threaded initialization, but not
concurrent initialization (like lazy binding).

=== alternatives:

use non_atomic_load (&obj) annotations instead of
relaxed. (but even this is very intrusive.)

redesign the dynamic tls handling code. (this is
not obvious.)