Message-ID: <CAJfuBxwPBG2yOEWjf_giWGPmfNPr2nHxncah=oyF7ChXzbRdZg@mail.gmail.com>
Date: Mon, 15 Dec 2025 07:04:11 +1300
From: jim.cromie@...il.com
To: Jason Baron <jbaron@...mai.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	gregkh@...uxfoundation.org, ukaszb@...omium.org, louis.chauvet@...tlin.com
Subject: Re: [PATCH v6 29/31] dyndbg: resolve "protection" of class'd pr_debug

On Fri, Dec 12, 2025 at 10:26 AM Jason Baron <jbaron@...mai.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@...il.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
>

Powered by blists - more mailing lists