linux-kernel - Re: [PATCH v1 11/12] dyndbg: write debug logs to trace instance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8Bye+yYQZ6UGYB6sTaJpwRnhHYRF=qdz7FC5bMA4zqzCEgOw@mail.gmail.com>
Date:   Tue, 14 Nov 2023 23:09:41 +0100
From:   Łukasz Bartosik <lb@...ihalf.com>
To:     jim.cromie@...il.com
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Jason Baron <jbaron@...mai.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Guenter Roeck <groeck@...gle.com>,
        Yaniv Tzoreff <yanivt@...gle.com>,
        Benson Leung <bleung@...gle.com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>,
        Pekka Paalanen <ppaalanen@...il.com>,
        Sean Paul <seanpaul@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org,
        upstream@...ihalf.com
Subject: Re: [PATCH v1 11/12] dyndbg: write debug logs to trace instance

wt., 14 lis 2023 o 16:41 <jim.cromie@...il.com> napisał(a):
>
> On Tue, Nov 14, 2023 at 12:45 AM Łukasz Bartosik <lb@...ihalf.com> wrote:
> >
> > wt., 14 lis 2023 o 02:08 <jim.cromie@...il.com> napisał(a):
> > >
>
> > > > > > Also I think we need a reserved keyword for writing debug logs to
> > > > > > trace events - maybe "event" keyword ?
> > > > >
> > > > > do you mean "event" as a selector, like module, function, class, etc ?
> > > > > if so, what are the values ?
> > > > > any event under  /sys/kernel/debug/tracing/events/ ?
> > > > >
> > > > > how does this get used ?
> > > > >
> > > >
> > > > I meant that we need to reserve name/keyword to enable writing debug
> > > > logs to trace events (prdbg and devdbg), for example
> > > > echo module usbcore +T:event > /proc/dynamic_debug/control
> > > >
> > > > Or do you anticipate other way to do it ?
> > >
> > > way back, when I had even fewer clues,
> > > I sent patches to call trace-printk when +T was set.
> > > Steve didnt like it, I think cuz it could flood the tracebuf.
> > >
> > > Thats why I added the prdbg and devdbg event-types,
> > > so that they could be disabled easily using /sys/kernel/debug/tracing/
> > > putting them squarely under trace-control.
> > >
> > > Note that this puts 2 off-switches in series,
> > > both tracefs and >control can disable all the pr_debug traffic,
> > > tracefs by event-type, and >control at individual callsite level.
> > >
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable
> > >
> > > I briefly thought about linking the 2 off-switches,
> > > but punted cuz I thought it complicated things,
> > > (how exactly would they get coupled?)
> > > and I didnt want to distract from larger goals
> > >
> > > Does that address your question ?
> > >
> >
> > Jim,
> >
> > Thanks but it doesn't answer my question.
> >
> > How do you plan to enable output to tracefs event at a callsite level ?
> >
> > In my original proposal it was enabled by setting trace destination to
> > 0. Since we are moving to names instead of numbers now I guess we need
> > to reserve a name for it not to clash with trace instance names
> > provided by users. That's why I proposed to reserve name "event" for
> > that purpose and be used as +T:event.
> >
>
> Ok, I got your point now.
>
> how about we call it "0" ?
> it should be an obvious "magical" value,
> cuz it doesnt need to be open'd, and cant be close'd
>
> then we can revert to global tracebuf by its "name"
> echo module foo +T:0 > /proc/dynamic_debug/control
>

I like it. It resembles surprise emoji :0

> we probably should also limit the trace-instance-names to ^\w+
>

Ack

I think this closes the last open topic to discuss for now.
I will get back with next patchset version to you soon.

> > Or did I miss answer to that in our long discussion :> ?
>
> nope :-)
>
> thanks,
> Jim
>
> >
> > Thanks,
> > Lukasz
> >
> > > On a related point, I also added drm_dbg and drm_devdbg.
> > > Those are issued from __drm_dbg & __drm_dev_dbg
> > >  respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.
> > >
> > > Im not sure theyre more useful than confusing yet.
> > >
> > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > > > > the existing setting)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Sounds good.
> > > > > > > >
> > > > > > >
> > > > > > > :-)