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.
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@gmail.com>
---
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;
+ } 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)
+ 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;
}
--
2.37.2
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?
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.
>
> 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@gmail.com>
> ---
> 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;
> }
On Wed, Sep 7, 2022 at 12:19 PM Jason Baron <jbaron@akamai.com> 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@gmail.com>
> > ---
> > 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;
> > }
On 9/9/22 16:44, jim.cromie@gmail.com wrote:
> On Wed, Sep 7, 2022 at 12:19 PM Jason Baron <jbaron@akamai.com> 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@gmail.com>
>>> ---
>>> 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;
>>> }
On Mon, Sep 12, 2022 at 2:17 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 9/9/22 16:44, jim.cromie@gmail.com wrote:
> > On Wed, Sep 7, 2022 at 12:19 PM Jason Baron <jbaron@akamai.com> 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,
Whats new here is the customer - a whole new subsystem.
theyre accustomed to a single point of control,
/sys/module/drm/parameters/debug,
and dont particularly value the infinite variety of combos under the hood.
Theyre here for (Im selling them on) the JUMP_LABEL.
Isolation from unintended manipulations under the hood
(which is afterall a global kernel state) seems like a good guarantee.
IOW class isolation from non-class, and from other INDEPENDENT classes.
Only LEVEL types have any interdependence, and only with the other level
classnames in the declared group.
Ultimately, whats the client want ?
I know my banker handles other folks money too,
but I can reasonably expect isolation from their businesses.
> 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.
but youre requiring they be modified, so as not to scribble all over DRM_*,
which wasnt there when they fiddled.
> 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.
>
theres a "when-in-rome" argument here, if DRM is Rome.
> >
> >
> >> 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@gmail.com>
> >>> ---
> >>> 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;
> >>> }
© 2016 - 2026 Red Hat, Inc.