Re: [PATCH 21/63] dyndbg: allow ddebug_add_module to fail

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 25/01/2025 à 07:45, Jim Cromie a écrit :
> To prep for failing modprobe on classid conflicts, upgrade the
> call-chain around ddebug_add_module(), in 2 ways:
> 
> 1. in ddebug_add_module() add local reserved_ids to accumulate
> reservations, pass it by ref to ddebug_attach_{,user_}module_classes()
> so they can examine the reservations as they work.
> 
> 2. return int from both ddebug_attach_{,user_}module_classes(), up to
> ddebug_add_module(), then to ddebug_module_notify().
> 
> No conflicts are currently detected or returned.
> 
> TBD: This is updated further by hoisting the reservation-check, which
> obsoletes part of 2, creating churn, maybe squash it away.

Hi Jim,

It could be very nice to squash when possible yes!

> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
>   lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 16c9b752822b..0ef243e30663 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -1216,8 +1216,9 @@ static void ddebug_apply_params(const struct ddebug_class_map *cm, const char *m
>    * modular classmap vector/section.  Save the start and length of the
>    * subrange at its edges.
>    */
> -static void ddebug_attach_module_classes(struct ddebug_table *dt,
> -					 const struct _ddebug_info *di)
> +static int ddebug_attach_module_classes(struct ddebug_table *dt,
> +					const struct _ddebug_info *di,
> +					u64 *reserved_ids)
>   {
>   	struct ddebug_class_map *cm;
>   	int i, nc = 0;
> @@ -1230,13 +1231,14 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
>   		}
>   	}
>   	if (!nc)
> -		return;
> +		return 0;
>   
>   	vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
>   	dt->info.maps.len = nc;
>   
>   	for_subvec(i, cm, &dt->info, maps)
>   		ddebug_apply_params(cm, cm->mod_name);
> +	return 0;
>   }
>   
>   /*
> @@ -1244,8 +1246,9 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
>    * means a query against the dt/module, which means it must be on the
>    * list to be seen by ddebug_change.
>    */
> -static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
> -					      const struct _ddebug_info *di)
> +static int ddebug_attach_user_module_classes(struct ddebug_table *dt,
> +					      const struct _ddebug_info *di,
> +					      u64 *reserved_ids)
>   {
>   	struct ddebug_class_user *cli;
>   	int i, nc = 0;
> @@ -1266,7 +1269,7 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
>   		}
>   	}
>   	if (!nc)
> -		return;
> +		return 0;
>   
>   	dt->info.users.len = nc;
>   
> @@ -1275,6 +1278,7 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
>   		ddebug_apply_params(cli->map, cli->mod_name);
>   
>   	vpr_dt_info(dt, "attach-client-module: ");
> +	return 0;
>   }
>   
>   /*
> @@ -1284,6 +1288,8 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
>   static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
>   {
>   	struct ddebug_table *dt;
> +	u64 reserved_ids = 0;
> +	int rc;
>   
>   	if (!di->descs.len)
>   		return 0;
> @@ -1306,16 +1312,23 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
>   
>   	INIT_LIST_HEAD(&dt->link);
>   
> -	if (di->maps.len)
> -		ddebug_attach_module_classes(dt, di);
> -
> +	if (di->maps.len) {
> +		rc = ddebug_attach_module_classes(dt, di, &reserved_ids);
> +		if (rc) {
> +			kfree(dt);
> +			return rc;
> +		}
> +	}
>   	mutex_lock(&ddebug_lock);
>   	list_add_tail(&dt->link, &ddebug_tables);
>   	mutex_unlock(&ddebug_lock);
>   
> -	if (di->users.len)
> -		ddebug_attach_user_module_classes(dt, di);
>   
> +	if (di->users.len) {
> +		rc = ddebug_attach_user_module_classes(dt, di, &reserved_ids);
> +		if (rc)
> +			return rc;
> +	}
>   	vpr_info("%3u debug prints in module %s\n", di->descs.len, modname);
>   	return 0;
>   }
> @@ -1400,6 +1413,11 @@ static int ddebug_module_notify(struct notifier_block *self, unsigned long val,
>   	switch (val) {
>   	case MODULE_STATE_COMING:
>   		ret = ddebug_add_module(&mod->dyndbg_info, mod->name);
> +		if (ret == -EINVAL) {
> +			pr_err("conflicting dyndbg-classmap reservations\n");
> +			ddebug_remove_module(mod->name);
> +			break;
> +		}
>   		if (ret)
>   			WARN(1, "Failed to allocate memory: dyndbg may not work properly.\n");
>   		break;

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com