Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dyndbg: TOCTOU race in ddebug_sync_classbits allows stale module settings - [Low] dyndbg: incompatible pointer type in module section load --
commit 0e12bed35baa8c038def6b06ffc95416176ee997 Author: Jim Cromie <[email protected]> dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP This commit replaces DECLARE_DYNDBG_CLASSMAP with an explicit client-server model using DEFINE and USE macros. It resolves boot-time parameter propagation failures by establishing a robust reference linkage. At module load time, dyndbg now scans and resolves parent classmap references. > diff --git a/kernel/module/main.c b/kernel/module/main.c > --- a/kernel/module/main.c > +++ b/kernel/module/main.c [ ... ] > @@ -2791,6 +2791,9 @@ static int find_module_sections(struct module *mod, > struct load_info *info) > mod->dyndbg_info.maps.start = section_objs(info, "__dyndbg_class_maps", > > sizeof(*mod->dyndbg_info.maps.start), > &mod->dyndbg_info.maps.len); > + mod->dyndbg_info.users.start = section_objs(info, > "__dyndbg_class_users", > + > sizeof(*mod->dyndbg_info.users.start), > + &mod->dyndbg_info.users.len); [Severity: Low] The len field in struct _ddebug_class_users is declared as a signed int, but section_objs() expects an unsigned int pointer for its num parameter. Could passing &mod->dyndbg_info.users.len here trigger an incompatible pointer type compiler warning and result in a build error under standard configurations? [ ... ] > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c [ ... ] > +static void ddebug_sync_classbits(const struct kernel_param *kp, const char > *modname) > +{ > + const struct ddebug_class_param *dcp = kp->arg; > + u32 val, new_bits; > + > + if (!dcp || !dcp->map) > + return; > + > + switch (dcp->map->map_type) { > + case DD_CLASS_TYPE_DISJOINT_BITS: > + val = READ_ONCE(*dcp->bits); > + new_bits = val; > + v2pr_info(" %s: classbits: 0x%x\n", KP_NAME(kp), new_bits); > + ddebug_apply_class_bitmap(dcp, &new_bits, 0, modname); [Severity: Medium] Because dcp->bits is read locklessly via READ_ONCE() before ddebug_apply_class_bitmap() takes ddebug_lock, is there a race condition here? If a user concurrently writes to the corresponding sysfs parameter, the sysfs thread will update existing modules under the lock, release it, and then update the parameter value. If a concurrent module load thread executing ddebug_sync_classbits() reads the old parameter value right before the sysfs thread writes the new value, and then acquires the lock to apply the settings: sysfs thread: param_set_dyndbg_module_classes() mutex_lock(&ddebug_lock); // updates existing modules mutex_unlock(&ddebug_lock); Module load thread in ddebug_sync_classbits(): val = READ_ONCE(*dcp->bits); // reads old stale value sysfs thread: *dcp->bits = new_value; Module load thread in ddebug_sync_classbits(): ddebug_apply_class_bitmap(...) mutex_lock(&ddebug_lock); // applies stale value Would the newly loading module miss the concurrent sysfs update and initialize with stale debug class settings? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=23
