[PATCH] kprobes: Allow kprobes coexist with livepatch

[PATCH] kprobes: Allow kprobes coexist with livepatch

[Masami Hiramatsu]

Allow kprobes which do not modify regs->ip, coexist with livepatch
by dropping FTRACE_OPS_FL_IPMODIFY from ftrace_ops.

User who wants to modify regs->ip (e.g. function fault injection)
must set a dummy post_handler to its kprobes when registering.
However, if such regs->ip modifying kprobes is set on a function,
that function can not be livepatched.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |   56 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9873fc627d61..29065380dad0 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -961,9 +961,16 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 
 #ifdef CONFIG_KPROBES_ON_FTRACE
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
+	.func = kprobe_ftrace_handler,
+	.flags = FTRACE_OPS_FL_SAVE_REGS,
+};
+
+static struct ftrace_ops kprobe_ipmodify_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
 	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
 };
+
+static int kprobe_ipmodify_enabled;
 static int kprobe_ftrace_enabled;
 
 /* Must ensure p->addr is really on ftrace */
@@ -976,58 +983,75 @@ static int prepare_kprobe(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static int arm_kprobe_ftrace(struct kprobe *p)
+static int __arm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops,
+			       int *cnt)
 {
 	int ret = 0;
 
-	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
-				   (unsigned long)p->addr, 0, 0);
+	ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 0, 0);
 	if (ret) {
 		pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
 			 p->addr, ret);
 		return ret;
 	}
 
-	if (kprobe_ftrace_enabled == 0) {
-		ret = register_ftrace_function(&kprobe_ftrace_ops);
+	if (*cnt == 0) {
+		ret = register_ftrace_function(ops);
 		if (ret) {
 			pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
 			goto err_ftrace;
 		}
 	}
 
-	kprobe_ftrace_enabled++;
+	(*cnt)++;
 	return ret;
 
 err_ftrace:
 	/*
-	 * Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
-	 * non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
-	 * empty filter_hash which would undesirably trace all functions.
+	 * At this point, sinec ops is not registered, we should be sefe from
+	 * registering empty filter.
 	 */
-	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+	ftrace_set_filter_ip(ops, (unsigned long)p->addr, 1, 0);
 	return ret;
 }
 
+static int arm_kprobe_ftrace(struct kprobe *p)
+{
+	bool ipmodify = (p->post_handler != NULL);
+
+	return __arm_kprobe_ftrace(p,
+		ipmodify ? &kprobe_ipmodify_ops : &kprobe_ftrace_ops,
+		ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled);
+}
+
 /* Caller must lock kprobe_mutex */
-static int disarm_kprobe_ftrace(struct kprobe *p)
+static int __disarm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops,
+				  int *cnt)
 {
 	int ret = 0;
 
-	if (kprobe_ftrace_enabled == 1) {
-		ret = unregister_ftrace_function(&kprobe_ftrace_ops);
+	if (*cnt == 1) {
+		ret = unregister_ftrace_function(ops);
 		if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
 			return ret;
 	}
 
-	kprobe_ftrace_enabled--;
+	(*cnt)--;
 
-	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
-			   (unsigned long)p->addr, 1, 0);
+	ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 1, 0);
 	WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
 		  p->addr, ret);
 	return ret;
 }
+
+static int disarm_kprobe_ftrace(struct kprobe *p)
+{
+	bool ipmodify = (p->post_handler != NULL);
+
+	return __disarm_kprobe_ftrace(p,
+		ipmodify ? &kprobe_ipmodify_ops : &kprobe_ftrace_ops,
+		ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled);
+}
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)	(-ENODEV)

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Steven Rostedt]

On Thu, 25 Jul 2019 15:24:37 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Allow kprobes which do not modify regs->ip, coexist with livepatch
> by dropping FTRACE_OPS_FL_IPMODIFY from ftrace_ops.
> 
> User who wants to modify regs->ip (e.g. function fault injection)
> must set a dummy post_handler to its kprobes when registering.
> However, if such regs->ip modifying kprobes is set on a function,
> that function can not be livepatched.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>


Looks good! I applied it.

Thanks Masami!

-- Steve

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Joe Lawrence]

