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

Greetings, Carlos!

A complete solution divided into parts:

https://inbox.sourceware.org/libc-alpha/[email protected]/

https://inbox.sourceware.org/libc-alpha/[email protected]/

https://inbox.sourceware.org/libc-alpha/[email protected]/

https://inbox.sourceware.org/libc-alpha/[email protected]/

21.07.2026 16:44, Carlos O'Donell:
> On 7/21/26 6:47 AM, Artem Proskurnev wrote:
>> 21.07.2026 00:43, Carlos O'Donell:
>>> (1) Solving the narrower problem as a short-term solution.
>>>
>>> Adhemerval's solution seems like a good immediate fix for the problem.
>>>
>>> Does it fix the issue?
>>>
>>> As a project lead I would like to see Bug 15686 fixed, but if it takes
>>> us time to get to that solution, we also want to resolve the problem
>>> that you're seeing in a timely fashion and are willing to accept a 
>>> short-term
>>> solutions to help you out.
>>>
>>> (2) Solving the architectural problem may take longer or require 
>>> more testing.
>>>
>>> Large changes like those which are being proposed here will take 
>>> time to review
>>> and check against existing application expectations.
>>>
>>> The implementation is constrained by more than just what the standards
>>> say, or the glibc manual says, it is constrained by expectations that
>>> applications may have created.
>>>
>>> It should not be this way, but it is, and we attempt to consider such
>>> constraints when making changes because we don't want to knowingly 
>>> break
>>> such applications unless there is a meaningful and valuable gain.
>>>
>>> We might be able to run with this change downstream in Fedora and use
>>> our build infrastructure to do A/B build testing of the distribution 
>>> to see if
>>> something comes out of the larger scale testing.
>>>
>>> I'm excited to see the results of ROSA Labs running with these 
>>> changes in
>>> production and reporting back the results.
>>>
>>> (3) POSIX and glibc say nothing about this today --- making this a 
>>> hard problem.
>>>
>>> The problem *is* that we say nothing about what is and is not 
>>> allowed and have
>>> permitted the implementation to grow to the point that users expect 
>>> a completely
>>> flexible asynchronous processing of dependencies across multiple 
>>> threads
>>> allowing calls to all library functionality.
>>>
>>> For example, some users expect a malloc interposer can call dlopen, 
>>> but this
>>> clearly has to have some constraints. If dlopen needs malloc then it 
>>> doesn't
>>> work as the solution has a dependency cycle (discussed previously on 
>>> list as a
>>> new form of safety e.g. "synchronous reetrancy").
>>>
>>> Since we have never said anything before about this, as Adhemerval 
>>> notes, this
>>> area is going to possibly cause problems with existing applications, 
>>> and for
>>> which this patch provides a tunable.
>>>
>>> It would be better if instead of using a tunable we spent more time 
>>> evaluating
>>> the problem space and a solution, hence I think Adhemerval's 
>>> suggestion to use
>>> an intermediate solution. This intermediate solution is not a final 
>>> solution,
>>> but an option to resolve the short-term problem while we take more 
>>> time to
>>> evaluate a full solution.
>>>
>>> In truth we say a few things about this in the glibc manual in the 
>>> Dynamic Linker
>>> chapter, but only to warn against having any expectations :-)
>>>
>>> (4) A full solution requires understanding what we're asking --- and 
>>> possibly tooling.
>>>
>>> Are we asking that all foreign function callbacks can call into the 
>>> library safely?
>>>
>>> Are we only asking that all foreign function callbacks for library 
>>> initializers and
>>> finalizers be able to call back into the library safely?
>>>
>>> Constructors and destructors are foreign function calls made during 
>>> library
>>> initialization and finalization, and as such we have never discussed 
>>> what is and
>>> isn't permitted to be called in those functions.
>>>
>>> These foreign functions are not occurring in the normal context of 
>>> the application
>>> runtime, nor do we provide ways for them to know what *other* 
>>> non-libc functionality
>>> can be safely accessed and if it will ever change (based on load 
>>> order).
>>>
>>> Is there any way we can add tooling or _dl_debug_printf options to 
>>> detect problems?
>>>
>>> Example: lari: 
>>> https://docs.oracle.com/cd/E88353_01/html/E37839/lari-1.html
>>>
>>> Should we add the option to randomly order dependencies whose 
>>> relative order shouldn't
>>> matter? To shake out problems in development?
>>>
>>> I think a solution to this problem is going to take time, 
>>> particularly to evaluate
>>> the effect on existing applications and runtimes.
>>>
>>> Thoughts on (1), (2), (3) or (4)?
>>>
>> Thanks for the thoughtful response.  Replying to your four points in
>> the order that I think moves the discussion forward fastest: (1)
>> first because it is the easiest to settle factually, (3) next
>> because it is the most actionable, then (2) and (4).
>>
>> == (1) Does Adhemerval's fix solve the reported issue?
>>
>> No.  I A/B tested it before sending v3, and the result is in my reply
>> to Adhemerval earlier in this thread.  Repeating the headline for
>> directness:
>>
>> azanella/bz15686 (9039a9cb0f)  DEADLOCK  exit 124
>> v3 (release-dl-load-lock...)   PASS      exit 0, PNG 256x256
>>
>> The blocking site is getgrouplist -> __nss_lookup_function ->
>> __nss_module_load -> __libc_dlopen_mode -> _dl_open -> dl_load_lock,
>> not __cxa_thread_atexit_impl.  Adhemerval's patch removes one
>> acquisition site but leaves the rest of the dl_load_lock surface
>> intact, and the glycin reproducer fires through one of the sites
>> that is left intact.
>>
>> I would still like to see Adhemerval's patch merged independently.
>> It is a strict improvement on the path it touches, the cleanup of
>> the per-thread dso_symbol_cache/lm_cache is real, and it does not
>> conflict with v3.  My position is "both, not either-or".
>
> Thanks for confirming that.
>
> So the short-term fix doesn't solve your problem.
>
> That makes the issue more pressing to review and discuss.
>
>> == (3) POSIX is silent, tunable is not ideal
>>
>> I agree the tunable is not ideal, and your concern is well-founded:
>> a runtime knob becomes permanent API surface the moment it ships,
>> and we should not enshrine a non-standard invariant just to hedge
>> against an unknown regression.  Two options, in order of my
>> preference:
>>
>> (a) Keep the tunable, but ship it undocumented and explicitly marked
>> deprecated from day one.  The manual would say nothing about it;
>> the tunables list would carry a one-line entry flagged "deprecated,
>> escape hatch for unforeseen regressions, scheduled for removal".
>> If no regression surfaces within one release cycle, it gets removed.
>> If one does, we have a concrete report to discuss rather than a
>> hypothetical.  This keeps the escape hatch while making it clear
>> that the v3 default is the contract, not the fallback.
>>
>> (b) Remove the tunable entirely for v4.  Default behaviour is the v3
>> behaviour, period.  If a regression surfaces, we handle it the way
>> we handle any other regression -- a revert or a targeted fix in the
>> next release, not a runtime knob.  This is the cleanest position
>> but also the riskiest, because it removes the safety net for
>> downstreams that may not have a way to test before shipping.
>>
>> I prefer (a).  The cost of carrying an undocumented flag for one
>> release cycle is small, and the benefit is that if a regression
>> does surface we have a clean way to write "set this flag, verify
>> the regression goes away, then we know what we are fixing". Without
>> it, we are stuck arguing from backtraces.
>>
>> That said, if the consensus in this thread is (b), I will do that.
>> I can send a v4 with the tunable removed and the XFAIL test removed
>> or reworded accordingly.  Just say the word.
>
> Since you ask for guidance further down:
>
> * We should always document tunables.
>
> * We should not ship undocumented tunables.
>
> * Tunables are not ABI. We should remind downstreams.
>
> * Tunables should not change standards conforming behaviour (they 
> don't in this case).
>
> I think it's OK to keep the tunable.
>
> However, the fact that we need a tunable means we should be reviewing
> the solution space more carefully and thinking about the impact.
>
> For example how will developers know the crashing application can be
> fixed by using the tunable? This is why I asked the open question about
> _dl_debug_printf, tooling, and other means of observability. If we can
> add observability via LD_DEBUG=all or another means that supports the
> analysis then that would be beneficial.
>
> Lastly, we should consider that glibc uptake in downstream takes
> almost 1-2 years, and as such the tunable would have to remain in place
> probably for 4 releases before we see all the reports, resolve them
> and then remove the tunable.
>
> All of this is normal for glibc, and I just wanted to set the 
> expectation.
>
>> == (2) Architectural fix needs time, Fedora A/B testing
>>
>> I would welcome Fedora A/B testing, and I will help however I can.
>> The minimal reproducer attached to my earlier reply runs in under a
>> second on any system with libnss_systemd.so.2 active and glycin
>> installed.  If Fedora wants a heavier workload, the original report
>> is ROSA bug 21031 -- Codeblocks startup -- and that is a real
>> user-visible scenario that a distro-scale build test can exercise.
>>
>> For the ROSA side: Mikhail Novosyolov has confirmed earlier in this
>> thread that v3 fixes ROSA bug 21031 in our downstream testing. Our
>> plan is to ship v3 (with Adhemerval's lock-free
>> __cxa_thread_atexit_impl rebased on top) once the upstream design
>> is settled.
>
> Great. I'd like to hear how the deployment goes and if you see any
> issues.
>
>> On the application-expectations concern: the risk surface of v3 is
>> specifically an application that depends on the constructor of DSO A
>> always running before the constructor of DSO B when A and B are
>> loaded by independent dlopen calls in independent threads.  That is
>> the only ordering property v3 changes.  Constructors within a single
>> dlopen still run in dependency order; constructors of the executable
>> and its startup-time dependencies still run before main.  If Fedora
>> A/B testing turns up a regression, it is almost certainly of this
>> shape, and the discussion can focus on whether the application was
>> relying on undocumented behaviour, documented behaviour that we
>> missed, or whether v3 has a bug.
>
> Agreed.
>
>> == (4) What are we asking for, and tooling
>>
>> This is the question I have spent the most time thinking about.
>> Laying out what I believe v3 is and is not claiming, after
>> re-reading the patch.
>>
>> v3 makes one specific claim:
>>
>>    Constructors -- foreign function calls made during library
>>    initialisation -- may call any libc and dynamic-loader function
>>    that is safe to call from a regular non-signal-handler thread of
>>    the same process.
>
> OK.
>
>> That claim implies:
>>
>>    - A constructor may call dlsym, _dl_addr, _dl_find_dso_for_object,
>>      dlopen of a different DSO, dlclose of a different DSO, and the
>>      NSS-backed libc functions (getpwnam, getgrnam, getaddrinfo,
>>      getgrouplist, ...).  dl_iterate_phdr was already safe via
>>      dl_load_write_lock after BZ 28357 and is unaffected by v3.
>>    - A constructor may spawn threads, and those threads may call any
>>      of the above.
>>    - A constructor may call malloc, and a malloc interposer that is
>>      itself loaded via dlopen may in turn call any of the above from
>>      its own constructors.
>>
>> The claim does NOT cover:
>>
>>    - Destructors.  v3 deliberately scopes to the constructor path,
>>      which is where the actual reproducer fires.  Destructors run
>>      with dl_load_lock held inside _dl_close_worker just as
>>      constructors used to inside dl_open_worker, so the same
>>      theoretical deadlock shape exists for a destructor that calls
>>      dlopen, spawns a thread that hits NSS, etc.  In practice this
>>      does not seem to happen: destructors usually free resources
>>      rather than load new code or spawn threads, and the one
>>      recursive case that does come up (a destructor calling dlclose)
>>      is already handled separately via dl_close_state in dl-close.c.
>>      Extending v3 to also release dl_load_lock around _dl_call_fini
>>      would require the same state machine on the close path without
>>      a real-world reproducer justifying it.  If a destructor
>>      deadlock of this shape surfaces, it should be a follow-up
>>      patch with its own reproducer; v3 does not pretend to fix it.
>>
>>    - Synchronous loader-internal reentrancy -- the case you describe
>>      in (4) where the loader's own malloc calls during dlopen
>>      processing (outside the ctor window) route through an
>>      interposer that itself calls dlopen.  v3 does not address this;
>>      it remains the "synchronous reentrancy" problem.  Note that
>>      malloc calls from inside a constructor are a different matter
>>      and ARE covered, because the constructor runs with the lock
>>      released.
>
> Good. This is a *better* more narrowly scoped definition of the problem.
>
>> Two special cases that v3 does handle, mentioned because they are
>> easy to get wrong when reading the patch:
>>
>>    - Recursive dlopen of the same DSO from within its own
>>      constructor: detected via l_init_owner (same-TID check in
>>      call_init), returns immediately without waiting for itself.
>>      This is the l_init_owner field, not l_init_pending.
>
> This is correct (only for same-TID case).
>
>>    - Concurrent dlopen of an already-being-initialised DSO from a
>>      different thread: the late caller takes the already-loaded
>>      early-return path, observes l_init_pending or l_init_called
>>      set, and waits on l_init_once until the constructor finishes.
>>      This preserves the property that dlopen does not return before
>>      the constructor has run.
>
> This is correct.
>
>> == Fundamental limitation: ctor-spawned threads are not originally
>>     independent
>>
>> Beyond the NOT-covered list above, there is one deadlock shape
>> that no same-design patch can lift, and v3 does not claim to.
>>
>> A thread spawned from inside a DSO's constructor is not originally
>> independent -- its existence begins inside the ctor, and any work
>> it does that transitively depends on the ctor's DSO must wait for
>> the ctor to complete.  If a spawned worker calls dlopen on a DSO
>> that (directly or transitively) depends on the ctor's DSO, the
>> worker correctly blocks on l_init_once: unblocking it would expose
>> a half-constructed DSO to the new load.  That is DSO dependency
>> ordering, not lock contention, and the serialization is required
>> for correctness.
>
> Correct. This would be a compositional defect in the application.
> Tooling to detect this would be beneficial but not required.
>
>> What v3 does lift is the orthogonal case: the spawned worker (or
>> any other thread) doing dlopen of a DSO that has no dependency on
>> the ctor's DSO.  The glycin reproducer is exactly this shape --
>> the worker's getgrouplist call routes through NSS module loading,
>> which dlopens libnss_systemd.so.2, which does not depend on the
>> ctor's DSO.  v3 lets the worker proceed; pre-v3 it deadlocked on
>> dl_load_lock.
>
> Agreed.
>
>> So v3's actual claim, stated precisely, is: originally independent
>> threads can run independent dlopens concurrently, and a ctor (or a
>> thread it spawns) can do dlopen of an independent DSO without
>> serialising against the in-progress ctor.  It cannot untie work
>> that transitively depends on the ctor's DSO -- but no patch in
>> this design space can, because that serialization is what makes
>> concurrent dlopen correct.
>
> Agreed.
>
>> That answers your question "all callbacks, or just init/fini
>> callbacks?".  v3's claim is precisely about init callbacks
>> (constructors).  Fini callbacks (destructors) are not yet covered
>> by v3; see above.  Other foreign function callbacks (signal
>> handlers, malloc hooks, pthread destructors, atfork handlers) are
>> not in scope.
>
> Good.
>
>> I think stating this scope explicitly is more useful than leaving
>> it implicit.  If the project's position is that the scope should be
>> narrower (e.g. exclude some of the bullets above), or that v3
>> should not ship without also covering one of the NOT-covered
>> cases, that is a conversation worth having now rather than after
>> v3 ships.
>
> No, the scope as defined is sufficiently narrow.
>
> However, I would like to discuss destructors too, and if anything
> needs to be done there.
>
>> On tooling:
>>
>> - Random-order dependency init is orthogonal to v3 and would be a
>> useful shakedown tool.  I would support it as a separate patch
>> series, behind a tunable with the same "undocumented, deprecated,
>> removed after one cycle" caveat if it turns up no real-world
>> breakage.
>
> Agreed.
>
>> - An _dl_debug_printf option that logs every dl_load_lock
>> acquisition site with a backtrace would help diagnose residual
>> deadlocks of this shape.  I do not have a patch for this but I am
>> happy to write one if there is interest.
>
> I think we should add this as a 2/2 patch, which improves the
> observability.
>
> My question to you is: How does a system admin, or developer know
> to flip the tunable?
>
>> - lari-style tooling for "which DSO registered which constructor"
>> is useful but I think out of scope for this thread.
>
> Agreed. I just showed it as an example of observability improvements.
>
> I want us to think of "solutions" not just "patches."
>
> That is to say that solving this problem might require more than
> just patching.
>
>> Alexander Pevzner's reply on the same question is worth reading in
>> this light -- he makes the architectural argument independently
>> and reaches the same place: per-case patches do not fix the
>> underlying invariant, they just move manifestations around.
>
> Right.
>
>> == Next steps
>>
>> Concretely, what would help from the list:
>>
>> - A steer on (3): undocumented-and-deprecated vs. removed entirely.
>> Once I have that I can send v4.  If the answer takes time, that is
>> fine -- I would rather get it right than get it fast.
>
> I think we should keep the tunable, it should be documented, and the
> documentation should clearly state we intend to remove this when the
> ecosystem has been shown to work with the new implemetnation.
>
>> - A steer on (4): does the scope claim above match what the project
>> wants to guarantee, or is it too strong?  If too strong, which
>> bullet should I narrow?
>
> The scope makes sense, but I'm also concerned about destructors since
> both see this kind of problem. What prevents us from providing the same
> invariant in destructors?
>
>
>> Thanks,
>> Artem
>>
>