[PATCH v2 0/2] kprobes: improve error handling when arming/disarming kprobes

[PATCH v2 0/2] kprobes: improve error handling when arming/disarming kprobes

[Jessica Yu]

Hi,

This patchset attempts to improve error handling when arming or disarming
ftrace-based kprobes. The current behavior is to simply WARN when ftrace
(un-)registration fails, without propagating the error code. This can lead
to confusing situations where, for example, register_kprobe()/enable_kprobe()
would return 0 indicating success even if arming via ftrace had failed. In
this scenario we'd end up with a non-functioning kprobe even though kprobe
registration (or enablement) returned success. In this patchset, we take
errors from ftrace into account and propagate the error when we cannot arm
or disarm a kprobe.

Below is an example that illustrates the problem using livepatch and
systemtap (which uses kprobes underneath). Both livepatch and kprobes use
ftrace ops with the IPMODIFY flag set, so registration at the same
function entry is limited to only one ftrace user. 

Before
------
# modprobe livepatch-sample 	# patches cmdline_proc_show, ftrace ops has IPMODIFY set
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'

   .. (nothing prints after reading /proc/cmdline) ..

The systemtap handler doesn't execute due to a kprobe arming failure caused
by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
indication of error from systemtap (because register_kprobe() returned
success) unless the user inspects dmesg.

After
-----
# modprobe livepatch-sample 
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)

Although the systemtap handler doesn't execute (as it shouldn't), the
ftrace error is propagated and now systemtap prints a visible error message
stating that (kprobe) registration had failed (because register_kprobe()
returned an error), along with the propagated error code.

This patchset was 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.

This patchset has been lightly sanity-tested (on linux-next) with kprobes,
kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
more testing is greatly appreciated.


Changes from v1:
- Don't arm the kprobe before adding it to the kprobe table, otherwise
  we'll temporarily see a stray breakpoint.
- Remove kprobe from the kprobe_table and call synchronize_sched() if
  arming during register_kprobe() fails.
- add Masami's ack on the 2nd patch (unchanged from v1)

---
Jessica Yu (2):
  kprobes: propagate error from arm_kprobe_ftrace()
  kprobes: propagate error from disarm_kprobe_ftrace()

 kernel/kprobes.c | 164 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 114 insertions(+), 50 deletions(-)

-- 
2.13.6

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

[Jessica Yu]

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 <pmladek@suse.com>
Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 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(&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);
+	return ret;
 }
 
 /* Caller must lock kprobe_mutex */
@@ -1008,22 +1017,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p)	do {} while (0)
+#define arm_kprobe_ftrace(p)	(0)
 #define disarm_kprobe_ftrace(p)	do {} while (0)
 #endif
 
 /* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
 {
-	if (unlikely(kprobe_ftrace(kp))) {
-		arm_kprobe_ftrace(kp);
-		return;
-	}
+	if (unlikely(kprobe_ftrace(kp)))
+		return arm_kprobe_ftrace(kp);
+
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
 	__arm_kprobe(kp);
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
+
+	return 0;
 }
 
 /* Disarm a kprobe with text_mutex */
@@ -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(&p->list);
+			}
+		}
 	}
 	return ret;
 }
@@ -1573,8 +1588,14 @@ int register_kprobe(struct kprobe *p)
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-	if (!kprobes_all_disarmed && !kprobe_disabled(p))
-		arm_kprobe(p);
+	if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
+		ret = arm_kprobe(p);
+		if (ret) {
+			hlist_del_rcu(&p->hlist);
+			synchronize_sched();
+			goto out;
+		}
+	}
 
 	/* Try to optimize kprobe */
 	try_to_optimize_kprobe(p);