On Thu, Jul 25, 2019 at 03:24:37PM +0900, Masami Hiramatsu wrote:
> Allow kprobes which do not modify regs->ip, coexist with livepatch
> by dropping FTRACE_OPS_FL_IPMODIFY from ftrace_ops.
> 
> User who wants to modify regs->ip (e.g. function fault injection)
> must set a dummy post_handler to its kprobes when registering.
> However, if such regs->ip modifying kprobes is set on a function,
> that function can not be livepatched.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c |   56 +++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 9873fc627d61..29065380dad0 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -961,9 +961,16 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
>  
>  #ifdef CONFIG_KPROBES_ON_FTRACE
>  static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
> +	.func = kprobe_ftrace_handler,
> +	.flags = FTRACE_OPS_FL_SAVE_REGS,
> +};
> +
> +static struct ftrace_ops kprobe_ipmodify_ops __read_mostly = {
>  	.func = kprobe_ftrace_handler,
>  	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
>  };
> +
> +static int kprobe_ipmodify_enabled;
>  static int kprobe_ftrace_enabled;
>  
>  /* Must ensure p->addr is really on ftrace */
> @@ -976,58 +983,75 @@ static int prepare_kprobe(struct kprobe *p)
>  }
>  
>  /* Caller must lock kprobe_mutex */
> -static int arm_kprobe_ftrace(struct kprobe *p)
> +static int __arm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops,
> +			       int *cnt)
>  {
>  	int ret = 0;
>  
> -	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> -				   (unsigned long)p->addr, 0, 0);
> +	ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 0, 0);
>  	if (ret) {
>  		pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
>  			 p->addr, ret);
>  		return ret;
>  	}
>  
> -	if (kprobe_ftrace_enabled == 0) {
> -		ret = register_ftrace_function(&kprobe_ftrace_ops);
> +	if (*cnt == 0) {
> +		ret = register_ftrace_function(ops);
>  		if (ret) {
>  			pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
>  			goto err_ftrace;
>  		}
>  	}
>  
> -	kprobe_ftrace_enabled++;
> +	(*cnt)++;
>  	return ret;
>  
>  err_ftrace:
>  	/*
> -	 * Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
> -	 * non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
> -	 * empty filter_hash which would undesirably trace all functions.
> +	 * At this point, sinec ops is not registered, we should be sefe from
> +	 * registering empty filter.
>  	 */
> -	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
> +	ftrace_set_filter_ip(ops, (unsigned long)p->addr, 1, 0);
>  	return ret;
>  }
>  
> +static int arm_kprobe_ftrace(struct kprobe *p)
> +{
> +	bool ipmodify = (p->post_handler != NULL);
> +
> +	return __arm_kprobe_ftrace(p,
> +		ipmodify ? &kprobe_ipmodify_ops : &kprobe_ftrace_ops,
> +		ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled);
> +}
> +
>  /* Caller must lock kprobe_mutex */
> -static int disarm_kprobe_ftrace(struct kprobe *p)
> +static int __disarm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops,
> +				  int *cnt)
>  {
>  	int ret = 0;
>  
> -	if (kprobe_ftrace_enabled == 1) {
> -		ret = unregister_ftrace_function(&kprobe_ftrace_ops);
> +	if (*cnt == 1) {
> +		ret = unregister_ftrace_function(ops);
>  		if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
>  			return ret;
>  	}
>  
> -	kprobe_ftrace_enabled--;
> +	(*cnt)--;
>  
> -	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> -			   (unsigned long)p->addr, 1, 0);
> +	ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 1, 0);
>  	WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
>  		  p->addr, ret);
>  	return ret;
>  }
> +
> +static int disarm_kprobe_ftrace(struct kprobe *p)
> +{
> +	bool ipmodify = (p->post_handler != NULL);
> +
> +	return __disarm_kprobe_ftrace(p,
> +		ipmodify ? &kprobe_ipmodify_ops : &kprobe_ftrace_ops,
> +		ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled);
> +}
>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
>  #define arm_kprobe_ftrace(p)	(-ENODEV)
> 

Thanks for the quick patch, Masami!

I gave it a spin and here are my new testing results:


perf probe, then livepatch
--------------------------

% perf probe --add cmdline_proc_show
Added new event:
  probe:cmdline_proc_show (on cmdline_proc_show)

