Re: [PATCH v2 1/2] kprobes: propagate error from arm_kprobe_ftrace()

[Masami Hiramatsu <mhiramat@kernel.org>]

On Fri, 3 Nov 2017 13:33:12 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 3 Nov 2017 09:53:37 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > > > -static void arm_kprobe_ftrace(struct kprobe *p)
> > > > +static int arm_kprobe_ftrace(struct kprobe *p)
> > > >  {
> > > > -	int ret;
> > > > +	int ret = 0;
> > > >  
> > > >  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> > > >  				   (unsigned long)p->addr, 0, 0);
> > > > -	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> > > > -	kprobe_ftrace_enabled++;
> > > > -	if (kprobe_ftrace_enabled == 1) {
> > > > +	if (WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret))
> > > > +		return ret;
> > > > +
> > > > +	if (kprobe_ftrace_enabled == 0) {
> > > >  		ret = register_ftrace_function(&kprobe_ftrace_ops);
> > > > -		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
> > > > +		if (WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret))
> > > > +			goto err_ftrace;
> > > >  	}
> > > > +
> > > > +	kprobe_ftrace_enabled++;
> > > > +	return ret;
> > > > +
> > > > +err_ftrace:
> > > > +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);  
> > > 
> > > Hmm, this could have a very nasty side effect. If you remove a function
> > > from the ops, and it was the last function, an empty ops means to trace
> > > *all* functions.  
> > 
> > But this error path only runs when register_ftrace_function() fails, in
> > which case the ops aren't live anyway, right?
> 
> I was thinking that if there was more than one function that is going
> to be registered, that only this one would be black listed. But yeah,
> if there was only one function in the hash, then it probably wouldn't
> matter if it was cleared, because it failed. But I'm paranoid about
> things like this, and prefer to be more robust than to depend on the
> design to enforce correctness than to have each individual function
> being contained and do what is expected of it.

So, what coding will be better? I can only think of this;

ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
			(unsigned long)p->addr, 0, !kprobe_ftrace_enabled);

And do not remove ip from filter in error case of register_ftrace_function.


BTW, ftrace_set_filter_ip(..., 1, 0); is not easy to read (and @reset
is meaningless in removing)

ftrace_new_filter_ip(ops, addr);
ftrace_append_filter_ip(ops, addr);
ftrace_remove_filter_ip(ops, addr);

wrappers will be more useful.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>