On Wed, Sep 7, 2022 at 12:19 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> On 9/4/22 17:40, Jim Cromie wrote:
> > Add module-to-class validation:
> >
> > #> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control
> >
> > If a query has "class FOO", then ddebug_find_valid_class(), called
> > from ddebug_change(), requires that FOO is known to module X,
> > otherwize the query is skipped entirely for X. This protects each
> > module's class-space, other than the default:31.
> >
> > The authors' choice of FOO is highly selective, giving isolation
> > and/or coordinated sharing of FOOs. For example, only DRM modules
> > should know and respond to DRM_UT_KMS.
> >
> > So this, combined with module's opt-in declaration of known classes,
> > effectively privatizes the .class_id space for each module (or
> > coordinated set of modules).
> >
> > Notes:
> >
> > For all "class FOO" queries, ddebug_find_valid_class() is called, it
> > returns the map matching the query, and sets valid_class via an
> > *outvar).
> >
> > If no "class FOO" is supplied, valid_class = _CLASS_DFLT. This
> > insures that legacy queries do not trample on new class'd callsites,
> > as they get added.
>
>
> Hi Jim,
>
> I'm wondering about the case where we have a callsite which is marked
> as 'class foo', but the query string is done by say module and file, so:
>
> # echo "module bar file foo.c +p" > /proc/dynamic_debug_control
>
> With the proposed code, I think this ends up not enabling anything right?
correct - the only way to enable : pr_debug_cls(CL_FOO, " ...")
is
echo class CL_FOO +p > control
1st, existing dyndbg query uses, whether ad-hoc or scripted,
were not written in anticipation of new / classified subsystems.
2nd, new class users dont want to sit in coach. no damn legroom.
3rd, consider DRM, which already has drm.debug
ie: /sys/module/drm/parameters/debug
and prefers it, at least by inertia.
protecting these new class'd callsites (3-5k of them)
from casual (unintended) manipulations of the kernel-wide
dyndbg state seems prudent, and a usability win.
Not everyone will use module bar, requiring "class foo"
guarantees that changes are intentional.
> Because valid class is set to _DPRINTK_CLASS_DFLT and then:
> 'dp->class_id != valid_class' is true?
>
> This seems confusing to me as a user as this doesn't work like the
> other queries....so maybe we should only do the
> 'dp->class_id != valid_class' check *if* query->class_string is set,
> see below.
>
Could you clarify whether you think this is a logic error
or a frame-of-reference difference as elaborated above ?
ISTM theres a place for a well-worded paragraph in doc
about the class distinction, perhaps a whole for-authors section.
>
>
> >
> > Also add a new column to control-file output, displaying non-default
> > class-name (when found) or the "unknown _id:", if it has not been
> > (correctly) declared with one of the declarator macros.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
> > ---
> > lib/dynamic_debug.c | 76 ++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 68 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index b71efd0b491d..db96ded78c3f 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -56,6 +56,7 @@ struct ddebug_query {
> > const char *module;
> > const char *function;
> > const char *format;
> > + const char *class_string;
> > unsigned int first_lineno, last_lineno;
> > };
> >
> > @@ -136,15 +137,33 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
> > fmtlen--;
> > }
> >
> > - v3pr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u\n",
> > - msg,
> > - query->function ?: "",
> > - query->filename ?: "",
> > - query->module ?: "",
> > - fmtlen, query->format ?: "",
> > - query->first_lineno, query->last_lineno);
> > + v3pr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u class=%s\n",
> > + msg,
> > + query->function ?: "",
> > + query->filename ?: "",
> > + query->module ?: "",
> > + fmtlen, query->format ?: "",
> > + query->first_lineno, query->last_lineno, query->class_string);
> > }
> >
> > +static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
> > + const char *class_string, int *class_id)
> > +{
> > + struct ddebug_class_map *map;
> > + int idx;
> > +
> > + list_for_each_entry(map, &dt->maps, link) {
> > + idx = match_string(map->class_names, map->length, class_string);
> > + if (idx >= 0) {
> > + *class_id = idx + map->base;
> > + return map;
> > + }
> > + }
> > + *class_id = -ENOENT;
> > + return NULL;
> > +}
> > +
> > +#define __outvar /* filled by callee */
> > /*
> > * Search the tables for _ddebug's which match the given `query' and
> > * apply the `flags' and `mask' to them. Returns number of matching
> > @@ -159,6 +178,8 @@ static int ddebug_change(const struct ddebug_query *query,
> > unsigned int newflags;
> > unsigned int nfound = 0;
> > struct flagsbuf fbuf, nbuf;
> > + struct ddebug_class_map *map = NULL;
> > + int __outvar valid_class;
> >
> > /* search for matching ddebugs */
> > mutex_lock(&ddebug_lock);
> > @@ -169,9 +190,22 @@ static int ddebug_change(const struct ddebug_query *query,
> > !match_wildcard(query->module, dt->mod_name))
> > continue;
> >
> > + if (query->class_string) {
> > + map = ddebug_find_valid_class(dt, query->class_string, &valid_class);
> > + if (!map)
> > + continue;
>
> So remove the else here.
>
> > + } else {
> > + /* constrain query, do not touch class'd callsites */
> > + valid_class = _DPRINTK_CLASS_DFLT;
> > + }
> > +
> > for (i = 0; i < dt->num_ddebugs; i++) {
> > struct _ddebug *dp = &dt->ddebugs[i];
> >
> > + /* match site against query-class */
> > + if (dp->class_id != valid_class)
>
> And then make this: if (query->class_string && (dp->class_id != valid_class))
>
> thoughts?
>
>
> > + continue;
> > +> /* match against the source filename */
> > if (query->filename &&
> > !match_wildcard(query->filename, dp->filename) &&
> > @@ -420,6 +454,8 @@ static int ddebug_parse_query(char *words[], int nwords,
> > } else if (!strcmp(keyword, "line")) {
> > if (parse_linerange(query, arg))
> > return -EINVAL;
> > + } else if (!strcmp(keyword, "class")) {
> > + rc = check_set(&query->class_string, arg, "class");
> > } else {
> > pr_err("unknown keyword \"%s\"\n", keyword);
> > return -EINVAL;
> > @@ -854,6 +890,20 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
> > return dp;
> > }
> >
> > +#define class_in_range(class_id, map) \
> > + (class_id >= map->base && class_id < map->base + map->length)
> > +
> > +static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug *dp)
> > +{
> > + struct ddebug_class_map *map;
> > +
> > + list_for_each_entry(map, &iter->table->maps, link)
> > + if (class_in_range(dp->class_id, map))
> > + return map->class_names[dp->class_id - map->base];
> > +
> > + return NULL;
> > +}
> > +
> > /*
> > * Seq_ops show method. Called several times within a read()
> > * call from userspace, with ddebug_lock held. Formats the
> > @@ -865,6 +915,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
> > struct ddebug_iter *iter = m->private;
> > struct _ddebug *dp = p;
> > struct flagsbuf flags;
> > + char const *class;
> >
> > if (p == SEQ_START_TOKEN) {
> > seq_puts(m,
> > @@ -877,7 +928,16 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
> > iter->table->mod_name, dp->function,
> > ddebug_describe_flags(dp->flags, &flags));
> > seq_escape_str(m, dp->format, ESCAPE_SPACE, "\t\r\n\"");
> > - seq_puts(m, "\"\n");
> > + seq_puts(m, "\"");
> > +
> > + if (dp->class_id != _DPRINTK_CLASS_DFLT) {
> > + class = ddebug_class_name(iter, dp);
> > + if (class)
> > + seq_printf(m, " class:%s", class);
> > + else
> > + seq_printf(m, " class unknown, _id:%d", dp->class_id);
> > + }
> > + seq_puts(m, "\n");
> >
> > return 0;
> > }
