Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] dyndbg: TOCTOU race causes module debug state desynchronization - [Low] dyndbg: Missing ARRAY_SIZE include in DYNAMIC_DEBUG_CLASSMAP_DEFINE - [Low] dyndbg: Unreachable return statement in param_get_dyndbg_classes --
commit b545d297dc88b17fae1f9dd05bfa8c4854dc7fac Author: Jim Cromie <[email protected]> dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP This commit replaces DECLARE_DYNDBG_CLASSMAP with an explicit client-server model using DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE. This allows resolving references to parent classmaps and automatically applying the initial bitmap to the loading module at load time. > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h > --- a/include/linux/dynamic_debug.h > +++ b/include/linux/dynamic_debug.h > @@ -121,17 +138,86 @@ struct ddebug_class_param { > [ ... ] > + * DYNAMIC_DEBUG_CLASSMAP_DEFINE - define debug classes used by a module. > + * @_var: name of the classmap, exported for other modules coordinated use. > + * @_mapty: enum ddebug_class_map_type: 0:DISJOINT - independent, 1:LEVEL - > v2>v1 > + * @_base: reserve N classids starting at _base, to split 0..62 classid > space > + * @classes: names of the N classes. > + * > + * This tells dyndbg what class_ids the module is using: _base..+N, by > + * mapping names onto them. This qualifies "class NAME" >controls on > + * the defining module, ignoring unknown names. > + */ > +#define DYNAMIC_DEBUG_CLASSMAP_DEFINE(_var, _mapty, _base, ...) > \ > + static const char *_var##_classnames[] = { __VA_ARGS__ }; \ > + extern struct ddebug_class_map _var; \ > + struct ddebug_class_map __aligned(8) __used \ > + __section("__dyndbg_class_maps") _var = { \ > + .mod = THIS_MODULE, \ > + .mod_name = KBUILD_MODNAME, \ > + .base = (_base), \ > + .map_type = (_mapty), \ > + .length = ARRAY_SIZE(_var##_classnames), \ [Severity: Low] Does this macro rely on ARRAY_SIZE without including <linux/array_size.h> or <linux/kernel.h> in <linux/dynamic_debug.h>? This can cause implicit declaration build errors for modules that use this macro without independently including the required headers. > + .class_names = _var##_classnames, \ > + }; \ > + EXPORT_SYMBOL(_var) > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -773,6 +795,7 @@ int param_get_dyndbg_classes(char *buffer, const struct > kernel_param *kp) > default: > return -1; > } > + return 0; [Severity: Low] Is this return statement reachable? The switch statement on map->map_type exhaustively handles the valid cases and the default case, returning from all branches. This appears to make the subsequent return 0 dead code. > } > EXPORT_SYMBOL(param_get_dyndbg_classes); > [ ... ] > @@ -1210,163 +1404,163 @@ static const struct proc_ops proc_fops = { > [ ... ] > +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] Can this lockless read of the global parameter lead to a desynchronization of the module debug state? If a concurrent sysfs write to the parameter occurs via param_set_dyndbg_module_classes(), it reads the old value, applies the new state to all modules, and then updates the global parameter. If the module load thread in ddebug_sync_classbits() reads the global parameter locklessly before the sysfs thread updates it, it will see the old value and apply it to the new module, overwriting the sysfs changes. The sysfs thread would then complete, leaving the global parameter updated but the new module stuck with the old state. > + break; > + case DD_CLASS_TYPE_LEVEL_NUM: > + val = READ_ONCE(*dcp->lvl); > + new_bits = CLASSMAP_BITMASK(val); > + v2pr_info(" %s: lvl:%d bits:0x%x\n", KP_NAME(kp), val, > new_bits); > + ddebug_apply_class_bitmap(dcp, &new_bits, 0, modname); > + break; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
