Re: [PATCH v7 25/29] dyndbg,module: make proper substructs in _ddebug_info

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Built-in modules without their own dynamic debug class maps incorrectly inherit the entire array of built-in class maps, causing cross-contamination of debug class settings.
--

--- Patch [25]: [PATCH v7 25/29] dyndbg,module: make proper substructs in _ddebug_info ---
commit 9690152a047a31662e977dac229f6c3d6cb68cd2
Author: Jim Cromie <[email protected]>

dyndbg,module: make proper substructs in _ddebug_info

This commit recomposes struct _ddebug_info by inserting proper sub-structs
for descriptors and class maps, and updates the dynamic debug core to use
these new structures.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 60b67face46e1..e7742fcca50ba 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
[ ... ]
> @@ -1185,12 +1186,12 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug
>  	 * the builtin/modular classmap vector/section.  Save the start
>  	 * and length of the subrange at its edges.
>  	 */
> -	for_subvec(i, cm, di, classes) {
> +	for_subvec(i, cm, di, maps) {
>  		if (!strcmp(cm->mod_name, dt->mod_name)) {
>  			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;
> +				dt->info.maps.start = cm;
>  			}
>  			nc++;
>  		} else if (nc) {
> @@ -1199,7 +1200,7 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug
>  		}
>  	}
>  	if (nc) {
> -		dt->num_classes = nc;
> +		dt->info.maps.len = nc;
>  		vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
>  	}
>  }

[Severity: Medium]
When a module has no classes and nc remains 0, does dt->info.maps retain
the values originally copied from di?

Since dt->info = *di is performed before this function is called, this means
the module will incorrectly inherit the entire global array of built-in
class maps. Could this cause cross-contamination of debug class settings
between unrelated modules?

[ ... ]
> @@ -1212,10 +1213,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) {
> @@ -1229,19 +1230,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;
>  
>  	INIT_LIST_HEAD(&dt->link);
>  
> -	if (di->classes && di->num_classes)
> +	if (di->maps.len)
>  		ddebug_attach_module_classes(dt, di);

[Severity: Medium]
If di->maps.len > 0 (which is true for built-ins since di spans all global
maps), but the module itself has no classes, ddebug_attach_module_classes()
won't reset dt->info.maps.len to 0.

Would it be better to clear dt->info.maps.len and dt->info.maps.start here
before calling ddebug_attach_module_classes() to prevent the module from
accidentally keeping the global state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=25