Re: [PATCH v3] elf: Release dl_load_lock before running dlopen constructors (BZ 15686)

On 16/07/26 13:04, [email protected] wrote:
> From: Artem Proskurnev <[email protected]>
> 
> This addresses one instance of the long-standing class of deadlocks
> described in BZ #15686: ELF constructors and destructors invoked by
> the dynamic loader run with dl_load_lock held, so any code path in
> those constructors that itself needs dl_load_lock deadlocks.
> 
> dl_open_worker holds dl_load_lock across the entire _dl_open call,
> including the call to call_dl_init that runs the new objects'
> constructors.  If one of those constructors spawns a thread whose
> first access to a thread_local object triggers
> __cxa_thread_atexit_impl, the new thread blocks trying to acquire
> dl_load_lock -- which is held by the dlopen thread -- deadlocking
> the process.  The same deadlock arises when the spawned thread calls
> a function that triggers NSS module loading through _dl_open, or any
> other code path that needs dl_load_lock.
> 
> The blocking site is __cxa_thread_atexit_impl at
> stdlib/cxa_thread_atexit_impl.c.  BZ #28357 was a partial fix for
> the wider BZ #15686 problem: it moved dl_open_worker_begin and
> _dl_close_worker to the finer-grained dl_load_tls_lock (commit
> 024a7640ab) and used that new lock in pthread_create and
> __tls_get_addr.  __cxa_thread_atexit_impl, however, still takes
> dl_load_lock to protect its DSO lookup (_dl_find_dso_for_object)
> against a racing dlclose, and that path is not covered by the
> BZ #28357 fix.  Moving it to dl_load_tls_lock is not straightforward
> because _dl_find_dso_for_object walks _ns_loaded, which is protected
> by dl_load_lock rather than dl_load_tls_lock.
> 
This is a *very* complex solution, that refactor a lot of internal code, and
change external lock assumptions.

I really dislike the glibc.rtld.strict_init_order tunable as an escape hatch
(even though we did add one for sorting dependencies).  And the XFAIL test
documenting that constructors of independent DSOs can now interleave means 
this is a potential semantic change that can regress of a lot of usercases.

From the the reported deadlock, it doesn't really require releasing 
dl_load_lock around constructors at all. The actual blocking site is
__cxa_thread_atexit_impl, which takes dl_load_lock only to look up the 
caller's link map. Replacing that lookup with the lock-free _dl_find_object 
removes the lock acquisition entirely.

So here is alternative solution [1], that follows this suit. I could not 
check with codeblock because I could not reproduce the issue with the nighly 
build on x86_64, but I have added some tests from your patch as regression 
testcase (while fixing some UB on some).

I checked on ARM and x86 without any regressions or surprises, but it would
be good to have more testing.

This is *not* a full fix for BZ#15686, the lock/release around ctors would
require a more complex solution (as you have noticed). But there is the
also the question whether we really need this to provide it to make full 
C-runtime support within constructors. 

PS: I might only reply this thread in two weeks.

[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/bz15686