lkml.org 
[lkml]   [2021]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] tracing: Do no reference char * as a string in histograms
From
Date
Hi Steve,

On Thu, 2021-07-15 at 15:54 -0400, Steven Rostedt wrote:
> On Thu, 15 Jul 2021 12:04:13 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
>
> > Not sure what the correct fix is at this point, but it would seem
> > that
> > if the original intent was honored, the histogram code should still
> > be
> > allowed to use them.
>
> Note, it can in a patch I plan on sending in the next merge window
> that
> Tzvetomir and I are working on. We succeeded in getting the "eprobe"
> working which allows you to do much more with event fields than the
> current histogram code does.
>
> Otherwise, I need a way to get the pointer and not a string. As I
> discovered, the string may not have any content in it, and you can't
> fault within a tracepoint, and you are left with just passing in
> "(fault)", or something. Which is not useful at all!
>
> What we have as a working prototype is this:
>
> # cd /sys/kernel/tracing
> # echo 'e:myopen syscalls.sys_enter_openat file=$filename:ustring' >
> kprobe_events
> # trace-cmd start -e myopen -F ~/bin/openat
> # cat trace
> openat-1928 [000] ...2 1540.638692: myopen: (0)
> file="/etc/ld.so.cache"
> openat-1928 [000] ...2 1540.638692: myopen: (0)
> file="/lib64/libc.so.6"
> openat-1928 [000] ...2 1540.638694: myopen: (0)
> file=(fault)
>
>
> And if we were to enforce this on a histogram, all we will see is
> "fault" because the filename is first referenced inside the system
> call
> and at the beginning of the call, it is not mapped in, and we can not
> map it in inside a tracepoint. You are left with just putting in some
> stub string (which kprobes uses "(fault)").
>
> But, because now filename is a pointer, that the histograms can pass
> around, it is much more useful, as I can use this to pass the
> filename
> pointer to the exit of the system call!
>
> # echo 'hist:keys=common_pid:f=filename' >
> events/syscalls/sys_enter_openat/trigger
> # echo 'openret unsigned long file; long ret' > synthetic_events
> # echo
> 'hist:keys=common_pid:file=$f:onmatch(syscalls.sys_enter_openat).trac
> e(openret,$file,ret)' > events/syscalls/sys_exit_openat/trigger
> # echo 'e:myopen synthetic.openret file=$file:ustring ret=$ret:s64'
> > kprobe_events
> # trace-cmd start -e myopen -F ~/bin/openat
> # cat trace
> openat-1994 [002] ...3 2052.403323: myopen: (0)
> file="/etc/ld.so.cache" ret=3
> openat-1994 [002] ...3 2052.403350: myopen: (0)
> file="/lib64/libc.so.6" ret=3
> openat-1994 [002] ...3 2052.403555: myopen: (0)
> file="/etc/passwd" ret=3
>
> Now, not only can we see the filename, because we passed the pointer
> from the sys_enter to the sys_exit, we also can show the return of
> the
> system call.
>

Very nice! Really looking forward to playing around with this.

> This allows for a much more flexibility than the histograms by
> themselves have. If you want the file names via pointers, you can
> then
> create these event_probes and use them in the histograms as well.
> Which
> gives you the full flexibility of kprobes but on the fields of
> events.
>
> Note, without this patch, because the histograms thinks 'char *' is a
> string, you can't pass the pointer, but only the string. Hence, you
> only get "(fault)", which is pretty darn useless.
>
> This is another rationale for why I want this patch.
>

Yeah, if it's needed to enable the eprobes and since it also fixes a
crashing bug, I'm all for it.

But should it also have an additional 'Fixes:' line?:

Fixes: 5967bd5c4239 ("tracing: Let filter_assign_type() detect
FILTER_PTR_STRING")

Acked-by: Tom Zanussi <zanussi@kernel.org>


\
 
 \ /
  Last update: 2021-07-15 22:41    [W:0.250 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site