Hi Mark, On Fri, Oct 30, 2026 at 11:21 AM Mark Wielaard <[email protected]> wrote: > > Hi Aaron, > > On Fri, 2026-10-30 at 01:16 -0400, Aaron Merey wrote: > > Add load_lock to Dwfl for serializing module loading, including loading > > of module's elf, dwarf, symbols as well as performing relocations. > > > > load_lock is used only for lazy loading of these components. Hot paths > > are kept lock-free using atomics. New Dwfl_Module fields elf_loaded, > > dwarf_loaded and symbols_loaded have been added. These are set to > > true when lazy loading of elf, dwarf and symbols is completed. They are > > also set to true when initialization has been attempted and failed. > > > > This commit serializes only the lazy loading entry points. > > > > libdwfl/ > > > > * dwfl_begin.c (dwfl_begin): Initialize load_lock. Also move > > the debuginfod_lock init to avoid NULL dereference. > > * dwfl_end.c (dwfl_end): Tear down load_lock. > > * dwfl_module_build_id.c (found_build_id): Store build_id_len > > atomically. > > (dwfl_module_build_id): Attempt to read build_id_len > > atomically. If it hasn't been loaded, acquire load_lock > > first. > > * dwfl_module_getdwarf.c (find_symtab_locked): Renamed from > > find_symtab. > > (find_symtab): Wrapper for find_symtab_locked that handles > > locking and checking symbols_loaded. > > (get_dwarf_locked): Renamed from dwfl_module_getdwarf. > > (dwfl_module_getdwarf): Wrapper for get_dwarf_locked that > > handles locking and checking dwarf_loaded. > > (__libdwfl_getelf): Reset mod->build_id_len atomically. > > * dwfl_module_getelf.c (get_elf_locked): Renamed from > > dwfl_module_getelf. > > (dwfl_module_getelf): Wrapper for get_elf_locked that handles > > locking and checking elf_loaded. > > * dwfl_module_getsym.c (__libdwfl_getsym): Check symbols_loaded > > before calling dwfl_module_getsymtab. > > * libdwflP.h (struct Dwfl): Add load_lock. > > (struct Dwfl_Module): Add bools symbols_loaded, dwarf_loaded > > and elf_loaded. > > > > Signed-off-by: Aaron Merey <[email protected]> > > --- > > v2 changes: > > Update dwarf_loaded in dwfl_module_getdwarf instead of > > get_dwarf_locked. > > This looks nicer/more consistent. > > > Also reset mod->build_id_len atomically in > > __libdwfl_getelf. > > Missed that one. But ... > > > diff --git a/libdwfl/dwfl_module_getdwarf.c b/libdwfl/dwfl_module_getdwarf.c > > index 430c3236..77af5189 100644 > > --- a/libdwfl/dwfl_module_getdwarf.c > > +++ b/libdwfl/dwfl_module_getdwarf.c > > @@ -237,7 +237,7 @@ __libdwfl_getelf (Dwfl_Module *mod) > > We'll fetch it from the file when asked. */ > > free (mod->build_id_bits); > > mod->build_id_bits = NULL; > > - mod->build_id_len = 0; > > + atomic_store_release (&mod->build_id_len, 0); > > } > > else if (fallback) > > mod_verify_build_id (mod); > > I am a little confused about how we end up in this situation. So, does > this switch mod->build_id_len from != 0 to 0? Then it seems to break > other callers to do atomic_load and checking whether or not it is zero. > The pattern is to atomically check if build_id_len is zero, if not, you > can continue with the value (or see that it is -1 for not found), > otherwise you take the load mutex (again check if it is still zero), > set the build_id and atomically update the build_id_len (to -1 if not > found), then drop the lock. > > What am I missing here?
You're right that concurrent readers could see a build_id_len > 0 and a non-NULL build_id_bits (set by dwfl_module_report_build_id) which become invalidated by another thread lazy loading the module's elf. We haven't published a libdwfl contract in THREAD-SAFETY yet but I'd propose requiring that the module's main.elf is set in a caller-serialized phase before concurrent access. Any handles/data derived from the module (including build_id_bits) would need to be rederived afterwards, like the contract specifies for elf_, dwarf_, etc interfaces. This libdwfl contract is being developed alongside the libdwfl thread safety implementation to help ensure we don't over or under promise, so the details of the contract are still in flux. I'd still like to merge this patch since it addresses thread safety gaps exercised by systemtap's concurrent use of elfutils. AFAICT systemtap reports ELF files before concurrent access so this build-id race condition does not occur. However I now see that adding the atomic_store_release to __libdwfl_getelf is unnecessary under the contract I proposed so this should remain an ordinary store. Precisely defining the libdwfl contract and comprehensively handling build-ids in accordance with the contract will be addressed in follow-up work. Aaron > > Thanks, > > Mark >
