Re: [PATCH v11 23/38] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

[sashiko-bot@kernel.org]

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 <jim.cromie@gmail.com>

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/20260923-dd-cmap-part2-clean-v11-0-9b6c217fdf2f@gmail.com?part=23