On Tue, Sep 8, 2026 at 7:13 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [Medium] Newly loaded classmap user modules fail to sync disabled classes
> due to a hardcoded `0UL` state assumption.
> - [Medium] TOCTOU race in `ddebug_apply_class_users` when a class parameter
> is modified via sysfs concurrently with module load.
> - [Low] struct ddebug_class_user defines mod_name as non-const char*, which
> discards qualifiers from string literals.
> --
>
> commit ed72477b68f823658c20da0da2c5754d60e12c69
> Author: Jim Cromie <[email protected]>
>
> dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP
>
> DECLARE_DYNDBG_CLASSMAP had a flaw: multiple invocations across both
> core and driver modules broke the K&R "define once, refer afterwards"
> principle, causing boot-time parameter propagation failures.
>
> Replace it with an explicit client-server model:
> [ ... ]
>
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > index 144a2a947abcb..6517e0169a803 100644
> > --- a/include/linux/dynamic_debug.h
> > +++ b/include/linux/dynamic_debug.h
> [ ... ]
> > +struct ddebug_class_user {
> > + char *mod_name;
>
> [Severity: Low]
> Does this code discard qualifiers from string literals?
>
> Using the DYNAMIC_DEBUG_CLASSMAP_USE_ macro assigns the string literal
> DDEBUG_MODNAME to this field, which can trigger compiler warnings and
> compromises type safety. Should this be declared as const char *mod_name
> instead, similar to how it is defined in struct ddebug_class_map?
>
> > + struct ddebug_class_map *map;
> > + const int offset; /* offset from map->base */
> > +} __aligned(8);
>
> [ ... ]
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 2065bfd1687e0..0e25b4d53942b 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> [ ... ]
> > +/* called for class-users only, parse_one does this for definer modules */
> > +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);
> > + ddebug_class_param_clamp_input(&val, kp);
> > + new_bits = val;
> > + v2pr_info(" %s: classbits: 0x%x\n", KP_NAME(kp), new_bits);
> > + ddebug_apply_class_bitmap(dcp, &new_bits, 0UL, modname);
>
> [Severity: Medium]
> Will this fail to sync disabled classes for newly loaded modules?
>
> By hardcoding 0UL as the old_bits argument, ddebug_apply_class_bitmap()
> assumes the module starts with all debug classes disabled. This means it
> will skip generating disable commands for any cleared bits.
>
> If a module is compiled with -DDEBUG, its callsites are enabled by default
> and won't be correctly disabled to match the system-wide classmap parameter
ack. handled with a macro that yields a classbits full of 1s on DEBUG.
> state.
>
> > + break;
> > + case DD_CLASS_TYPE_LEVEL_NUM:
> > + val = READ_ONCE(*dcp->lvl);
> > + ddebug_class_param_clamp_input(&val, kp);
> > + 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, 0UL, modname);
> > + break;
> > + default:
> > + pr_err("bad map type %d\n", dcp->map->map_type);
> > + return;
> > + }
> > +}
> [ ... ]
> > @@ -1238,12 +1404,17 @@ static int ddebug_add_module(struct _ddebug_info
> > *di)
> > */
> > dt->info = *di;
> > dd_set_module_subrange(i, cm, &dt->info, maps);
> > + dd_set_module_subrange(i, cli, &dt->info, users);
> >
> > mutex_lock(&ddebug_lock);
> > list_add_tail(&dt->link, &ddebug_tables);
> > mutex_unlock(&ddebug_lock);
> >
> > - vpr_info("%3u debug prints in module %s\n", di->descs.len,
> > di->mod_name);
> > + if (dt->info.users.len)
> > + ddebug_apply_class_users(&dt->info);
>
> [Severity: Medium]
> Can this introduce a TOCTOU race with concurrent sysfs updates?
>
> Here, the new module is added to ddebug_tables and the ddebug_lock is
> dropped before the module's initial settings are applied by
> ddebug_apply_class_users().
>
> If a concurrent sysfs write to the dyndbg class parameter occurs in this
> lockless window, the sysfs thread will acquire ddebug_lock, apply its new
> parameter value to all modules (including this newly added one), and update
> the parameter value.
>
> When this load thread then calls ddebug_apply_class_users(), it will read the
> parameter unprotected. If it read the old parameter value before the sysfs
> thread overwrote it, it will subsequently acquire the lock deep inside
> ddebug_change() and apply the stale old value, overwriting the sysfs thread's
> newer changes. This would leave the module permanently out of sync.
>
There is no race here:
0. Separate definer vs user modules:
The locking in ddebug_add_module() adds the module to ddebug_tables so
it is reachable by params and >control queries.
Classmap definer modules (e.g. drm) EXPORT their classmap records;
class_user modules (e.g. amdgpu, drm_kms_helper) depend upon them.
The kernel module loader guarantees strict serialization: the definer
is fully live before any user module can begin loading.
1. Builtin ordering:
For builtins, the linker orders drm ahead of dependent drivers and
helpers in vmlinux, strictly ordering ddebug_add_module() calls during
early_initcall.
2. Deadlock avoidance:
ddebug_lock must be dropped before calling ddebug_apply_class_users()
because the sync path calls ddebug_change(), which acquires ddebug_lock.
Holding the lock across ddebug_apply_class_users() would self-deadlock.
3. Init bits handling:
Calculate init_bits based on (_DPRINTK_FLAGS_DEFAULT &
_DPRINTK_FLAGS_ACTIVE) (CLASSMAP_BITMASK(map->length) if DEBUG is
defined, else 0UL). Pass init_bits as old_bits to
ddebug_apply_class_bitmap() in ddebug_sync_classbits(), ensuring
disabled classbits correctly generate "-p" queries for modules built
with -DDEBUG.
