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

Unfortunately, I mistakenly sent the patch chain in the previous email.

I have fixed it:

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

https://patchwork.sourceware.org/project/glibc/patch/[email protected]/

31.07.2026 16:30, Artem Proskurnev:
> 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
>>>
>>