@@ -2116,7 +2137,9 @@ int enable_kprobe(struct kprobe *kp)
 
 	if (!kprobes_all_disarmed && kprobe_disabled(p)) {
 		p->flags &= ~KPROBE_FLAG_DISABLED;
-		arm_kprobe(p);
+		ret = arm_kprobe(p);
+		if (ret)
+			p->flags |= KPROBE_FLAG_DISABLED;
 	}
 out:
 	mutex_unlock(&kprobe_mutex);
@@ -2407,11 +2430,12 @@ static const struct file_operations debugfs_kprobe_blacklist_ops = {
 	.release        = seq_release,
 };
 
-static void arm_all_kprobes(void)
+static int arm_all_kprobes(void)
 {
 	struct hlist_head *head;
 	struct kprobe *p;
-	unsigned int i;
+	unsigned int i, errors = 0;
+	int err, ret = 0;
 
 	mutex_lock(&kprobe_mutex);
 
@@ -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 = &kprobe_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);
+	else
+		pr_info("Kprobes globally enabled\n");
 
 already_enabled:
 	mutex_unlock(&kprobe_mutex);
-	return;
+	return ret;
 }
 
 static void disarm_all_kprobes(void)
@@ -2494,6 +2528,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 {
 	char buf[32];
 	size_t buf_size;
+	int ret = 0;
 
 	buf_size = min(count, (sizeof(buf)-1));
 	if (copy_from_user(buf, user_buf, buf_size))
@@ -2504,7 +2539,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 	case 'y':
 	case 'Y':
 	case '1':
-		arm_all_kprobes();
+		ret = arm_all_kprobes();
 		break;
 	case 'n':
 	case 'N':
@@ -2515,6 +2550,9 @@ static ssize_t write_enabled_file_bool(struct file *file,
 		return -EINVAL;
 	}
 
+	if (ret)
+		return ret;
+
 	return count;
 }
 
-- 
2.13.6

[PATCH v2 2/2] kprobes: propagate error from disarm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.

disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all kprobes could be disarmed.

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 <pmladek@suse.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 kernel/kprobes.c | 76 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f4a094007cb5..2d50d3af78d7 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1002,23 +1002,27 @@ static int arm_kprobe_ftrace(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
 {
-	int ret;
+	int ret = 0;
 
-	kprobe_ftrace_enabled--;
-	if (kprobe_ftrace_enabled == 0) {
+	if (kprobe_ftrace_enabled == 1) {
 		ret = unregister_ftrace_function(&kprobe_ftrace_ops);
-		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+		if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
+			return ret;
 	}
+
+	kprobe_ftrace_enabled--;
+
 	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
 			   (unsigned long)p->addr, 1, 0);
 	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+	return ret;
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)	(0)
-#define disarm_kprobe_ftrace(p)	do {} while (0)
+#define disarm_kprobe_ftrace(p)	(0)
 #endif
 
 /* Arm a kprobe with text_mutex */
@@ -1037,18 +1041,18 @@ static int arm_kprobe(struct kprobe *kp)
 }
 
 /* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
 {
-	if (unlikely(kprobe_ftrace(kp))) {
-		disarm_kprobe_ftrace(kp);
-		return;
-	}
+	if (unlikely(kprobe_ftrace(kp)))
+		return disarm_kprobe_ftrace(kp);
 
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
 	__disarm_kprobe(kp, reopt);
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
+
+	return 0;
 }
 
 /*
@@ -1629,11 +1633,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 static struct kprobe *__disable_kprobe(struct kprobe *p)
 {
 	struct kprobe *orig_p;
+	int ret;
 
 	/* Get an original kprobe for return */
 	orig_p = __get_valid_kprobe(p);
 	if (unlikely(orig_p == NULL))
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	if (!kprobe_disabled(p)) {
 		/* Disable probe if it is a child probe */
@@ -1647,8 +1652,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
 			 * should have already been disarmed, so
 			 * skip unneed disarming process.
 			 */
-			if (!kprobes_all_disarmed)
-				disarm_kprobe(orig_p, true);
+			if (!kprobes_all_disarmed) {
+				ret = disarm_kprobe(orig_p, true);
+				if (ret) {
+					p->flags &= ~KPROBE_FLAG_DISABLED;
+					return ERR_PTR(ret);
+				}
+			}
 			orig_p->flags |= KPROBE_FLAG_DISABLED;
 		}
 	}
