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 <[email protected]> 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/[email protected]?part=19
