Re: [PATCH v4 16/25] dyndbg: replace classmap list with an array-slice

[sashiko-bot@kernel.org]

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

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/20260602-dd-maint-2-v4-0-19a1445585a8@gmail.com?part=16