@@ -1665,8 +1675,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
 
 	/* Disable kprobe. This will disarm it if needed. */
 	ap = __disable_kprobe(p);
-	if (ap == NULL)
-		return -EINVAL;
+	if (IS_ERR(ap))
+		return PTR_ERR(ap);
 
 	if (ap == p)
 		/*
@@ -2099,12 +2109,14 @@ static void kill_kprobe(struct kprobe *p)
 int disable_kprobe(struct kprobe *kp)
 {
 	int ret = 0;
+	struct kprobe *p;
 
 	mutex_lock(&kprobe_mutex);
 
 	/* Disable this kprobe */
-	if (__disable_kprobe(kp) == NULL)
-		ret = -EINVAL;
+	p = __disable_kprobe(kp);
+	if (IS_ERR(p))
+		ret = PTR_ERR(p);
 
 	mutex_unlock(&kprobe_mutex);
 	return ret;
@@ -2474,34 +2486,48 @@ static int arm_all_kprobes(void)
 	return ret;
 }
 
-static void disarm_all_kprobes(void)
+static int disarm_all_kprobes(void)
 {
 	struct hlist_head *head;
 	struct kprobe *p;
-	unsigned int i;
+	unsigned int i, errors = 0;
+	int err, ret = 0;
 
 	mutex_lock(&kprobe_mutex);
 
 	/* If kprobes are already disarmed, just return */
 	if (kprobes_all_disarmed) {
 		mutex_unlock(&kprobe_mutex);
-		return;
+		return 0;
 	}
 
 	kprobes_all_disarmed = true;
-	printk(KERN_INFO "Kprobes globally disabled\n");
 
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
+		/* Disarm all kprobes on a best-effort basis */
 		hlist_for_each_entry_rcu(p, head, hlist) {
-			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
-				disarm_kprobe(p, false);
+			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
+				err = disarm_kprobe(p, false);
+				if (err) {
+					errors++;
+					ret = err;
+				}
+			}
 		}
 	}
+
+	if (errors)
+		pr_warn("Kprobes globally disabled, but failed to disarm %d kprobes\n", errors);
+	else
+		pr_info("Kprobes globally disabled\n");
+
 	mutex_unlock(&kprobe_mutex);
 
 	/* Wait for disarming all kprobes by optimizer */
 	wait_for_kprobe_optimizer();
