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

[Masami Hiramatsu]

Hi Jessica,

Since this is under discussion about ftrace part, I just comment another one.

On Thu,  2 Nov 2017 17:33:33 +0100
Jessica Yu  wrote:

[...]
> @@ -1362,9 +1372,14 @@ static int register_aggr_kprobe(struct kprobe *orig_p, 
> struct kprobe *p)
>  
>   if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
>   ap->flags &= ~KPROBE_FLAG_DISABLED;
> - if (!kprobes_all_disarmed)
> + if (!kprobes_all_disarmed) {
>   /* Arm the breakpoint again. */
> - arm_kprobe(ap);
> + ret = arm_kprobe(ap);
> + if (ret) {
> + ap->flags |= KPROBE_FLAG_DISABLED;
> + list_del_rcu(>list);

You also have to wait rcu here.

> + }
> + }
>   }
>   return ret;
>  }
[...]
> @@ -2428,16 +2452,26 @@ static void arm_all_kprobes(void)
>   /* Arming kprobes doesn't optimize kprobe itself */
>   for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>   head = _table[i];
> - hlist_for_each_entry_rcu(p, head, hlist)
> - if (!kprobe_disabled(p))
> - arm_kprobe(p);
> + /* Arm all kprobes on a best-effort basis */
> + hlist_for_each_entry_rcu(p, head, hlist) {
> + if (!kprobe_disabled(p)) {
> + err = arm_kprobe(p);
> + if (err)  {
> + errors++;
> + ret = err;
> + }
> + }
> + }
>   }
>  
> - printk(KERN_INFO "Kprobes globally enabled\n");
> + if (errors)
> + pr_warn("Kprobes globally enabled, but failed to arm %d 
> kprobes\n", errors);

The last "kprobes" should be "probes" because it can include kretprobes :P

Thank you,

-- 
Masami Hiramatsu 

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

[Masami Hiramatsu]

Hi Jessica,

Since this is under discussion about ftrace part, I just comment another one.

On Thu,  2 Nov 2017 17:33:33 +0100
Jessica Yu  wrote:

[...]
> @@ -1362,9 +1372,14 @@ static int register_aggr_kprobe(struct kprobe *orig_p, 
> struct kprobe *p)
>  
>   if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
>   ap->flags &= ~KPROBE_FLAG_DISABLED;
> - if (!kprobes_all_disarmed)
> + if (!kprobes_all_disarmed) {
>   /* Arm the breakpoint again. */
> - arm_kprobe(ap);
> + ret = arm_kprobe(ap);
> + if (ret) {
> + ap->flags |= KPROBE_FLAG_DISABLED;
> + list_del_rcu(>list);

You also have to wait rcu here.

> + }
> + }
>   }
>   return ret;
>  }
[...]
> @@ -2428,16 +2452,26 @@ static void arm_all_kprobes(void)
>   /* Arming kprobes doesn't optimize kprobe itself */
>   for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>   head = _table[i];
> - hlist_for_each_entry_rcu(p, head, hlist)
> - if (!kprobe_disabled(p))
> - arm_kprobe(p);
> + /* Arm all kprobes on a best-effort basis */
> + hlist_for_each_entry_rcu(p, head, hlist) {
> + if (!kprobe_disabled(p)) {
> + err = arm_kprobe(p);
> + if (err)  {
> + errors++;
> + ret = err;
> + }
> + }
> + }
>   }
>  
> - printk(KERN_INFO "Kprobes globally enabled\n");
> + if (errors)
> + pr_warn("Kprobes globally enabled, but failed to arm %d 
> kprobes\n", errors);

The last "kprobes" should be "probes" because it can include kretprobes :P

Thank you,

-- 
Masami Hiramatsu 

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

[Masami Hiramatsu]

On Fri, 3 Nov 2017 13:33:12 -0400
Steven Rostedt  wrote:

> On Fri, 3 Nov 2017 09:53:37 -0500
> Josh Poimboeuf  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(_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(_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(_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(_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 

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

[Masami Hiramatsu]

On Fri, 3 Nov 2017 13:33:12 -0400
Steven Rostedt  wrote:

> On Fri, 3 Nov 2017 09:53:37 -0500
> Josh Poimboeuf  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(_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(_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(_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(_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 

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

[Steven Rostedt]

On Fri, 3 Nov 2017 09:53:37 -0500
Josh Poimboeuf  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(_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(_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(_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.

-- Steve

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

[Steven Rostedt]

On Fri, 3 Nov 2017 09:53:37 -0500
Josh Poimboeuf  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(_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(_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(_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.

-- Steve

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

[Josh Poimboeuf]

On Fri, Nov 03, 2017 at 10:03:17AM -0400, Steven Rostedt wrote:
> On Thu,  2 Nov 2017 17:33:33 +0100
> Jessica Yu  wrote:
> 
> > Improve error handling when arming ftrace-based kprobes. Specifically, if
> > we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
> > should report an error instead of success. Previously, this has lead to
> > confusing situations where register_kprobe() would return 0 indicating
> > success, but the kprobe would not be functional if ftrace registration
> > during the kprobe arming process had failed. We should therefore take any
> > errors returned by ftrace into account and propagate this error so that we
> > do not register/enable kprobes that cannot be armed. This can happen if,
> > for example, register_ftrace_function() finds an IPMODIFY conflict (since
> > kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
> > is possible since livepatches also set the IPMODIFY flag for their 
> > ftrace_ops.
> > 
> > arm_all_kprobes() keeps its current behavior and attempts to arm all
> > kprobes. It returns the last encountered error and gives a warning if
> > not all kprobes could be armed.
> > 
> > This patch is based on Petr Mladek's original patchset (patches 2 and 3)
> > back in 2015, which improved kprobes error handling, found here:
> > 
> >https://lkml.org/lkml/2015/2/26/452
> > 
> > However, further work on this had been paused since then and the patches
> > were not upstreamed.
> > 
> > Based-on-patches-by: Petr Mladek 
> > Signed-off-by: Jessica Yu 
> > ---
> >  kernel/kprobes.c | 88 
> > 
> >  1 file changed, 63 insertions(+), 25 deletions(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index da2ccf142358..f4a094007cb5 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p)
> >  }
> >  
> >  /* Caller must lock kprobe_mutex */
> > -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(_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(_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(_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?

-- 
Josh

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

[Josh Poimboeuf]

On Fri, Nov 03, 2017 at 10:03:17AM -0400, Steven Rostedt wrote:
> On Thu,  2 Nov 2017 17:33:33 +0100
> Jessica Yu  wrote:
> 
> > Improve error handling when arming ftrace-based kprobes. Specifically, if
> > we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
> > should report an error instead of success. Previously, this has lead to
> > confusing situations where register_kprobe() would return 0 indicating
> > success, but the kprobe would not be functional if ftrace registration
> > during the kprobe arming process had failed. We should therefore take any
> > errors returned by ftrace into account and propagate this error so that we
> > do not register/enable kprobes that cannot be armed. This can happen if,
> > for example, register_ftrace_function() finds an IPMODIFY conflict (since
> > kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
> > is possible since livepatches also set the IPMODIFY flag for their 
> > ftrace_ops.
> > 
> > arm_all_kprobes() keeps its current behavior and attempts to arm all
> > kprobes. It returns the last encountered error and gives a warning if
> > not all kprobes could be armed.
> > 
> > This patch is based on Petr Mladek's original patchset (patches 2 and 3)
> > back in 2015, which improved kprobes error handling, found here:
> > 
> >https://lkml.org/lkml/2015/2/26/452
> > 
> > However, further work on this had been paused since then and the patches
> > were not upstreamed.
> > 
> > Based-on-patches-by: Petr Mladek 
> > Signed-off-by: Jessica Yu 
> > ---
> >  kernel/kprobes.c | 88 
> > 
> >  1 file changed, 63 insertions(+), 25 deletions(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index da2ccf142358..f4a094007cb5 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p)
> >  }
> >  
> >  /* Caller must lock kprobe_mutex */
> > -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(_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(_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(_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?

-- 
Josh

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

[Steven Rostedt]

On Thu,  2 Nov 2017 17:33:33 +0100
Jessica Yu  wrote:

> Improve error handling when arming ftrace-based kprobes. Specifically, if
> we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
> should report an error instead of success. Previously, this has lead to
> confusing situations where register_kprobe() would return 0 indicating
> success, but the kprobe would not be functional if ftrace registration
> during the kprobe arming process had failed. We should therefore take any
> errors returned by ftrace into account and propagate this error so that we
> do not register/enable kprobes that cannot be armed. This can happen if,
> for example, register_ftrace_function() finds an IPMODIFY conflict (since
> kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
> is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.
> 
> arm_all_kprobes() keeps its current behavior and attempts to arm all
> kprobes. It returns the last encountered error and gives a warning if
> not all kprobes could be armed.
> 
> This patch is based on Petr Mladek's original patchset (patches 2 and 3)
> back in 2015, which improved kprobes error handling, found here:
> 
>https://lkml.org/lkml/2015/2/26/452
> 
> However, further work on this had been paused since then and the patches
> were not upstreamed.
> 
> Based-on-patches-by: Petr Mladek 
> Signed-off-by: Jessica Yu 
> ---
>  kernel/kprobes.c | 88 
> 
>  1 file changed, 63 insertions(+), 25 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index da2ccf142358..f4a094007cb5 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p)
>  }
>  
>  /* Caller must lock kprobe_mutex */
> -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(_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(_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(_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.

Perhaps you want to add it to the "notrace" list. Which would require
implementing a ftrace_set_notrace_ip() function. Which I believe is
what you want. Any function in the notrace hash will have the same
functions in the filter hash be ignored.

I'll let Masami review the rest.

-- Steve


> + return ret;
>  }
>  

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

[Steven Rostedt]

On Thu,  2 Nov 2017 17:33:33 +0100
Jessica Yu  wrote:

> Improve error handling when arming ftrace-based kprobes. Specifically, if
> we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
> should report an error instead of success. Previously, this has lead to
> confusing situations where register_kprobe() would return 0 indicating
> success, but the kprobe would not be functional if ftrace registration
> during the kprobe arming process had failed. We should therefore take any
> errors returned by ftrace into account and propagate this error so that we
> do not register/enable kprobes that cannot be armed. This can happen if,
> for example, register_ftrace_function() finds an IPMODIFY conflict (since
> kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
> is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.
> 
> arm_all_kprobes() keeps its current behavior and attempts to arm all
> kprobes. It returns the last encountered error and gives a warning if
> not all kprobes could be armed.
> 
> This patch is based on Petr Mladek's original patchset (patches 2 and 3)
> back in 2015, which improved kprobes error handling, found here:
> 
>https://lkml.org/lkml/2015/2/26/452
> 
> However, further work on this had been paused since then and the patches
> were not upstreamed.
> 
> Based-on-patches-by: Petr Mladek 
> Signed-off-by: Jessica Yu 
> ---
>  kernel/kprobes.c | 88 
> 
>  1 file changed, 63 insertions(+), 25 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index da2ccf142358..f4a094007cb5 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p)
>  }
>  
>  /* Caller must lock kprobe_mutex */
> -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(_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(_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(_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.

Perhaps you want to add it to the "notrace" list. Which would require
implementing a ftrace_set_notrace_ip() function. Which I believe is
what you want. Any function in the notrace hash will have the same
functions in the filter hash be ignored.

I'll let Masami review the rest.

-- Steve


> + return ret;
>  }
>