Hi Aaron, On Fri, 2026-10-30 at 13:40 -0400, Aaron Merey wrote: > On Fri, Oct 30, 2026 at 11:21 AM Mark Wielaard <[email protected]> wrote: > > On Fri, 2026-10-30 at 01:16 -0400, Aaron Merey wrote: > > > > > 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.
OK. Makes sense to not "publish" (or share) a Dwfl_Module that doesn't yet have a valid Elf associated with it yet. Please add a comment in __libdwfl_getelf that explains why we don't need the atomic store here or a comment pointing to the (future) THEAD_SAFETY doc. Thanks, Mark