+
+	return ret;
 }
 
 /*
@@ -2544,7 +2570,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 	case 'n':
 	case 'N':
 	case '0':
-		disarm_all_kprobes();
+		ret = disarm_all_kprobes();
 		break;
 	default:
 		return -EINVAL;
-- 
2.13.6

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 <jeyu@kernel.org> 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 <pmladek@suse.com>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---
>  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(&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.

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()

[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 <jeyu@kernel.org> 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 <pmladek@suse.com>
> > Signed-off-by: Jessica Yu <jeyu@kernel.org>
> > ---
> >  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(&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?

-- 
Josh

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 <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.

-- Steve

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 <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>

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 <jeyu@kernel.org> 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(&p->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 = &kprobe_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 <mhiramat@kernel.org>

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

+++ Steven Rostedt [03/11/17 10:03 -0400]:
>On Thu,  2 Nov 2017 17:33:33 +0100
>Jessica Yu <jeyu@kernel.org> 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 <pmladek@suse.com>
>> Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> ---
>>  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(&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.

Good point, and yes, normally this would be the (undesirable) outcome.

However in this case, kprobes_ftrace_ops has the IPMODIFY flag set, so
ftrace_set_filter_ip() will not allow the removal of a function if it
is the very last function, and it will return an error in
__ftrace_hash_update_ipmodify(). The comment there explains, if
IPMODIFY is set, "return -EINVAL if the new_hash tries to trace all
recs". So I think we are safe here...

>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 think this would've been a good alternative if we wanted to protect
against the empty ops case, but IPMODIFY is also incompatible with notrace..
(See: commit f8b8be8a310 and this comment here:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg688632.html)

Speaking of IPMODIFY, a question for Masami - is this flag still
relevant/needed for kprobes, since jprobes has been deprecated
recently? IIRC, IPMODIFY was needed in the first place because jprobes
and livepatch were in direct conflict, but I recall some work being
done in the past to remove the IPMODIFY flag from kprobes, but I don't
think this was ever upstreamed. (See: https://patchwork.kernel.org/patch/5352481/)

Thanks!

Jessica

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Masami Hiramatsu]

On Tue, 7 Nov 2017 18:14:56 +0100
Jessica Yu <jeyu@kernel.org> wrote:

> +++ Steven Rostedt [03/11/17 10:03 -0400]:
> >On Thu,  2 Nov 2017 17:33:33 +0100
> >Jessica Yu <jeyu@kernel.org> 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 <pmladek@suse.com>
> >> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> >> ---
> >>  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(&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.
> 
> Good point, and yes, normally this would be the (undesirable) outcome.
> 
> However in this case, kprobes_ftrace_ops has the IPMODIFY flag set, so
> ftrace_set_filter_ip() will not allow the removal of a function if it
> is the very last function, and it will return an error in
> __ftrace_hash_update_ipmodify(). The comment there explains, if
> IPMODIFY is set, "return -EINVAL if the new_hash tries to trace all
> recs". So I think we are safe here...
> 
> >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 think this would've been a good alternative if we wanted to protect
> against the empty ops case, but IPMODIFY is also incompatible with notrace..
> (See: commit f8b8be8a310 and this comment here:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg688632.html)
> 
> Speaking of IPMODIFY, a question for Masami - is this flag still
> relevant/needed for kprobes, since jprobes has been deprecated
> recently? IIRC, IPMODIFY was needed in the first place because jprobes
> and livepatch were in direct conflict, but I recall some work being
> done in the past to remove the IPMODIFY flag from kprobes, but I don't
> think this was ever upstreamed. (See: https://patchwork.kernel.org/patch/5352481/)

Hmm, good point. I just want to make kprobes transparently using ftrace.
This means if someone writes a kernel module which uses kprobes to
change IP address, which should work with/without CONFIG_FTRACE enabled.
kprobes itself supports to modify regs->ip (under some special settings,
see Documentation/kprobes.txt:Note for geeks), so we can not remove it.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

+++ Masami Hiramatsu [09/11/17 09:35 +0900]:
>On Tue, 7 Nov 2017 18:14:56 +0100
>Jessica Yu <jeyu@kernel.org> wrote:
>
>> +++ Steven Rostedt [03/11/17 10:03 -0400]:
>> >On Thu,  2 Nov 2017 17:33:33 +0100
>> >Jessica Yu <jeyu@kernel.org> 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 <pmladek@suse.com>
>> >> Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> >> ---
>> >>  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(&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.
>>
>> Good point, and yes, normally this would be the (undesirable) outcome.
>>
>> However in this case, kprobes_ftrace_ops has the IPMODIFY flag set, so
>> ftrace_set_filter_ip() will not allow the removal of a function if it
>> is the very last function, and it will return an error in
>> __ftrace_hash_update_ipmodify(). The comment there explains, if
>> IPMODIFY is set, "return -EINVAL if the new_hash tries to trace all
>> recs". So I think we are safe here...
>>
>> >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 think this would've been a good alternative if we wanted to protect
>> against the empty ops case, but IPMODIFY is also incompatible with notrace..
>> (See: commit f8b8be8a310 and this comment here:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg688632.html)
>>
>> Speaking of IPMODIFY, a question for Masami - is this flag still
>> relevant/needed for kprobes, since jprobes has been deprecated
>> recently? IIRC, IPMODIFY was needed in the first place because jprobes
>> and livepatch were in direct conflict, but I recall some work being
>> done in the past to remove the IPMODIFY flag from kprobes, but I don't
>> think this was ever upstreamed. (See: https://patchwork.kernel.org/patch/5352481/)
>
>Hmm, good point. I just want to make kprobes transparently using ftrace.
>This means if someone writes a kernel module which uses kprobes to
>change IP address, which should work with/without CONFIG_FTRACE enabled.
>kprobes itself supports to modify regs->ip (under some special settings,
>see Documentation/kprobes.txt:Note for geeks), so we can not remove it.

OK, makes sense. If kprobes keeps the ftrace IPMODIFY flag, then I
believe we are safe from the empty ops case Steven pointed out before,
since ftrace ops with IPMODIFY set require at least one entry in the
filter hash (an error is returned if an attempt is made to remove the
last function from the ops). I can add a comment noting this in the
error path since it doesn't look like this is explicitly documented.

Thanks for the comments, will send v3 shortly after the merge window.

Jessica

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Masami Hiramatsu]

On Wed, 3 Jan 2018 22:00:00 +0100
Jessica Yu <jeyu@kernel.org> wrote:

> +++ Steven Rostedt [03/01/18 09:33 -0500]:
> >On Wed,  3 Jan 2018 02:40:47 +0100
> >Jessica Yu <jeyu@kernel.org> 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 probes 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 <pmladek@suse.com>
> >> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> >> ---
> >>  kernel/kprobes.c | 94 +++++++++++++++++++++++++++++++++++++++++---------------
> >>  1 file changed, 69 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >> index b4aab48ad258..ae6b6fe79de3 100644
> >> --- a/kernel/kprobes.c
> >> +++ b/kernel/kprobes.c
> >> @@ -988,18 +988,32 @@ 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(&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;
> >
> >I wonder if we should change this from a WARN to a printk(). No reason
> >to do stack dumps here.
> 
> Yeah, I was trying to preserve the current behavior. I'll leave it up
> to Masami.

Thanks Jassica and Steve,

I wonder what are the possible cases of ftrace failure here. If it really rarely
happen, I would like to leave WARN() for debugging or reporting. But if there are
normal cases, we would better make it pr_warn() as Steve said.

> 
> >> +
> >> +	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:
> >> +	/*
> >> +	 * 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.
> >> +	 */
> >> +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
> >> +	return ret;
> >>  }
> >>
> >>  /* Caller must lock kprobe_mutex */
> >> @@ -1018,22 +1032,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
> >>  }
> >>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
> >>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
> >> -#define arm_kprobe_ftrace(p)	do {} while (0)
> >> +#define arm_kprobe_ftrace(p)	(0)
> >
> >Hmm. Perhaps we should have arm_kprobe_ftrace() return a failure.

