Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Masami Hiramatsu <mhiramat@kernel.org>]

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>