Re: [PATCH v12 22/40] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

[sashiko-bot@kernel.org]

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

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/20261012-dd-cmap-part2-clean-v12-0-3c8cc5725def@gmail.com?part=22