Good catch!

> >
> >>  #define disarm_kprobe_ftrace(p)	do {} while (0)
> >>  #endif
> >>
> >>  /* Arm a kprobe with text_mutex */
> >> -static void arm_kprobe(struct kprobe *kp)
> >> +static int arm_kprobe(struct kprobe *kp)
> >>  {
> >> -	if (unlikely(kprobe_ftrace(kp))) {
> >> -		arm_kprobe_ftrace(kp);
> >> -		return;
> >> -	}
> >> +	if (unlikely(kprobe_ftrace(kp)))
> >> +		return arm_kprobe_ftrace(kp);
> >
> >If CONFIG_KPROBES_ON_FTRACE is not defined, this if should always be
> >false. But if for some reason in the future, it is not false, we just
> >had arm_kprobe_ftrace() return success when it really is a failure.
> >
> > -ENODEV?
> 
> Good point, I will include this change in v4, unless there are
> objections.

I have no objection :)

> 
> >> +
> >>  	cpus_read_lock();
> >>  	mutex_lock(&text_mutex);
> >>  	__arm_kprobe(kp);
> >>  	mutex_unlock(&text_mutex);
> >>  	cpus_read_unlock();
> >> +
> >> +	return 0;
> >>  }
> >>
> >>  /* Disarm a kprobe with text_mutex */
> >> @@ -1372,9 +1387,15 @@ 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(&p->list);
> >
> >Don't we need to hold the mutex to modify the list?
> 
> It is unfortunately unclear from this snippet, but we do hold the
> kprobe_mutex here. It's held for most of the duration of
> register_kprobe(), where register_aggr_kprobe() is called.

