Messages in this thread | | | Date | Sun, 4 Nov 2018 01:21:52 +0900 | From | Masami Hiramatsu <> | Subject | Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order |
| |
On Sat, 3 Nov 2018 09:17:54 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> > > Then I tested with this: > > > > > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > > > index 3ef15a6683c0..4ddafddf1343 100644 > > > --- a/kernel/trace/trace_probe.c > > > +++ b/kernel/trace/trace_probe.c > > > @@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg > > > *arg) kfree(code->data); > > > code++; > > > } > > > + printk("free arg->code %s\n", arg->code); > > > kfree(arg->code); > > > kfree(arg->name); > > > kfree(arg->comm); > > > @@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg) > > > if (code[1].op != FETCH_OP_IMM) > > > return -EINVAL; > > > > > > + tmp = strpbrk(code->data, "+-"); > > > + printk("first tmp tmp=%s\n", tmp); > > > tmp = strpbrk("+-", code->data); > > > + printk("second tmp=%s data=%s\n", tmp, > > > code->data); if (tmp) > > > c = *tmp; > > > ret = > > > traceprobe_split_symbol_offset(code->data, &offset); > > > + printk("third data=%s\n", code->data); > > > if (ret) > > > return ret; > > > > > > @@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg) > > > (unsigned > > > long)kallsyms_lookup_name(code->data); if (tmp) > > > *tmp = c; > > > + printk("forth data=%s\n", code->data); > > > if (!code[1].immediate) > > > return -ENOENT; > > > code[1].immediate += offset; > > > > > > And I don't see where that code->data is used elsewhere. That is, why > > > even bother saving the character? > > > > Would you mean parsing the symbol+offs every time is useless? > > It needs to solve the symbol address always because traceprobe_update_arg > > is called when new symbols added on the kernel (by loading modules). > > OK, so it is called multiple times? That is when a module is loaded? > Because I couldn't get this called multiple times.
Sorry I mislead you. See trace_kprobe_module_callback(), which calls __register_trace_kprobe() if the probepoint is on the module. And __register_trace_kprobe() calls traceprobe_update_arg(). So, if you call it multiple time, it should be with the probe point is on the module too.
e.g.
echo "p newmod:function @var_symbol+offset" > kprobe_events
can be called multiple times if we load/unload "newmod" module.
> I'll try that out and if that's the case, then yeah, this needs to be > fixed (otherwise, I was thinking we could just remove the strpbrk() > altogether). > > > > > > Hmm, maybe I can introduce a struct like > > > > struct symbol_offset { > > long offset; > > char symbol[]; > > }; > > > > and use it instead of parsing the symbol+offset always. > > The parsing should be fine. The issue I had was that I couldn't trigger > it to happen more than once. That's why this passed testing. We didn't > do something that required it to be called more than once and that is > here the bug would trigger.
Hmm, I hit it by kprobe_args_syntax.tc, so hit once is enough to kick this bug. Maybe some config option makes "+-" readonly, which previously didn't enabled.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |