Re: [PATCH v2 19/24] dyndbg,module: make proper substructs in _ddebug_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Module loader searches for incorrectly named dynamic debug sections, completely disabling dynamic debug for all loadable modules.
- [High] Built-in modules without class maps erroneously inherit all global system-wide class maps due to missing state reset.
- [Low] Renaming `struct ddebug_class_param` breaks compilation of external users like the DRM subsystem.
--

commit 647ef6a50774bbbe7f638336c42ae9d912b4ca12
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg,module: make proper substructs in _ddebug_info

This commit recomposes struct _ddebug_info by moving the descriptors and class
maps fields into new sub-structures struct _ddebug_descs and struct
_ddebug_class_maps. It also normalizes all related struct names to use a
_ddebug_ prefix.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index a84dcc127e5a7..001bc84eadcab 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
[ ... ]
> @@ -81,21 +81,33 @@ struct ddebug_class_map {
>  	enum ddebug_class_map_type map_type;
>  };
>  
> -/* encapsulate linker provided built-in (or module) dyndbg data */
> +/*
> + * @_ddebug_info: gathers module/builtin dyndbg_* __sections together.
> + * For builtins, it is used as a cursor, with the inner structs
> + * marking sub-vectors of the builtin __sections in DATA.
> + */
> +struct _ddebug_descs {
> +	struct _ddebug *start;
> +	unsigned int len;
> +};
> +
> +struct _ddebug_class_maps {
> +	struct _ddebug_class_map *start;
> +	unsigned int len;
> +};
> +
>  struct _ddebug_info {
> -	struct _ddebug *descs;
> -	struct ddebug_class_map *classes;
> -	unsigned int num_descs;
> -	unsigned int num_classes;
> +	struct _ddebug_descs descs;
> +	struct _ddebug_class_maps maps;
>  };
>  
> -struct ddebug_class_param {
> +struct _ddebug_class_param {

[Severity: Low]
Does renaming this structure break compilation for external users?

For example, the DRM subsystem directly references the old structure name in
drivers/gpu/drm/drm_print.c:
"static struct ddebug_class_param drm_debug_bitmap"

Failing to update these external users will result in a build failure for
configurations enabling DRM.

>  	union {
>  		u64 *bits;
>  		u64 *lvl;
>  	};
>  	char flags[8];
> -	const struct ddebug_class_map *map;
> +	const struct _ddebug_class_map *map;
>  };
>  
>  /*
[ ... ]
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a6058..c2b6e70f2e773 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2774,12 +2774,12 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  		pr_warn("%s: Ignoring obsolete parameters\n", mod->name);
>  
>  #ifdef CONFIG_DYNAMIC_DEBUG_CORE
> -	mod->dyndbg_info.descs = section_objs(info, "__dyndbg",
> -					      sizeof(*mod->dyndbg_info.descs),
> -					      &mod->dyndbg_info.num_descs);
> -	mod->dyndbg_info.classes = section_objs(info, "__dyndbg_classes",
> -						sizeof(*mod->dyndbg_info.classes),
> -						&mod->dyndbg_info.num_classes);
> +	mod->dyndbg_info.descs.start = section_objs(info, "__dyndbg_descriptors",
> +						    sizeof(*mod->dyndbg_info.descs.start),
> +						    &mod->dyndbg_info.descs.len);
> +	mod->dyndbg_info.maps.start = section_objs(info, "__dyndbg_class_maps",
> +						   sizeof(*mod->dyndbg_info.maps.start),
> +						   &mod->dyndbg_info.maps.len);

[Severity: High]
Does this correctly match the section names emitted by the linker?

It looks like the section names here were updated to "__dyndbg_descriptors"
and "__dyndbg_class_maps", but the macros like DECLARE_DYNDBG_CLASSMAP and
DEFINE_DYNAMIC_DEBUG_METADATA still emit to "__dyndbg" and "__dyndbg_classes".

Because the strings do not match, section_objs() will fail to find the
sections, setting their lengths to 0 and effectively discarding all
dynamic debug prints for loadable modules.

I see this regression is fixed later in the series by commit f9409a020a4bc
("dyndbg: improve section names").

>  #endif
>  
>  	return 0;
[ ... ]
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 580aa258f902f..169b94ff6f8ed 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
[ ... ]
> @@ -1202,10 +1203,10 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
>  {
>  	struct ddebug_table *dt;
>  
> -	if (!di->num_descs)
> +	if (!di->descs.len)
>  		return 0;
>  
> -	v3pr_info("add-module: %s %d sites\n", modname, di->num_descs);
> +	v3pr_info("add-module: %s %d sites\n", modname, di->descs.len);
>  
>  	dt = kzalloc_obj(*dt);
>  	if (dt == NULL) {
> @@ -1219,19 +1220,18 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
>  	 * this struct ddebug_table.
>  	 */
>  	dt->mod_name = modname;
> -	dt->ddebugs = di->descs;
> -	dt->num_ddebugs = di->num_descs;
> +	dt->info = *di;

[Severity: High]
Does this code leave dt->info.maps initialized to the entire system-wide
class maps array if the built-in module has no class maps?

In dynamic_debug_init(), the global _ddebug_info di is initialized with
.maps spanning the entire __dyndbg_classes section.

When this is copied via dt->info = *di here, dt->info.maps also spans
the entire section.

If the module has no class maps, ddebug_attach_module_classes() will run, but
the local variable nc remains 0. The block if (nc) at the end is skipped,
and dt->info.maps is never updated to reflect 0 length.

As a result, built-in modules without class maps falsely match unrelated
class maps from other built-in modules.

I see this regression is fixed later in the series by commit 6fc0ea29f4f01
("dyndbg: hoist classmap-filter-by-modname up to ddebug_add_module").

>  
>  	INIT_LIST_HEAD(&dt->link);
>  
> -	if (di->classes && di->num_classes)
> +	if (di->maps.len)
>  		ddebug_attach_module_classes(dt, di);
>  
>  	mutex_lock(&ddebug_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523-dd-maint-2-v2-0-b937312aa083@gmail.com?part=19