Re: [PATCH v6 17/57] dyndbg: validate class FOO by checking with module

[Jason Baron]

On 9/9/22 16:44, jim.cromie@xxxxxxxxx wrote:
> 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.
> 

I sort of get that your trying to protect these from unintended toggling,
but I would say it's that's not really new with these statements,
prr_debug() come and go before and I'm not aware of this is an issue.
And in any case, a query can be modified.

I think what bugs me is now query stuff works differently. Previously,
all the query strings - 'module', 'file', 'line', 'format', were
used as additional selectors, but now we have this new one 'class'
that works differently as it's requited for pr_debug_cls() statements.

> 
> 
>> 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 ?

'frame-of-reference' I'm questioning the how 'class' works as mentioned
above not the implementation.

Thanks,

-Jason

> 
> 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;
>>>  }