Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ddebug_attach_module_classes() incorrectly assigns the first matching classmap subrange to all non-contiguous descriptor blocks of identically-named built-in modules, permanently dropping subsequent classmap subranges. --
commit 6fb00fe7f4da26525b10dfd1edfb384c35a9b281 Author: Jim Cromie <[email protected]> dyndbg: replace classmap list with an array-slice This commit replaces the list-based storage of dyndbg classmaps with contiguous array-slices. It simplifies classmap attachment by dropping list construction and reusing the module name filter to find subranges. > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 9bd521651c484..b8a494835ef58 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c [ ... ] > @@ -1150,30 +1152,34 @@ static const struct proc_ops proc_fops = { > .proc_write = ddebug_proc_write > }; > > -static void ddebug_attach_module_classes(struct ddebug_table *dt, > - struct ddebug_class_map *classes, > - int num_classes) > +static void ddebug_attach_module_classes(struct ddebug_table *dt, struct > _ddebug_info *di) > { > struct ddebug_class_map *cm; > - int i, j, ct = 0; > + int i, nc = 0; > > - for (cm = classes, i = 0; i < num_classes; i++, cm++) { > + /* > + * Find this module's classmaps in a subrange/wholerange of > + * the builtin/modular classmap vector/section. Save the start > + * and length of the subrange at its edges. > + */ > + for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) { > > if (!strcmp(cm->mod_name, dt->mod_name)) { > - > - v2pr_info("class[%d]: module:%s base:%d len:%d > ty:%d\n", i, > - cm->mod_name, cm->base, cm->length, > cm->map_type); > - > - for (j = 0; j < cm->length; j++) > - v3pr_info(" %d: %d %s\n", j + cm->base, j, > - cm->class_names[j]); > - > - list_add(&cm->link, &dt->maps); > - ct++; > + if (!nc) { > + v2pr_info("start subrange, class[%d]: module:%s > base:%d len:%d ty:%d\n", > + i, cm->mod_name, cm->base, > cm->length, cm->map_type); > + dt->classes = cm; > + } > + nc++; > + } else if (nc) { > + /* end of matching classmaps */ > + break; > } [Severity: High] Does this break prematurely for identically-named built-in modules? Because KBUILD_MODNAME currently allows multiple built-in modules to have the same name (e.g., "main"), these modules can have non-contiguous descriptor and classmap blocks in the ELF section. When ddebug_attach_module_classes() is called for a second "main" module, it scans from the beginning of di->classes, finds the first "main" module's classmaps, and immediately breaks upon the first non-match. Will this cause the second module's actual classmaps to be permanently discarded, assigning it the wrong classmap subrange instead? Note: I saw this issue is resolved later in the series by commit 3e924d4e19d326f9d0f87715e82bd2970e66e217 ("dynamic_debug: use KBUILD_MODFILE for unique builtin module names"), which prevents identically-named built-in modules by using unique file paths. > } > - if (ct) > - vpr_info("module:%s attached %d classes\n", dt->mod_name, ct); > + if (nc) { > + dt->num_classes = nc; > + vpr_info("module:%s attached %d classes\n", dt->mod_name, nc); > + } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