Right, we already hold kprobe_mutex here so it is safe. :)

> 
> >> +				synchronize_sched();
> >> +			}
> >> +		}
> >>  	}
> >>  	return ret;
> >>  }
> >> @@ -1594,8 +1615,14 @@ int register_kprobe(struct kprobe *p)
> >>  	hlist_add_head_rcu(&p->hlist,
> >>  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
> >>
> >> -	if (!kprobes_all_disarmed && !kprobe_disabled(p))
> >> -		arm_kprobe(p);
> >> +	if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
> >> +		ret = arm_kprobe(p);
> >> +		if (ret) {
> >> +			hlist_del_rcu(&p->hlist);
> >
> >Same here.
> 
> We do hold kprobe_mutex here as well (see above comment).
> 
> >> +			synchronize_sched();
> >> +			goto out;
> >> +		}
> >> +	}
> >>
> >>  	/* Try to optimize kprobe */
> >>  	try_to_optimize_kprobe(p);
> >> @@ -2137,7 +2164,9 @@ int enable_kprobe(struct kprobe *kp)
> >>
> >>  	if (!kprobes_all_disarmed && kprobe_disabled(p)) {
> >>  		p->flags &= ~KPROBE_FLAG_DISABLED;
> >> -		arm_kprobe(p);
> >> +		ret = arm_kprobe(p);
> >> +		if (ret)
> >> +			p->flags |= KPROBE_FLAG_DISABLED;
> >>  	}
> >>  out:
> >>  	mutex_unlock(&kprobe_mutex);
> >> @@ -2565,11 +2594,12 @@ static const struct file_operations debugfs_kprobe_ei_ops = {
> >>  	.release        = seq_release,
> >>  };
> >>
> >> -static void arm_all_kprobes(void)
> >> +static int arm_all_kprobes(void)
> >>  {
> >>  	struct hlist_head *head;
> >>  	struct kprobe *p;
> >> -	unsigned int i;
> >> +	unsigned int i, errors = 0;
> >> +	int err, ret = 0;
> >>
> >>  	mutex_lock(&kprobe_mutex);
> >>
> >> @@ -2586,16 +2616,26 @@ static void arm_all_kprobes(void)
> >>  	/* Arming kprobes doesn't optimize kprobe itself */
> >>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> >>  		head = &kprobe_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 probes\n", errors);
> >
> >Perhaps we should have a count of all kprobes that were tried, and
> >write something like:
> >
> > "Kprobes globally enabled, but failed to arm %d out of %d probes\n",
> >   errors, total

Sounds good to me :)

Thanks!

> 
> Sure, ok.
> 
> Thank you for the review!
> 
> Jessica
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

