Messages in this thread | | | From | Łukasz Bartosik <> | Date | Sun, 12 Nov 2023 17:28:23 +0100 | Subject | Re: [PATCH v1 04/12] dyndbg: add 2 trace-events: pr_debug, dev_dbg |
| |
pt., 10 lis 2023 o 20:20 <jim.cromie@gmail.com> napisał(a): > > On Fri, Nov 10, 2023 at 7:51 AM Łukasz Bartosik <lb@semihalf.com> wrote: > > > > wt., 7 lis 2023 o 00:55 Steven Rostedt <rostedt@goodmis.org> napisał(a): > > > > > > On Fri, 3 Nov 2023 14:10:03 +0100 > > > Łukasz Bartosik <lb@semihalf.com> wrote: > > > > > > > +/* capture pr_debug() callsite descriptor and message */ > > > > +TRACE_EVENT(prdbg, > > > > + TP_PROTO(const struct _ddebug *desc, const char *text, size_t len), > > > > + > > > > + TP_ARGS(desc, text, len), > > > > + > > > > + TP_STRUCT__entry( > > > > + __field(const struct _ddebug *, desc) > > > > + __dynamic_array(char, msg, len + 1) > > > > + ), > > > > + > > > > + TP_fast_assign( > > > > + __entry->desc = desc; > > > > + /* > > > > + * Each trace entry is printed in a new line. > > > > + * If the msg finishes with '\n', cut it off > > > > + * to avoid blank lines in the trace. > > > > + */ > > > > + if (len > 0 && (text[len - 1] == '\n')) > > > > + len -= 1; > > > > + > > > > + memcpy(__get_str(msg), text, len); > > > > + __get_str(msg)[len] = 0; > > > > + ), > > > > + > > > > > > > > > > + TP_printk("%s.%s %s", __entry->desc->modname, > > > > + __entry->desc->function, __get_str(msg)) > > > > +); > > > > + > > > > > > That TP_printk() is dangerous. How do you know __entry->desc still exists > > > when reading the buffer? > > > > > > Is the struct _ddebug permanent? Can it be freed? If so, the above can > > > easily cause a crash. > > > > > > > I assume that we're talking here about the scenario where TP prdbg is > > called and before TP_printk runs _ddebug pointer > > becomes invalid, is that correct ? If so then I believe this also > > applied to __dynamic_pr_debug and other dyndbg functions because there > > is also potential for _ddebug pointer to become invalid (in case of > > rrmod) before a function dereferences it. > > > > Would it be acceptable to increase reference count of a module and > > hold it until at least one callsite in that module is enabled ? > > This would ensure that passed pointer to a _ddebug struct is valid. > > > > Im not understanding you, but I dont think its on-point - > > a loadable module might write lots to trace-log, and each trace-entry > would have the descriptor address, with which it could deref and print 3 fields. > Then rmmod happens, all the module mem is freed, and reused for someth9ing else. > > then someone cats trace, and the descriptor addrs are used to render > the tracelog. > BOOM. >
Jim, thanks for educating me on this one. I completely missed the fact that TP_printk is delayed until, for example as you mentioned cat is run on a trace.
I'll remove passing of _ddebug ptr to trace_prdbg and trace_devdbg functions. Probably also passing of dev ptr can removed from trace_devdbg.
> > > > > -- Steve
| |