Re: [PATCH 23/63] dyndbg: fail modprobe on ddebug_class_range_overlap()

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

Le 25/01/2025 à 07:45, Jim Cromie a écrit :
> 1. All classes used by a module (declared DYNDBG_CLASSMAP_{DEFINE,USE}
> by module code) must share 0..62 class-id space; ie their respective
> base,+length reservations shouldn't overlap.  Overlaps would lead to
> unintended changes in ddebug enablements.
> 
> Detecting these class-id range overlaps at compile-time would be ideal
> but is not obvious how; failing at modprobe at least insures that the
> developer sees and fixes the conflict.
> 
> ddebug_class_range_overlap() implements the range check, accumulating
> the reserved-ids as it examines each class.  It probably should use
> bitmaps.
> 
> A previous commit reworked the modprobe callchain to allow failure,
> now call ddebug_class_range_overlap() to check when classid conflicts
> happen, and signal that failure.
> 
> NB: this can only happen when a module defines+uses several classmaps,
> 
> TBD: failing modprobe is kinda harsh, maybe warn and proceed ?
> 
> test_dynamic_debug*.ko:
> 
> If built with -DFORCE_CLASSID_CONFLICT_MODPROBE, the modules get 2 bad
> DYNDBG_CLASS_DEFINE declarations, into parent and the _submod.  These
> conflict with one of the good ones in the parent (D2_CORE..etc),
> causing the modprobe(s) to fail.  TODO: do in submod only, since fail
> of parent prevents submod from ever trying.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> ---
>   lib/dynamic_debug.c      | 30 ++++++++++++++++++++++++------
>   lib/test_dynamic_debug.c | 11 ++++++++++-
>   2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 8afcd4111531..8e1e087e07c3 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -1211,6 +1211,21 @@ static void ddebug_apply_params(const struct ddebug_class_map *cm, const char *m
>   	}
>   }
>   
> +static int ddebug_class_range_overlap(struct ddebug_class_map *cm,
> +				      u64 *reserved_ids)
> +{
> +	u64 range = (((1ULL << cm->length) - 1) << cm->base);
> +
> +	if (range & *reserved_ids) {
> +		pr_err("[%d..%d] on %s conflicts with %llx\n", cm->base,
> +		       cm->base + cm->length - 1, cm->class_names[0],
> +		       *reserved_ids);
> +		return -EINVAL;
> +	}
> +	*reserved_ids |= range;
> +	return 0;
> +}
> +
>   /*
>    * scan the named array: @_vec, ref'd from inside @_box, for the
>    * start,len of the sub-array of elements matching on ->mod_name;
> @@ -1242,9 +1257,11 @@ static int ddebug_module_apply_class_maps(struct ddebug_table *dt,
>   	struct ddebug_class_map *cm;
>   	int i;
>   
> -	for_subvec(i, cm, &dt->info, maps)
> +	for_subvec(i, cm, &dt->info, maps) {
> +		if (ddebug_class_range_overlap(cm, reserved_ids))
> +			return -EINVAL;
>   		ddebug_apply_params(cm, cm->mod_name);
> -
> +	}
>   	vpr_info("module:%s attached %d classmaps\n", dt->mod_name, dt->info.maps.len);
>   	return 0;
>   }
> @@ -1255,10 +1272,11 @@ static int ddebug_module_apply_class_users(struct ddebug_table *dt,
>   	struct ddebug_class_user *cli;
>   	int i;
>   
> -	/* now iterate dt */
> -	for_subvec(i, cli, &dt->info, users)
> +	for_subvec(i, cli, &dt->info, users) {
> +		if (ddebug_class_range_overlap(cli->map, reserved_ids))
> +			return -EINVAL;
>   		ddebug_apply_params(cli->map, cli->mod_name);
> -
> +	}
>   	vpr_info("module:%s attached %d classmap uses\n", dt->mod_name, dt->info.users.len);
>   	return 0;
>   }
> @@ -1311,11 +1329,11 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
>   			return rc;
>   		}
>   	}
> +
>   	mutex_lock(&ddebug_lock);
>   	list_add_tail(&dt->link, &ddebug_tables);
>   	mutex_unlock(&ddebug_lock);
>   
> -

Hi Jim,

Strange line issues, can you squash it with the correct patch?

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

Thanks,

>   	if (dt->info.users.len) {
>   		rc = ddebug_module_apply_class_users(dt, &reserved_ids);
>   		if (rc)
> diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
> index b1555b0a2bb1..74b98adc4ed0 100644
> --- a/lib/test_dynamic_debug.c
> +++ b/lib/test_dynamic_debug.c
> @@ -81,7 +81,7 @@ enum cat_disjoint_bits {
>   	D2_DRMRES };
>   
>   /* numeric verbosity, V2 > V1 related */
> -enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
> +enum cat_level_num { V0 = 16, V1, V2, V3, V4, V5, V6, V7 };
>   
>   /* recapitulate DRM's parent(drm.ko) <-- _submod(drivers,helpers) */
>   #if !defined(TEST_DYNAMIC_DEBUG_SUBMOD)
> @@ -90,6 +90,7 @@ enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
>    * classmaps on the client enums above, and then declares the PARAMS
>    * ref'g the classmaps.  Each is exported.
>    */
> +
>   DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS,
>   		       D2_CORE,
>   		       "D2_CORE",
> @@ -113,6 +114,14 @@ DYNDBG_CLASSMAP_DEFINE(map_level_num, DD_CLASS_TYPE_LEVEL_NUM,
>   DYNDBG_CLASSMAP_PARAM(disjoint_bits, p);
>   DYNDBG_CLASSMAP_PARAM(level_num, p);
>   
> +#ifdef FORCE_CLASSID_CONFLICT_MODPROBE
> +/*
> + * Enable with -Dflag on compile to test overlapping class-id range
> + * detection.  This should break on modprobe.
> + */
> +DYNDBG_CLASSMAP_DEFINE(classid_range_conflict, 0, D2_CORE + 1, "D3_CORE");
> +#endif
> +
>   #else /* TEST_DYNAMIC_DEBUG_SUBMOD */
>   
>   /*

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