+++ Steven Rostedt [03/01/18 09:33 -0500]:
>On Wed,  3 Jan 2018 02:40:47 +0100
>Jessica Yu <jeyu@kernel.org> 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 probes 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 <pmladek@suse.com>
>> Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> ---
>>  kernel/kprobes.c | 94 +++++++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 69 insertions(+), 25 deletions(-)
>>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index b4aab48ad258..ae6b6fe79de3 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -988,18 +988,32 @@ 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(&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;
>
>I wonder if we should change this from a WARN to a printk(). No reason
>to do stack dumps here.

Yeah, I was trying to preserve the current behavior. I'll leave it up
to Masami.

>> +
>> +	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:
>> +	/*
>> +	 * 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.
>> +	 */
>> +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
>> +	return ret;
>>  }
>>
>>  /* Caller must lock kprobe_mutex */
>> @@ -1018,22 +1032,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
>>  }
>>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
>>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
>> -#define arm_kprobe_ftrace(p)	do {} while (0)
>> +#define arm_kprobe_ftrace(p)	(0)
>
>Hmm. Perhaps we should have arm_kprobe_ftrace() return a failure.
>
>>  #define disarm_kprobe_ftrace(p)	do {} while (0)
>>  #endif
>>
>>  /* Arm a kprobe with text_mutex */
>> -static void arm_kprobe(struct kprobe *kp)
>> +static int arm_kprobe(struct kprobe *kp)
>>  {
>> -	if (unlikely(kprobe_ftrace(kp))) {
>> -		arm_kprobe_ftrace(kp);
>> -		return;
>> -	}
>> +	if (unlikely(kprobe_ftrace(kp)))
>> +		return arm_kprobe_ftrace(kp);
>
>If CONFIG_KPROBES_ON_FTRACE is not defined, this if should always be
>false. But if for some reason in the future, it is not false, we just
>had arm_kprobe_ftrace() return success when it really is a failure.
>
> -ENODEV?

Good point, I will include this change in v4, unless there are
objections.

>> +
>>  	cpus_read_lock();
>>  	mutex_lock(&text_mutex);
>>  	__arm_kprobe(kp);
>>  	mutex_unlock(&text_mutex);
>>  	cpus_read_unlock();
>> +
>> +	return 0;
>>  }
>>
>>  /* Disarm a kprobe with text_mutex */
>> @@ -1372,9 +1387,15 @@ 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(&p->list);
>
>Don't we need to hold the mutex to modify the list?

It is unfortunately unclear from this snippet, but we do hold the
kprobe_mutex here. It's held for most of the duration of
register_kprobe(), where register_aggr_kprobe() is called.

