On Fri, 3 Nov 2017 13:33:12 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> On Fri, 3 Nov 2017 09:53:37 -0500
> Josh Poimboeuf <jpoim...@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 <mhira...@kernel.org>
