Re: [PATCH v13 22/39] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dyndbg: unused ddebug_class_param_to_bits() and unclamped init in ddebug_sync_classbits()
--

commit 061cb31380a7641ad87f8c44222d55e83ea7da16
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

This commit replaces DECLARE_DYNDBG_CLASSMAP with
DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE to support
exporting a classmap from a core module and referencing it from user modules.
At module load time, dynamic debug scans both classmap sections and syncs the
initial class bitmap from the controlling kernel parameter.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 18289ca7170b7..ad86365f2474f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c

[ ... ]

> @@ -1174,6 +1213,163 @@ static const struct proc_ops proc_fops = {
>  	.proc_write = ddebug_proc_write
>  };
>  
> +#define vpr_cm_info(cm_p, msg_fmt, ...) ({				\
> +	struct ddebug_class_map const *_cm = cm_p;			\
> +	v2pr_info(msg_fmt "%s [%d..%d] %s..%s\n", ##__VA_ARGS__,	\
> +		  _cm->mod_name, _cm->base, _cm->base + _cm->length,	\
> +		  _cm->class_names[0], _cm->class_names[_cm->length - 1]); \
> +	})
> +
> +/*
> + * Modules which define classmaps get them initialized by
> + * param-callback via module.c:parse_one.  Modules which use other's
> + * classmaps must be initialized explicitly.
> + */
> +static inline u32 ddebug_class_param_to_bits(const struct ddebug_class_param *dcp)
> +{
> +	const struct ddebug_class_map *map = dcp->map;
> +
> +	switch (map->map_type) {
> +	case DD_CLASS_TYPE_DISJOINT_BITS:
> +		return *dcp->bits & CLASSMAP_BITMASK(map->length);
> +	case DD_CLASS_TYPE_LEVEL_NUM:
> +		return CLASSMAP_BITMASK(min_t(u32, *dcp->lvl, map->length));
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static void __maybe_unused ddebug_class_param_clamp_input(u32 *inrep, const struct kernel_param *kp)
> +{
> +	const struct ddebug_class_param *dcp = kp->arg;
> +	const struct ddebug_class_map *map = dcp->map;
> +
> +	switch (map->map_type) {
> +	case DD_CLASS_TYPE_DISJOINT_BITS:
> +		/* expect bits. mask and warn if too many */
> +		if (*inrep & ~CLASSMAP_BITMASK(map->length)) {
> +			pr_warn("%s: input: 0x%x exceeds mask: 0x%x, masking\n",
> +				KP_NAME(kp), *inrep, CLASSMAP_BITMASK(map->length));
> +			*inrep &= CLASSMAP_BITMASK(map->length);
> +		}
> +		break;
> +	case DD_CLASS_TYPE_LEVEL_NUM:
> +		/* input is bitpos, of highest verbosity to be enabled */
> +		if (*inrep > map->length) {
> +			pr_warn("%s: level:%d exceeds max:%d, clamping\n",
> +				KP_NAME(kp), *inrep, map->length);
> +			*inrep = map->length;
> +		}
> +		break;
> +	}
> +}
> +
> +/*
> + * Called for class-users and native classmaps during module load.
> + *
> + * Initial sync assumes 0 as old_bits, so it only asserts (+p) classes
> + * explicitly set by the parameter. -DDEBUGs invalidate this assumption
> + * for some pr_debugs; a user can override the developer/builder choice
> + * by toggling the param at runtime.
> + *
> + * Parameters are sampled via READ_ONCE without ddebug_lock;
> + * holding ddebug_lock here would deadlock against ddebug_change().
> + */
> +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);
> +		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;

[Severity: Low]
Is ddebug_class_param_to_bits() intended to be used here, or should it be
dropped? It is introduced in this patch, but never called here or in the rest
of the series (while ddebug_class_param_clamp_input() is only wired into
param_set_dyndbg_module_classes() in the later commit "dyndbg-API: promote
DYNAMIC_DEBUG_CLASSMAP_PARAM to API").

Also, in lib/test_dynamic_debug.c (and later in include/linux/dynamic_debug.h
when promoted in "dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API"),
DYNAMIC_DEBUG_CLASSMAP_PARAM initializes the backing u32 to ~0 when DEBUG is
defined:

#ifdef DEBUG
#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_model, _flags)		\
	DYNAMIC_DEBUG_CLASSMAP_PARAM_(_model, _flags, ~0)
#else
#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_model, _flags)		\
	DYNAMIC_DEBUG_CLASSMAP_PARAM_(_model, _flags, 0)
#endif

Because ddebug_sync_classbits() reads *dcp->bits and *dcp->lvl directly
without masking or clamping them to dcp->map->length, won't
param_get_dyndbg_classes() report 0xffffffff for DD_CLASS_TYPE_DISJOINT_BITS
(instead of CLASSMAP_BITMASK(map->length)) and 4294967295 for
DD_CLASS_TYPE_LEVEL_NUM (instead of map->length) when built with -DDEBUG?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=22