You can now use it in all perf tools, such as:

        perf record -e probe:cmdline_proc_show -aR sleep 1

% perf record -e probe:cmdline_proc_show -aR sleep 30 &
[1] 980

% insmod samples/livepatch/livepatch-sample.ko

% cat /proc/cmdline 
this has been live patched

% fg
perf record -e probe:cmdline_proc_show -aR sleep 30
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.177 MB perf.data (2 samples) ]

% perf script
          insmod   983 [000]   157.126556: probe:cmdline_proc_show: (ffffffff9bf74890)
             cat   985 [000]   162.304028: probe:cmdline_proc_show: (ffffffff9bf74890)


livepatch, then perf probe
--------------------------

% insmod samples/livepatch/livepatch-sample.ko

% cat /proc/cmdline 
this has been live patched

% perf record -e probe:cmdline_proc_show -aR sleep 30
event syntax error: 'probe:cmdline_proc_show'
                     \___ unknown tracepoint

Error:  File /sys/kernel/debug/tracing/events/probe/cmdline_proc_show not found.
Hint:   Perhaps this kernel misses some CONFIG_ setting to enable this feature?.

Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list available events


These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in
light of your changes, so feel free to add my:

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Steven Rostedt]

On Thu, 25 Jul 2019 22:07:52 -0400
Joe Lawrence <joe.lawrence@redhat.com> wrote:

> These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in
> light of your changes, so feel free to add my:
> 
> Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

Is this an urgent patch (needs to go in now and not wait for the next
merge window) and if so, should it be marked for stable?

-- Steve

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Joe Lawrence]

On 7/26/19 12:14 PM, Steven Rostedt wrote:
> On Thu, 25 Jul 2019 22:07:52 -0400
> Joe Lawrence <joe.lawrence@redhat.com> wrote:
> 
>> These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in
>> light of your changes, so feel free to add my:
>>
>> Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Is this an urgent patch (needs to go in now and not wait for the next
> merge window) and if so, should it be marked for stable?
> 

Hi Steve,

IMHO, it's not urgent..  so unless Josh or other livepatch folks say 
otherwise, I'm ok with waiting for next merge window.  Given summer 
holiday schedules, that would give them more time to comment if they like.

Thanks,

-- Joe

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Steven Rostedt]

On Fri, 26 Jul 2019 13:38:41 -0400
Joe Lawrence <joe.lawrence@redhat.com> wrote:

> On 7/26/19 12:14 PM, Steven Rostedt wrote:
> > On Thu, 25 Jul 2019 22:07:52 -0400
> > Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >   
> >> These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in
> >> light of your changes, so feel free to add my:
> >>
> >> Acked-by: Joe Lawrence <joe.lawrence@redhat.com>  
> > 
> > Is this an urgent patch (needs to go in now and not wait for the next
> > merge window) and if so, should it be marked for stable?
> >   
> 
> Hi Steve,
> 
> IMHO, it's not urgent..  so unless Josh or other livepatch folks say 
> otherwise, I'm ok with waiting for next merge window.  Given summer 
> holiday schedules, that would give them more time to comment if they like.

I added it to my next merge window queue. If someone thinks it's more
urgent, I can move it to my urgent queue. I wont post to linux-next for
a few weeks, so there's time to extract it if need be.

Thanks!

-- Steve

Re: [PATCH] kprobes: Allow kprobes coexist with livepatch

[Masami Hiramatsu]

On Fri, 26 Jul 2019 13:38:41 -0400
Joe Lawrence <joe.lawrence@redhat.com> wrote:

> On 7/26/19 12:14 PM, Steven Rostedt wrote:
> > On Thu, 25 Jul 2019 22:07:52 -0400
> > Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > 
> >> These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in
> >> light of your changes, so feel free to add my:
> >>
> >> Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
> > 
> > Is this an urgent patch (needs to go in now and not wait for the next
> > merge window) and if so, should it be marked for stable?
> > 
> 
> Hi Steve,
> 
> IMHO, it's not urgent..  so unless Josh or other livepatch folks say 
> otherwise, I'm ok with waiting for next merge window.  Given summer 
> holiday schedules, that would give them more time to comment if they like.

Agreed. Since system admin can control kprobes and livepatch, which
means the confliction can be avoided.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>