Re: [PATCH v2 30/59] dyndbg: drop "protection" of class'd pr_debugs from legacy queries

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

Le 20/03/2025 à 19:52, Jim Cromie a écrit :
> Current classmap code protects class'd pr_debugs from unintended
> changes by "legacy" unclassed queries:
> 
>    # this doesn't disable all of DRM_UT_* categories
>    echo "-p" > /proc/dynamic_debug/control
> 
>    # name the class to change it - protective but tedious
>    echo "class DRM_UT_CORE +p" > /proc/dynamic_debug/control
> 
>    # or do it the subsystem way
>    echo 1 > /sys/module/drm/parameters/debug
> 
> This "name the class to change it" behavior gave a modicum of
> protection to classmap users (ie DRM) so their debug settings aren't
> trivially and unintentionally altered underneath them.
> 
> But this made the class keyword special in some sense; the other
> keywords skip only on explicit mismatch, otherwize the code falls thru

s/otherwize/otherwise/

> to adjust the pr-debug site.
> 
> So Jason Baron didn't like this special case when I 1st proposed it;
> I argued 2 points:
> - "protection gives stable-debug, improving utility"
> - __drm_debug is authoritative w/o dyndbg under it.
> 
> I thought I'd convinced him back then, (and the patchset got merged),
> but he noted it again when he reviewed this series.  So this commit
> names the "special case": ddebug_client_module_protects_classes(), and
> reverts it to Jason's preference.
  >
> If a class mismatch is seen, code distinguishes whether the class was
> explicitly given (and always skips/continue), or the DFLT was assumed
> because no class was given.  Here we test
> ddebug_client_module_protects_classes(), skip if so.
> 
> Later, if any user/module wants to protect its classes, we could add a
> flag to ddebug_table, a means to set it from CLASSMAP_DEFINE, and
> check it when applying a classless query/cmd.

I don't really understand the goal of the protection, do you have the 
discussion between you and Jason so I can have some context and some 
answer to my questions?

With the example you gave above, I think this could lead to a very odd 
behavior: if I enable dyndbg, I expect any pr_dbg to be managed by 
dyndbg settings.

If a user writes stuff on dyndbg control, he clearly knows what he is 
doing, and he wants to control what logs he wants.

And if you allow multiple "protected" users, the normal way to disable 
all dyndbg logs will be:

	ddcmd -p
	ddcmd class DRM_UT_CORE -p
	ddcmd class DRM_... -p # all drm classes
	ddcmd class SPI_... -p # all spi classes
	ddcmd class WHATEVER_... -p # all other subsystem

	# And only now you can enable only what you want
	ddcmd module my_mod +p

This is clearly annoying to write.

If DRM (or whatever subsystem) wants to add a debug parameter and use it 
to control their logs without being impacted by dyndbg, I believe it 
should not use dyndbg classes to do it.

> CC: jbaron@akamai.com
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>   lib/dynamic_debug.c | 34 +++++++++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index c44502787c2b..13de0dd3a4ad 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -193,6 +193,17 @@ static int ddebug_find_valid_class(struct ddebug_table const *dt, const char *cl
>   	return -ENOENT;
>   }
>   
> +/*
> + * classmaps-v1 protected classes from changes by legacy commands
> + * (those selecting _DPRINTK_CLASS_DFLT by omission), v2 undoes that
> + * special treatment.  State so explicitly.  Later we could give
> + * modules the choice to protect their classes or to keep v2 behavior.
> + */
> +static inline bool ddebug_client_module_protects_classes(const struct ddebug_table *dt)
> +{
> +	return false;
> +}
> +
>   /*
>    * Search the tables for _ddebug's which match the given `query' and
>    * apply the `flags' and `mask' to them.  Returns number of matching
> @@ -206,7 +217,7 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
>   	unsigned int newflags;
>   	unsigned int nfound = 0;
>   	struct flagsbuf fbuf, nbuf;
> -	int valid_class;
> +	int slctd_class;

Nitpick: can you use full words? slctd is difficult to read.

>   
>   	/* search for matching ddebugs */
>   	mutex_lock(&ddebug_lock);
> @@ -218,21 +229,26 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
>   			continue;
>   
>   		if (query->class_string) {
> -			valid_class = ddebug_find_valid_class(dt, query->class_string);
> -			if (valid_class < 0)
> +			slctd_class = ddebug_find_valid_class(dt, query->class_string);
> +			if (slctd_class < 0)
> +				/* skip/reject classes unknown by module */
>   				continue;
>   		} else {
> -			/* constrain query, do not touch class'd callsites */
> -			valid_class = _DPRINTK_CLASS_DFLT;
> +			slctd_class = _DPRINTK_CLASS_DFLT;
>   		}
>   
>   		for (i = 0; i < dt->info.descs.len; i++) {
>   			struct _ddebug *dp = &dt->info.descs.start[i];
>   
> -			/* match site against query-class */
> -			if (dp->class_id != valid_class)
> -				continue;
> -
> +			if (dp->class_id != slctd_class) {
> +				if (query->class_string)
> +					/* site.class != given class */
> +					continue;
> +				/* legacy query, class'd site */
> +				else if (ddebug_client_module_protects_classes(dt))
> +					continue;
> +				/* allow change on class'd pr_debug */
> +			}
>   			/* match against the source filename */
>   			if (query->filename &&
>   			    !match_wildcard(query->filename, dp->filename) &&

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