>> +				synchronize_sched();
>> +			}
>> +		}
>>  	}
>>  	return ret;
>>  }
>> @@ -1594,8 +1615,14 @@ int register_kprobe(struct kprobe *p)
>>  	hlist_add_head_rcu(&p->hlist,
>>  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
>>
>> -	if (!kprobes_all_disarmed && !kprobe_disabled(p))
>> -		arm_kprobe(p);
>> +	if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
>> +		ret = arm_kprobe(p);
>> +		if (ret) {
>> +			hlist_del_rcu(&p->hlist);
>
>Same here.

We do hold kprobe_mutex here as well (see above comment).

>> +			synchronize_sched();
>> +			goto out;
>> +		}
>> +	}
>>
>>  	/* Try to optimize kprobe */
>>  	try_to_optimize_kprobe(p);
>> @@ -2137,7 +2164,9 @@ int enable_kprobe(struct kprobe *kp)
>>
>>  	if (!kprobes_all_disarmed && kprobe_disabled(p)) {
>>  		p->flags &= ~KPROBE_FLAG_DISABLED;
>> -		arm_kprobe(p);
>> +		ret = arm_kprobe(p);
>> +		if (ret)
>> +			p->flags |= KPROBE_FLAG_DISABLED;
>>  	}
>>  out:
>>  	mutex_unlock(&kprobe_mutex);
>> @@ -2565,11 +2594,12 @@ static const struct file_operations debugfs_kprobe_ei_ops = {
>>  	.release        = seq_release,
>>  };
>>
>> -static void arm_all_kprobes(void)
>> +static int arm_all_kprobes(void)
>>  {
>>  	struct hlist_head *head;
>>  	struct kprobe *p;
>> -	unsigned int i;
>> +	unsigned int i, errors = 0;
>> +	int err, ret = 0;
>>
>>  	mutex_lock(&kprobe_mutex);
>>
>> @@ -2586,16 +2616,26 @@ static void arm_all_kprobes(void)
>>  	/* Arming kprobes doesn't optimize kprobe itself */
>>  	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>>  		head = &kprobe_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 probes\n", errors);
>
>Perhaps we should have a count of all kprobes that were tried, and
>write something like:
>
> "Kprobes globally enabled, but failed to arm %d out of %d probes\n",
>   errors, total

Sure, ok.

Thank you for the review!

Jessica

Re: kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

+++ Masami Hiramatsu [05/10/17 06:23 +0000]:
>Hi Jessica,
>
>On Wed,  4 Oct 2017 21:14:13 +0200
>Jessica Yu <jeyu@kernel.org> 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.
>
>Ok, I have some comment. See below.
>
>>
>> Based-on-patches-by: Petr Mladek <pmladek@suse.com>
>> Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> ---
>>  kernel/kprobes.c | 87 +++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 61 insertions(+), 26 deletions(-)
>>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 2d28377a0e32..6e889be0d93c 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -979,18 +979,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(&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);
>> +	return ret;
>>  }
>>
>>  /* Caller must lock kprobe_mutex */
>> @@ -1009,22 +1018,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
>>  }
>>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
>>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
>> -#define arm_kprobe_ftrace(p)	do {} while (0)
>> +#define arm_kprobe_ftrace(p)	(0)
>>  #define disarm_kprobe_ftrace(p)	do {} while (0)
>>  #endif
>>
>>  /* Arm a kprobe with text_mutex */
>> -static void arm_kprobe(struct kprobe *kp)
>> +static int arm_kprobe(struct kprobe *kp)
>>  {
>> -	if (unlikely(kprobe_ftrace(kp))) {
>> -		arm_kprobe_ftrace(kp);
>> -		return;
>> -	}
>> +	if (unlikely(kprobe_ftrace(kp)))
>> +		return arm_kprobe_ftrace(kp);
>> +
>>  	cpus_read_lock();
>>  	mutex_lock(&text_mutex);
>>  	__arm_kprobe(kp);
>>  	mutex_unlock(&text_mutex);
>>  	cpus_read_unlock();
>> +
>> +	return 0;
>>  }
>>
>>  /* Disarm a kprobe with text_mutex */
>> @@ -1363,9 +1373,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(&p->list);
>
>Nice catch :) this list_del_rcu() is important to keep error case
>behavior sane.
>
>> +			}
>> +		}
>>  	}
>>  	return ret;
>>  }
>> @@ -1570,13 +1585,16 @@ int register_kprobe(struct kprobe *p)
>>  	if (ret)
>>  		goto out;
>>
>> +	if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
>> +		ret = arm_kprobe(p);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>
>No, this is no good. It is a small chance to hit kprobe on other
>CPUs before adding it to kprobe_table hashlist. In that case,
>we will see a stray breakpoint instruction.

Ah yes, you are right, this is incorrect. There is a short window of
time where we could have a stray breakpoint from an armed kprobe, but
the breakpoint handler would not be able to find the associated kprobe
in the hashlist. Will fix this in v2.

>>  	INIT_HLIST_NODE(&p->hlist);
>>  	hlist_add_head_rcu(&p->hlist,
>>  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
>>
>> -	if (!kprobes_all_disarmed && !kprobe_disabled(p))
>> -		arm_kprobe(p);
>> -
>
>So, you'll have to rollback by hlist_del_rcu() here.
>Hmm, by the way, in this case, you also have to add a synchronize_rcu()
>in the end of error path, so that user can release kprobe right after
>error return of register_kprobe... (I think that's OK because it is not
>a hot path)

Yes, I'll fix this in the error path as well. Thank you for your
comments! Will send a v2.

Jessica