classmap-v1 code protected class'd pr_debugs from unintended
changes by unclassed/_DFLT queries:
# - to declutter examples:
alias ddcmd='echo $* > /proc/dynamic_debug/control'
# IOW, this should NOT alter drm.debug settings
ddcmd -p
# Instead, you must name the class to change it.
# Protective but tedious
ddcmd class DRM_UT_CORE +p
# Or do it the (old school) subsystem way
# This is ABI !!
echo 1 > /sys/module/drm/parameters/debug
Since the debug sysfs-node is ABI, if dyndbg is going to implement it,
it must also honor its settings; it must at least protect against
accidental changes to its classes from legacy queries.
The protection allows all previously conceived queries to work the way
they always have; ie select the same set of pr_debugs, despite the
inclusion of whole new classes of pr_debugs.
But that choice has 2 downsides:
1. "name the class to change it" makes a tedious long-winded
interface, needing many commands to set DRM_UT_* one at a time.
2. It makes the class keyword special in some sense; the other
keywords skip only on query mismatch, otherwise the code falls thru to
adjust the pr-debug site.
Jason Baron didn't like v1 on point 2.
Louis Chauvet didn't like recent rev on point 1 tedium.
But that said: /sys/ is ABI, so this must be reliable:
#> echo 0x1f > /sys/module/drm/parameters/debug
It 'just works' without dyndbg underneath; we must deliver that same
stability. Convenience is secondary.
The new resolution:
If ABI is the blocking issue, then no ABI means no blocking issue.
IOW, if the classmap has no presence under /sys/*, ie no PARAM, there
is no ABI to guard, and no reason to enforce a tedious interface.
In the future, if DRM wants to alter this protection, that is
practical, but I think default-on is the correct mode.
So atm classes without a PARAM are unprotected at >control, allowing
admins their shortcuts. I think this could satisfy all viewpoints.
That said, theres also a possibility of wildcard classes:
#> ddcmd class '*' +p
Currently, the query-class is exact-matched against each module's
classmaps.names. This gives precise behavior, a good basis.
But class wildcards are possible, they just did'nt appear useful for
DRM, whose classmap names are a flat DRM_UT_* namespace.
IOW, theres no useful selectivity there:
#> ddcmd class "DRM_*" +p # these enable every DRM_* class
#> ddcmd class "DRM_UT_*" +p
#> ddcmd class "DRM_UT_V*" +p # finally select just 1: DRM_UT_VBL
#> ddcmd class "DRM_UT_D*" +p # but this gets 3
#> ddcmd class "D*V*" +p # here be dragons
But there is debatable utility in the feature.
#> ddcmd class __DEFAULT__ -p # what about this ?
#> ddcmd -p # thats what this does. automatically
Anyway, this patch does:
1. adds link field from _ddebug_class_map to the .controlling_param
2. sets it in ddebug_match_apply_kparam(), during modprobe/init,
when options like drm.debug=VAL are handled.
3. ddebug_class_has_param() now checks .controlling_param
4. ddebug_class_wants_protection() macro renames 3.
this frames it as a separable policy decision
5. ddebug_match_desc() gets the most attention:
a. move classmap consideration to the bottom
this insures all other constraints act 1st.
allows simpler 'final' decisions.
b. split class choices cleanly on query:
class FOO vs none, and class'd vs _DPRINTK_CLASS_DFLT site.
c. calls 4 when applying a class-less query to a class'd pr_debug
here we need a new fn to find the classmap with this .class_id
d. calls new ddebug_find_classmap_by_class_id().
when class-less query looks at a class'd pr_debug.
finds classmap, which can then decide, currently by PARAM existence.
NOTES:
protection is only against class-less queries, explicit "class FOO"
adjustments are allowed (that is the mechanism).
The drm.debug sysfs-node heavily under-specifies the class'd pr_debugs
it controls; none of the +mfls prefixing flags have any effect, and
each callsite remains individually controllable. drm.debug just
toggles the +p flag for all the modules' class'd pr_debugs.
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
history
-v0 - original, before classmaps: no special case keywords
-v1 - "class DEFAULT" is assumed if not mentioned.
this protects classes from class-less queries
-v2.pre-this-patch - protection macro'd to false
-v2.with-this-patch - sysfs knob decides
-v2.speculative - module decides wrt classmap protection
seems unneeded now, TBD
v3 - new patch
v4
- drop fn-scope map var, with 2 local vars, renamed to purpose
- fix for NULL ptr case.
- Add loop-var to reduce many "&dt->info." exprs to "di->"
- add 1-liner postcondition comments
fixus
---
include/linux/dynamic_debug.h | 14 ++--
lib/dynamic_debug.c | 127 +++++++++++++++++++++++++++-------
2 files changed, 110 insertions(+), 31 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index b1d11d946780..b22da40e2583 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -75,6 +75,7 @@ enum ddebug_class_map_type {
* map @class_names 0..N to consecutive constants starting at @base.
*/
struct _ddebug_class_map {
+ struct _ddebug_class_param *controlling_param;
const struct module *mod; /* NULL for builtins */
const char *mod_name;
const char **class_names;
@@ -259,7 +260,12 @@ struct _ddebug_class_param {
*
* Creates a sysfs-param to control the classes defined by the
* exported classmap, with bits 0..N-1 mapped to the classes named.
- * This version keeps class-state in a private long int.
+ *
+ * Since sysfs-params are ABI, this also protects the classmap'd
+ * pr_debugs from un-class'd `echo -p > /proc/dynamic_debug/control`
+ * changes.
+ *
+ * This keeps class-state in a private long int.
*/
#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags) \
static unsigned long _name##_bvec; \
@@ -272,10 +278,8 @@ struct _ddebug_class_param {
* @_var: name of the (exported) classmap var defining the classes/bits
* @_flags: flags to be toggled, typically just 'p'
*
- * Creates a sysfs-param to control the classes defined by the
- * exported clasmap, with bits 0..N-1 mapped to the classes named.
- * This version keeps class-state in user @_bits. This lets drm check
- * __drm_debug elsewhere too.
+ * Like DYNAMIC_DEBUG_CLASSMAP_PARAM, but maintains param-state in
+ * extern @_bits. This lets DRM check __drm_debug elsewhere too.
*/
#define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) \
__DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 636a6b5741f7..1082e0273f0e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -206,6 +206,50 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
return NULL;
}
+static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
+{
+ return (class_id >= map->base &&
+ class_id < map->base + map->length);
+}
+
+static struct _ddebug_class_map *
+ddebug_find_map_by_class_id(struct _ddebug_info *di, int class_id)
+{
+ struct _ddebug_class_map *map;
+ struct _ddebug_class_user *cli;
+ int i;
+
+ for_subvec(i, map, di, maps)
+ if (ddebug_class_in_range(class_id, map))
+ return map;
+
+ for_subvec(i, cli, di, users)
+ if (ddebug_class_in_range(class_id, cli->map))
+ return cli->map;
+
+ return NULL;
+}
+
+/*
+ * classmaps-V1 protected classes from changes by legacy commands
+ * (those selecting _DPRINTK_CLASS_DFLT by omission). This had the
+ * downside that saying "class FOO" for every change can get tedious.
+ *
+ * V2 is smarter, it protects class-maps if the defining module also
+ * calls DYNAMIC_DEBUG_CLASSMAP_PARAM to create a sysfs parameter.
+ * Since the author wants the knob, we should assume they intend to
+ * use it (in preference to "class FOO +p" >control), and want to
+ * trust its settings. This gives protection when its useful, and not
+ * when its just tedious.
+ */
+static inline bool ddebug_class_has_param(const struct _ddebug_class_map *map)
+{
+ return !!(map->controlling_param);
+}
+
+/* re-framed as a policy choice */
+#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map))
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -214,11 +258,10 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
*/
static bool ddebug_match_desc(const struct ddebug_query *query,
struct _ddebug *dp,
- int valid_class)
+ struct _ddebug_info *di,
+ int selected_class)
{
- /* match site against query-class */
- if (dp->class_id != valid_class)
- return false;
+ struct _ddebug_class_map *site_map;
/* match against the source filename */
if (query->filename &&
@@ -255,7 +298,28 @@ static bool ddebug_match_desc(const struct ddebug_query *query,
dp->lineno > query->last_lineno)
return false;
- return true;
+ /*
+ * above are all satisfied, so we can make final decisions:
+ * 1- class FOO or implied class __DEFAULT__
+ * 2- site.is_classed or not
+ */
+ if (query->class_string) {
+ /* class FOO given, exact match required */
+ return (dp->class_id == selected_class);
+ }
+ /* query class __DEFAULT__ by omission. */
+ if (dp->class_id == _DPRINTK_CLASS_DFLT) {
+ /* un-classed site */
+ return true;
+ }
+ /* site is class'd */
+ site_map = ddebug_find_map_by_class_id(di, dp->class_id);
+ if (!site_map) {
+ /* _UNKNOWN_ class_id. XXX: Allow changes here ? */
+ return false;
+ }
+ /* module(-param) decides protection */
+ return !ddebug_class_wants_protection(site_map);
}
static int ddebug_change(const struct ddebug_query *query, struct flag_settings *modifiers)
@@ -265,33 +329,31 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
unsigned int newflags;
unsigned int nfound = 0;
struct flagsbuf fbuf, nbuf;
- struct _ddebug_class_map *map = NULL;
- int valid_class;
+ int selected_class;
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
list_for_each_entry(dt, &ddebug_tables, link) {
struct _ddebug_info *di = &dt->info;
+ struct _ddebug_class_map *mods_map;
/* match against the module name */
if (query->module &&
!match_wildcard(query->module, di->mod_name))
continue;
+ selected_class = _DPRINTK_CLASS_DFLT;
if (query->class_string) {
- map = ddebug_find_valid_class(&dt->info, query->class_string,
- &valid_class);
- if (!map)
+ mods_map = ddebug_find_valid_class(di, query->class_string,
+ &selected_class);
+ if (!mods_map)
continue;
- } else {
- /* constrain query, do not touch class'd callsites */
- valid_class = _DPRINTK_CLASS_DFLT;
}
for (i = 0; i < di->descs.len; i++) {
struct _ddebug *dp = &di->descs.start[i];
- if (!ddebug_match_desc(query, dp, valid_class))
+ if (!ddebug_match_desc(query, dp, di, selected_class))
continue;
nfound++;
@@ -656,6 +718,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
}
/* apply a new class-param setting */
+
static int ddebug_apply_class_bitmap(const struct _ddebug_class_param *dcp,
const unsigned long *new_bits,
const unsigned long old_bits,
@@ -1107,12 +1170,6 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
return dp;
}
-static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
-{
- return (class_id >= map->base &&
- class_id < map->base + map->length);
-}
-
static const char *ddebug_class_name(struct _ddebug_info *di, struct _ddebug *dp)
{
struct _ddebug_class_map *map;
@@ -1237,25 +1294,36 @@ static void ddebug_sync_classbits(const struct kernel_param *kp, const char *mod
}
}
-static void ddebug_match_apply_kparam(const struct kernel_param *kp,
- const struct _ddebug_class_map *map,
- const char *mod_name)
+static struct _ddebug_class_param *
+ddebug_get_classmap_kparam(const struct kernel_param *kp,
+ const struct _ddebug_class_map *map)
{
struct _ddebug_class_param *dcp;
if (kp->ops != ¶m_ops_dyndbg_classes)
- return;
+ return false;
dcp = (struct _ddebug_class_param *)kp->arg;
- if (map == dcp->map) {
+ return (map == dcp->map)
+ ? dcp : (struct _ddebug_class_param *)NULL;
+}
+
+static void ddebug_match_apply_kparam(const struct kernel_param *kp,
+ struct _ddebug_class_map *map,
+ const char *mod_name)
+{
+ struct _ddebug_class_param *dcp = ddebug_get_classmap_kparam(kp, map);
+
+ if (dcp) {
+ map->controlling_param = dcp;
v2pr_info(" kp:%s.%s =0x%lx", mod_name, kp->name, *dcp->bits);
vpr_cm_info(map, " %s maps ", mod_name);
ddebug_sync_classbits(kp, mod_name);
}
}
-static void ddebug_apply_params(const struct _ddebug_class_map *cm, const char *mod_name)
+static void ddebug_apply_params(struct _ddebug_class_map *cm, const char *mod_name)
{
const struct kernel_param *kp;
#if IS_ENABLED(CONFIG_MODULES)
@@ -1275,6 +1343,13 @@ static void ddebug_apply_params(const struct _ddebug_class_map *cm, const char *
}
}
+/*
+ * called from add_module, ie early. it can find controlling kparams,
+ * which can/does? enable protection of this classmap from class-less
+ * queries, on the grounds that the user created the kparam, means to
+ * use it, and expects it to reflect reality. We should oblige him,
+ * and protect those classmaps from classless "-p" changes.
+ */
static void ddebug_apply_class_maps(const struct _ddebug_info *di)
{
struct _ddebug_class_map *cm;
--
2.51.1
On 11/18/25 3:18 PM, Jim Cromie wrote:
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
> This message came from outside your organization.
> |-------------------------------------------------------------------!
>
> classmap-v1 code protected class'd pr_debugs from unintended
> changes by unclassed/_DFLT queries:
>
> # - to declutter examples:
> alias ddcmd='echo $* > /proc/dynamic_debug/control'
>
> # IOW, this should NOT alter drm.debug settings
> ddcmd -p
>
> # Instead, you must name the class to change it.
> # Protective but tedious
> ddcmd class DRM_UT_CORE +p
>
> # Or do it the (old school) subsystem way
> # This is ABI !!
> echo 1 > /sys/module/drm/parameters/debug
>
> Since the debug sysfs-node is ABI, if dyndbg is going to implement it,
> it must also honor its settings; it must at least protect against
> accidental changes to its classes from legacy queries.
>
> The protection allows all previously conceived queries to work the way
> they always have; ie select the same set of pr_debugs, despite the
> inclusion of whole new classes of pr_debugs.
>
> But that choice has 2 downsides:
>
> 1. "name the class to change it" makes a tedious long-winded
> interface, needing many commands to set DRM_UT_* one at a time.
>
> 2. It makes the class keyword special in some sense; the other
> keywords skip only on query mismatch, otherwise the code falls thru to
> adjust the pr-debug site.
>
> Jason Baron didn't like v1 on point 2.
> Louis Chauvet didn't like recent rev on point 1 tedium.
>
> But that said: /sys/ is ABI, so this must be reliable:
>
> #> echo 0x1f > /sys/module/drm/parameters/debug
>
> It 'just works' without dyndbg underneath; we must deliver that same
> stability. Convenience is secondary.
>
> The new resolution:
>
> If ABI is the blocking issue, then no ABI means no blocking issue.
> IOW, if the classmap has no presence under /sys/*, ie no PARAM, there
> is no ABI to guard, and no reason to enforce a tedious interface.
>
> In the future, if DRM wants to alter this protection, that is
> practical, but I think default-on is the correct mode.
>
> So atm classes without a PARAM are unprotected at >control, allowing
> admins their shortcuts. I think this could satisfy all viewpoints.
>
> That said, theres also a possibility of wildcard classes:
>
> #> ddcmd class '*' +p
>
> Currently, the query-class is exact-matched against each module's
> classmaps.names. This gives precise behavior, a good basis.
>
> But class wildcards are possible, they just did'nt appear useful for
> DRM, whose classmap names are a flat DRM_UT_* namespace.
>
> IOW, theres no useful selectivity there:
>
> #> ddcmd class "DRM_*" +p # these enable every DRM_* class
> #> ddcmd class "DRM_UT_*" +p
>
> #> ddcmd class "DRM_UT_V*" +p # finally select just 1: DRM_UT_VBL
> #> ddcmd class "DRM_UT_D*" +p # but this gets 3
>
> #> ddcmd class "D*V*" +p # here be dragons
>
> But there is debatable utility in the feature.
>
> #> ddcmd class __DEFAULT__ -p # what about this ?
> #> ddcmd -p # thats what this does. automatically
>
> Anyway, this patch does:
>
> 1. adds link field from _ddebug_class_map to the .controlling_param
>
> 2. sets it in ddebug_match_apply_kparam(), during modprobe/init,
> when options like drm.debug=VAL are handled.
>
> 3. ddebug_class_has_param() now checks .controlling_param
>
> 4. ddebug_class_wants_protection() macro renames 3.
> this frames it as a separable policy decision
>
> 5. ddebug_match_desc() gets the most attention:
>
> a. move classmap consideration to the bottom
> this insures all other constraints act 1st.
> allows simpler 'final' decisions.
>
> b. split class choices cleanly on query:
> class FOO vs none, and class'd vs _DPRINTK_CLASS_DFLT site.
>
> c. calls 4 when applying a class-less query to a class'd pr_debug
> here we need a new fn to find the classmap with this .class_id
>
> d. calls new ddebug_find_classmap_by_class_id().
> when class-less query looks at a class'd pr_debug.
> finds classmap, which can then decide, currently by PARAM existence.
>
> NOTES:
>
> protection is only against class-less queries, explicit "class FOO"
> adjustments are allowed (that is the mechanism).
>
> The drm.debug sysfs-node heavily under-specifies the class'd pr_debugs
> it controls; none of the +mfls prefixing flags have any effect, and
> each callsite remains individually controllable. drm.debug just
> toggles the +p flag for all the modules' class'd pr_debugs.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> history
> -v0 - original, before classmaps: no special case keywords
> -v1 - "class DEFAULT" is assumed if not mentioned.
> this protects classes from class-less queries
>
> -v2.pre-this-patch - protection macro'd to false
> -v2.with-this-patch - sysfs knob decides
> -v2.speculative - module decides wrt classmap protection
> seems unneeded now, TBD
>
> v3 - new patch
> v4
> - drop fn-scope map var, with 2 local vars, renamed to purpose
> - fix for NULL ptr case.
> - Add loop-var to reduce many "&dt->info." exprs to "di->"
> - add 1-liner postcondition comments
>
> fixus
> ---
> include/linux/dynamic_debug.h | 14 ++--
> lib/dynamic_debug.c | 127 +++++++++++++++++++++++++++-------
> 2 files changed, 110 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index b1d11d946780..b22da40e2583 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -75,6 +75,7 @@ enum ddebug_class_map_type {
> * map @class_names 0..N to consecutive constants starting at @base.
> */
> struct _ddebug_class_map {
> + struct _ddebug_class_param *controlling_param;
> const struct module *mod; /* NULL for builtins */
> const char *mod_name;
> const char **class_names;
> @@ -259,7 +260,12 @@ struct _ddebug_class_param {
> *
> * Creates a sysfs-param to control the classes defined by the
> * exported classmap, with bits 0..N-1 mapped to the classes named.
> - * This version keeps class-state in a private long int.
> + *
> + * Since sysfs-params are ABI, this also protects the classmap'd
> + * pr_debugs from un-class'd `echo -p > /proc/dynamic_debug/control`
> + * changes.
> + *
> + * This keeps class-state in a private long int.
> */
> #define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags) \
> static unsigned long _name##_bvec; \
> @@ -272,10 +278,8 @@ struct _ddebug_class_param {
> * @_var: name of the (exported) classmap var defining the classes/bits
> * @_flags: flags to be toggled, typically just 'p'
> *
> - * Creates a sysfs-param to control the classes defined by the
> - * exported clasmap, with bits 0..N-1 mapped to the classes named.
> - * This version keeps class-state in user @_bits. This lets drm check
> - * __drm_debug elsewhere too.
> + * Like DYNAMIC_DEBUG_CLASSMAP_PARAM, but maintains param-state in
> + * extern @_bits. This lets DRM check __drm_debug elsewhere too.
> */
> #define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) \
> __DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 636a6b5741f7..1082e0273f0e 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -206,6 +206,50 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
> return NULL;
> }
>
> +static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
> +{
> + return (class_id >= map->base &&
> + class_id < map->base + map->length);
> +}
> +
> +static struct _ddebug_class_map *
> +ddebug_find_map_by_class_id(struct _ddebug_info *di, int class_id)
> +{
> + struct _ddebug_class_map *map;
> + struct _ddebug_class_user *cli;
> + int i;
> +
> + for_subvec(i, map, di, maps)
> + if (ddebug_class_in_range(class_id, map))
> + return map;
> +
> + for_subvec(i, cli, di, users)
> + if (ddebug_class_in_range(class_id, cli->map))
> + return cli->map;
> +
> + return NULL;
> +}
> +
> +/*
> + * classmaps-V1 protected classes from changes by legacy commands
> + * (those selecting _DPRINTK_CLASS_DFLT by omission). This had the
> + * downside that saying "class FOO" for every change can get tedious.
> + *
> + * V2 is smarter, it protects class-maps if the defining module also
> + * calls DYNAMIC_DEBUG_CLASSMAP_PARAM to create a sysfs parameter.
> + * Since the author wants the knob, we should assume they intend to
> + * use it (in preference to "class FOO +p" >control), and want to
> + * trust its settings. This gives protection when its useful, and not
> + * when its just tedious.
> + */
> +static inline bool ddebug_class_has_param(const struct _ddebug_class_map *map)
> +{
> + return !!(map->controlling_param);
> +}
> +
> +/* re-framed as a policy choice */
> +#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map))
> +
> /*
> * Search the tables for _ddebug's which match the given `query' and
> * apply the `flags' and `mask' to them. Returns number of matching
> @@ -214,11 +258,10 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
> */
> static bool ddebug_match_desc(const struct ddebug_query *query,
> struct _ddebug *dp,
> - int valid_class)
> + struct _ddebug_info *di,
> + int selected_class)
> {
> - /* match site against query-class */
> - if (dp->class_id != valid_class)
> - return false;
> + struct _ddebug_class_map *site_map;
>
> /* match against the source filename */
> if (query->filename &&
> @@ -255,7 +298,28 @@ static bool ddebug_match_desc(const struct ddebug_query *query,
> dp->lineno > query->last_lineno)
> return false;
>
> - return true;
> + /*
> + * above are all satisfied, so we can make final decisions:
> + * 1- class FOO or implied class __DEFAULT__
> + * 2- site.is_classed or not
> + */
> + if (query->class_string) {
> + /* class FOO given, exact match required */
> + return (dp->class_id == selected_class);
> + }
> + /* query class __DEFAULT__ by omission. */
> + if (dp->class_id == _DPRINTK_CLASS_DFLT) {
> + /* un-classed site */
> + return true;
> + }
> + /* site is class'd */
> + site_map = ddebug_find_map_by_class_id(di, dp->class_id);
> + if (!site_map) {
> + /* _UNKNOWN_ class_id. XXX: Allow changes here ? */
> + return false;
> + }
Do we want a WARN_ON_ONCE() here? I think this is the case where we have
class_id for the call site but it's not default, so shouldn't it always
have a map or be a user of the map?
Thanks,
-Jason
On Fri, Dec 12, 2025 at 10:26 AM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 11/18/25 3:18 PM, Jim Cromie wrote:
> > !-------------------------------------------------------------------|
> > This Message Is From an External Sender
> > This message came from outside your organization.
> > |-------------------------------------------------------------------!
> >
> > classmap-v1 code protected class'd pr_debugs from unintended
> > changes by unclassed/_DFLT queries:
> >
> > # - to declutter examples:
> > alias ddcmd='echo $* > /proc/dynamic_debug/control'
> >
> > # IOW, this should NOT alter drm.debug settings
> > ddcmd -p
> >
> > # Instead, you must name the class to change it.
> > # Protective but tedious
> > ddcmd class DRM_UT_CORE +p
> >
> > # Or do it the (old school) subsystem way
> > # This is ABI !!
> > echo 1 > /sys/module/drm/parameters/debug
> >
> > Since the debug sysfs-node is ABI, if dyndbg is going to implement it,
> > it must also honor its settings; it must at least protect against
> > accidental changes to its classes from legacy queries.
> >
> > The protection allows all previously conceived queries to work the way
> > they always have; ie select the same set of pr_debugs, despite the
> > inclusion of whole new classes of pr_debugs.
> >
> > But that choice has 2 downsides:
> >
> > 1. "name the class to change it" makes a tedious long-winded
> > interface, needing many commands to set DRM_UT_* one at a time.
> >
> > 2. It makes the class keyword special in some sense; the other
> > keywords skip only on query mismatch, otherwise the code falls thru to
> > adjust the pr-debug site.
> >
> > Jason Baron didn't like v1 on point 2.
> > Louis Chauvet didn't like recent rev on point 1 tedium.
> >
> > But that said: /sys/ is ABI, so this must be reliable:
> >
> > #> echo 0x1f > /sys/module/drm/parameters/debug
> >
> > It 'just works' without dyndbg underneath; we must deliver that same
> > stability. Convenience is secondary.
> >
> > The new resolution:
> >
> > If ABI is the blocking issue, then no ABI means no blocking issue.
> > IOW, if the classmap has no presence under /sys/*, ie no PARAM, there
> > is no ABI to guard, and no reason to enforce a tedious interface.
> >
> > In the future, if DRM wants to alter this protection, that is
> > practical, but I think default-on is the correct mode.
> >
> > So atm classes without a PARAM are unprotected at >control, allowing
> > admins their shortcuts. I think this could satisfy all viewpoints.
> >
> > That said, theres also a possibility of wildcard classes:
> >
> > #> ddcmd class '*' +p
> >
> > Currently, the query-class is exact-matched against each module's
> > classmaps.names. This gives precise behavior, a good basis.
> >
> > But class wildcards are possible, they just did'nt appear useful for
> > DRM, whose classmap names are a flat DRM_UT_* namespace.
> >
> > IOW, theres no useful selectivity there:
> >
> > #> ddcmd class "DRM_*" +p # these enable every DRM_* class
> > #> ddcmd class "DRM_UT_*" +p
> >
> > #> ddcmd class "DRM_UT_V*" +p # finally select just 1: DRM_UT_VBL
> > #> ddcmd class "DRM_UT_D*" +p # but this gets 3
> >
> > #> ddcmd class "D*V*" +p # here be dragons
> >
> > But there is debatable utility in the feature.
> >
> > #> ddcmd class __DEFAULT__ -p # what about this ?
> > #> ddcmd -p # thats what this does. automatically
> >
> > Anyway, this patch does:
> >
> > 1. adds link field from _ddebug_class_map to the .controlling_param
> >
> > 2. sets it in ddebug_match_apply_kparam(), during modprobe/init,
> > when options like drm.debug=VAL are handled.
> >
> > 3. ddebug_class_has_param() now checks .controlling_param
> >
> > 4. ddebug_class_wants_protection() macro renames 3.
> > this frames it as a separable policy decision
> >
> > 5. ddebug_match_desc() gets the most attention:
> >
> > a. move classmap consideration to the bottom
> > this insures all other constraints act 1st.
> > allows simpler 'final' decisions.
> >
> > b. split class choices cleanly on query:
> > class FOO vs none, and class'd vs _DPRINTK_CLASS_DFLT site.
> >
> > c. calls 4 when applying a class-less query to a class'd pr_debug
> > here we need a new fn to find the classmap with this .class_id
> >
> > d. calls new ddebug_find_classmap_by_class_id().
> > when class-less query looks at a class'd pr_debug.
> > finds classmap, which can then decide, currently by PARAM existence.
> >
> > NOTES:
> >
> > protection is only against class-less queries, explicit "class FOO"
> > adjustments are allowed (that is the mechanism).
> >
> > The drm.debug sysfs-node heavily under-specifies the class'd pr_debugs
> > it controls; none of the +mfls prefixing flags have any effect, and
> > each callsite remains individually controllable. drm.debug just
> > toggles the +p flag for all the modules' class'd pr_debugs.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> > history
> > -v0 - original, before classmaps: no special case keywords
> > -v1 - "class DEFAULT" is assumed if not mentioned.
> > this protects classes from class-less queries
> >
> > -v2.pre-this-patch - protection macro'd to false
> > -v2.with-this-patch - sysfs knob decides
> > -v2.speculative - module decides wrt classmap protection
> > seems unneeded now, TBD
> >
> > v3 - new patch
> > v4
> > - drop fn-scope map var, with 2 local vars, renamed to purpose
> > - fix for NULL ptr case.
> > - Add loop-var to reduce many "&dt->info." exprs to "di->"
> > - add 1-liner postcondition comments
> >
> > fixus
> > ---
> > include/linux/dynamic_debug.h | 14 ++--
> > lib/dynamic_debug.c | 127 +++++++++++++++++++++++++++-------
> > 2 files changed, 110 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > index b1d11d946780..b22da40e2583 100644
> > --- a/include/linux/dynamic_debug.h
> > +++ b/include/linux/dynamic_debug.h
> > @@ -75,6 +75,7 @@ enum ddebug_class_map_type {
> > * map @class_names 0..N to consecutive constants starting at @base.
> > */
> > struct _ddebug_class_map {
> > + struct _ddebug_class_param *controlling_param;
> > const struct module *mod; /* NULL for builtins */
> > const char *mod_name;
> > const char **class_names;
> > @@ -259,7 +260,12 @@ struct _ddebug_class_param {
> > *
> > * Creates a sysfs-param to control the classes defined by the
> > * exported classmap, with bits 0..N-1 mapped to the classes named.
> > - * This version keeps class-state in a private long int.
> > + *
> > + * Since sysfs-params are ABI, this also protects the classmap'd
> > + * pr_debugs from un-class'd `echo -p > /proc/dynamic_debug/control`
> > + * changes.
> > + *
> > + * This keeps class-state in a private long int.
> > */
> > #define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags) \
> > static unsigned long _name##_bvec; \
> > @@ -272,10 +278,8 @@ struct _ddebug_class_param {
> > * @_var: name of the (exported) classmap var defining the classes/bits
> > * @_flags: flags to be toggled, typically just 'p'
> > *
> > - * Creates a sysfs-param to control the classes defined by the
> > - * exported clasmap, with bits 0..N-1 mapped to the classes named.
> > - * This version keeps class-state in user @_bits. This lets drm check
> > - * __drm_debug elsewhere too.
> > + * Like DYNAMIC_DEBUG_CLASSMAP_PARAM, but maintains param-state in
> > + * extern @_bits. This lets DRM check __drm_debug elsewhere too.
> > */
> > #define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) \
> > __DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 636a6b5741f7..1082e0273f0e 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -206,6 +206,50 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
> > return NULL;
> > }
> >
> > +static bool ddebug_class_in_range(const int class_id, const struct _ddebug_class_map *map)
> > +{
> > + return (class_id >= map->base &&
> > + class_id < map->base + map->length);
> > +}
> > +
> > +static struct _ddebug_class_map *
> > +ddebug_find_map_by_class_id(struct _ddebug_info *di, int class_id)
> > +{
> > + struct _ddebug_class_map *map;
> > + struct _ddebug_class_user *cli;
> > + int i;
> > +
> > + for_subvec(i, map, di, maps)
> > + if (ddebug_class_in_range(class_id, map))
> > + return map;
> > +
> > + for_subvec(i, cli, di, users)
> > + if (ddebug_class_in_range(class_id, cli->map))
> > + return cli->map;
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * classmaps-V1 protected classes from changes by legacy commands
> > + * (those selecting _DPRINTK_CLASS_DFLT by omission). This had the
> > + * downside that saying "class FOO" for every change can get tedious.
> > + *
> > + * V2 is smarter, it protects class-maps if the defining module also
> > + * calls DYNAMIC_DEBUG_CLASSMAP_PARAM to create a sysfs parameter.
> > + * Since the author wants the knob, we should assume they intend to
> > + * use it (in preference to "class FOO +p" >control), and want to
> > + * trust its settings. This gives protection when its useful, and not
> > + * when its just tedious.
> > + */
> > +static inline bool ddebug_class_has_param(const struct _ddebug_class_map *map)
> > +{
> > + return !!(map->controlling_param);
> > +}
> > +
> > +/* re-framed as a policy choice */
> > +#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map))
> > +
> > /*
> > * Search the tables for _ddebug's which match the given `query' and
> > * apply the `flags' and `mask' to them. Returns number of matching
> > @@ -214,11 +258,10 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
> > */
> > static bool ddebug_match_desc(const struct ddebug_query *query,
> > struct _ddebug *dp,
> > - int valid_class)
> > + struct _ddebug_info *di,
> > + int selected_class)
> > {
> > - /* match site against query-class */
> > - if (dp->class_id != valid_class)
> > - return false;
> > + struct _ddebug_class_map *site_map;
> >
> > /* match against the source filename */
> > if (query->filename &&
> > @@ -255,7 +298,28 @@ static bool ddebug_match_desc(const struct ddebug_query *query,
> > dp->lineno > query->last_lineno)
> > return false;
> >
> > - return true;
> > + /*
> > + * above are all satisfied, so we can make final decisions:
> > + * 1- class FOO or implied class __DEFAULT__
> > + * 2- site.is_classed or not
> > + */
> > + if (query->class_string) {
> > + /* class FOO given, exact match required */
> > + return (dp->class_id == selected_class);
> > + }
> > + /* query class __DEFAULT__ by omission. */
> > + if (dp->class_id == _DPRINTK_CLASS_DFLT) {
> > + /* un-classed site */
> > + return true;
> > + }
> > + /* site is class'd */
> > + site_map = ddebug_find_map_by_class_id(di, dp->class_id);
> > + if (!site_map) {
> > + /* _UNKNOWN_ class_id. XXX: Allow changes here ? */
> > + return false;
> > + }
>
> Do we want a WARN_ON_ONCE() here? I think this is the case where we have
> class_id for the call site but it's not default, so shouldn't it always
> have a map or be a user of the map?
>
Yes, I think so.
I will add it.
> Thanks,
>
> -Jason
>
© 2016 - 2026 Red Hat, Inc.