On Fri, Oct 30, 2026 at 2:29 PM Mark Wielaard <[email protected]> wrote: > > 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.
Pushed with a plain store and a comment explaining why atomic is unnecessary. Aaron > > Thanks, > > Mark >
