From: Jason Baron <jbaron@akamai.com>
To: jim.cromie@gmail.com
Cc: Greg KH <gregkh@linuxfoundation.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
amd-gfx mailing list <amd-gfx@lists.freedesktop.org>,
intel-gvt-dev@lists.freedesktop.org,
Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
LKML <linux-kernel@vger.kernel.org>,
Daniel Vetter <daniel.vetter@ffwll.ch>,
Sean Paul <seanpaul@chromium.org>,
robdclark@gmail.com, Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Joe Perches <joe@perches.com>
Subject: Re: [PATCH v6 17/57] dyndbg: validate class FOO by checking with module
Date: Mon, 12 Sep 2022 16:17:00 -0400 [thread overview]
Message-ID: <0d9f644f-3d60-02c3-7ce0-01296757e181@akamai.com> (raw)
In-Reply-To: <CAJfuBxzM=KKPbcks-aQLAJM0QVd5sjL-CucYbyFbeG5sgoCVjg@mail.gmail.com>
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;
>>> }
next prev parent reply other threads:[~2022-09-12 20:17 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-04 21:40 [PATCH v6 00/57] DYNDBG: opt-in class'd debug for modules, use in drm Jim Cromie
2022-09-04 21:40 ` [PATCH v6 01/57] dyndbg: fix static_branch manipulation Jim Cromie
2022-09-04 21:40 ` [PATCH v6 02/57] dyndbg: fix module.dyndbg handling Jim Cromie
2022-09-04 21:40 ` [PATCH v6 03/57] dyndbg: show both old and new in change-info Jim Cromie
2022-09-04 21:40 ` [PATCH v6 04/57] dyndbg: reverse module walk in cat control Jim Cromie
2022-09-04 21:40 ` [PATCH v6 05/57] dyndbg: reverse module.callsite " Jim Cromie
2022-09-04 21:40 ` [PATCH v6 06/57] dyndbg: use ESCAPE_SPACE for " Jim Cromie
2022-09-04 21:40 ` [PATCH v6 07/57] dyndbg: let query-modname override actual module name Jim Cromie
2022-09-04 21:40 ` [PATCH v6 08/57] dyndbg: add test_dynamic_debug module Jim Cromie
2022-09-04 21:40 ` [PATCH v6 09/57] dyndbg: drop EXPORTed dynamic_debug_exec_queries Jim Cromie
2022-09-04 21:40 ` [PATCH v6 10/57] dyndbg: cleanup auto vars in dynamic_debug_init Jim Cromie
2022-09-07 14:10 ` Jason Baron
2022-09-04 21:40 ` [PATCH v6 11/57] dyndbg: gather __dyndbg[] state into struct _ddebug_info Jim Cromie
2022-09-04 21:40 ` [PATCH v6 12/57] dyndbg: add class_id to pr_debug callsites Jim Cromie
2022-09-04 21:40 ` [PATCH v6 13/57] dyndbg: add __pr_debug_cls for testing Jim Cromie
2022-09-04 21:40 ` [PATCH v6 14/57] dyndbg: add DECLARE_DYNDBG_CLASSMAP macro Jim Cromie
2022-09-04 21:40 ` [PATCH v6 15/57] kernel/module: add __dyndbg_classes section Jim Cromie
2022-09-04 21:40 ` [PATCH v6 16/57] dyndbg: add ddebug_attach_module_classes Jim Cromie
2022-09-04 21:40 ` [PATCH v6 17/57] dyndbg: validate class FOO by checking with module Jim Cromie
2022-09-07 18:19 ` Jason Baron
2022-09-09 20:44 ` jim.cromie
2022-09-12 20:17 ` Jason Baron [this message]
2022-09-12 22:08 ` jim.cromie
2022-09-04 21:40 ` [PATCH v6 18/57] doc-dyndbg: describe "class CLASS_NAME" query support Jim Cromie
2022-09-04 21:40 ` [PATCH v6 19/57] doc-dyndbg: edit dynamic-debug-howto for brevity, audience Jim Cromie
2022-09-04 21:40 ` [PATCH v6 20/57] dyndbg: add drm.debug style (drm/parameters/debug) bitmap support Jim Cromie
2022-09-04 21:40 ` [PATCH v6 21/57] dyndbg: test DECLARE_DYNDBG_CLASSMAP, sysfs nodes Jim Cromie
2022-09-07 14:54 ` Greg KH
2022-09-07 14:57 ` Greg KH
2022-09-04 21:40 ` [PATCH v6 22/57] drm_print: condense enum drm_debug_category Jim Cromie
2022-09-07 6:09 ` Daniel Vetter
2022-09-04 21:41 ` [PATCH v6 23/57] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers Jim Cromie
2022-09-07 6:13 ` Daniel Vetter
2022-09-09 19:06 ` jim.cromie
2022-09-04 21:41 ` [PATCH v6 24/57] drm_print: interpose drm_*dbg with forwarding macros Jim Cromie
2022-09-04 21:41 ` [PATCH v6 25/57] drm_print: wrap drm_*_dbg in dyndbg descriptor factory macro Jim Cromie
2022-09-04 21:41 ` [PATCH v6 26/57] drm-print.h: include dyndbg header Jim Cromie
2022-09-04 21:41 ` [PATCH v6 27/57] drm-print: add drm_dbg_driver to improve namespace symmetry Jim Cromie
2022-09-04 21:41 ` [PATCH v6 28/57] drm_print: refine drm_debug_enabled for jump-label Jim Cromie
2022-09-07 6:40 ` Daniel Vetter
2022-09-09 23:42 ` jim.cromie
2022-09-04 21:41 ` [PATCH v6 29/57] drm_print: prefer bare printk KERN_DEBUG on generic fn Jim Cromie
2022-09-04 21:41 ` [PATCH v6 30/57] drm_print: add _ddebug descriptor to drm_*dbg prototypes Jim Cromie
2022-09-04 21:41 ` [PATCH v6 31/57] nouveau: change nvkm_debug/trace to use dev_dbg POC Jim Cromie
2022-09-04 21:41 ` [PATCH v6 32/57] nouveau: adapt NV_DEBUG, NV_ATOMIC to use DRM.debug Jim Cromie
2023-03-06 18:49 ` Timur Tabi
2023-03-07 5:10 ` jim.cromie
2022-09-04 21:41 ` [PATCH v6 33/57] nouveau: WIP add 2 LEVEL_NUM classmaps for CLI, SUBDEV Jim Cromie
2022-09-04 21:41 ` [PATCH v6 34/57] dyndbg: add _DPRINTK_FLAGS_ENABLED Jim Cromie
2022-09-04 21:41 ` [PATCH v6 35/57] dyndbg: add _DPRINTK_FLAGS_TRACE Jim Cromie
2022-09-04 21:41 ` [PATCH v6 36/57] dyndbg: add write-events-to-tracefs code Jim Cromie
2022-09-04 21:41 ` [PATCH v6 37/57] dyndbg: add 2 trace-events: drm_debug, drm_devdbg Jim Cromie
2022-09-04 21:41 ` [PATCH v6 38/57] dyndbg: add 2 more trace-events: pr_debug, dev_dbg Jim Cromie
2022-09-04 21:41 ` [PATCH v6 39/57] dyndbg/drm: POC add tracebits sysfs-knob Jim Cromie
2022-09-07 6:59 ` Daniel Vetter
2022-09-04 21:41 ` [PATCH v6 40/57] dyndbg: abstraction macros for modname,function,filename fields Jim Cromie
2022-09-04 21:41 ` [PATCH v6 41/57] dyndbg: split repeating columns to new struct _ddebug_site Jim Cromie
2022-09-04 21:41 ` [PATCH v6 42/57] dyndbg: shrink lineno field by 2 bits Jim Cromie
2022-09-04 21:41 ` [PATCH v6 43/57] dyndbg: add _index,_map to struct _ddebug Jim Cromie
2022-09-04 21:41 ` [PATCH v6 44/57] dyndbg: extend __ddebug_add_module proto to allow packing sites Jim Cromie
2022-09-04 21:41 ` [PATCH v6 45/57] dyndbg: de-duplicate sites Jim Cromie
2022-09-04 21:41 ` [PATCH v6 46/57] dyndbg: drop site-> in add-module, more needed Jim Cromie
2022-09-04 21:41 ` [PATCH v6 47/57] dyndbg: demote iter->site in _init Jim Cromie
2022-09-04 21:41 ` [PATCH v6 48/57] dyndbg: add .gnu.linkonce slot in vmlinux.lds.h KEEPs Jim Cromie
2022-09-04 21:41 ` [PATCH v6 49/57] dyndbg: add structs _ddebug_hdr, _ddebug_site_hdr Jim Cromie
2022-09-04 21:41 ` [PATCH v6 50/57] dyndbg: count unique callsites Jim Cromie
2022-09-04 21:41 ` [PATCH v6 51/57] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE Jim Cromie
2022-09-04 21:41 ` [PATCH v6 52/57] dyndbg: add DEFINE_DYNAMIC_DEBUG_TABLE, use it tacitly RFC Jim Cromie
2022-09-04 21:41 ` [PATCH v6 53/57] dyndbg: add/use is_dyndbg_header then set _uplink Jim Cromie
2022-09-04 21:41 ` [PATCH v6 54/57] dyndbg: add .gnu.linkonce. & __dyndbg* sections in module.lds.h Jim Cromie
2022-09-04 21:41 ` [PATCH v6 55/57] dyndbg: dynamic_debug_sites_reclaim() using free_reserved_page() WAG Jim Cromie
2022-09-04 21:41 ` [PATCH v6 56/57] dyndbg: work ddebug_map_site Jim Cromie
2022-09-04 21:41 ` [PATCH v6 57/57] dyndbg: fiddle with readback value on LEVEL_NAMES types Jim Cromie
2022-09-07 15:08 ` [PATCH v6 00/57] DYNDBG: opt-in class'd debug for modules, use in drm Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0d9f644f-3d60-02c3-7ce0-01296757e181@akamai.com \
--to=jbaron@akamai.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-gvt-dev@lists.freedesktop.org \
--cc=jim.cromie@gmail.com \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=robdclark@gmail.com \
--cc=seanpaul@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).