[PATCH 3/7] kprobes: Propagate error from disarm_kprobe_ftrace()

[Petr Mladek <pmladek@suse.cz>]

Also disarm_kprobe_ftrace() could fail, for example if there is an internal
error in the Kprobe code and we try to unregister some Kprobe that is not
registered.

If we fail to unregister the ftrace function, we still could try to disarm
the Kprobe by removing the filter. This is why the first error code is not
fatal and we try to continue.

__disable_kprobe() has to return the error code via ERR_PTR. It allows to
pass -EINVAL instead of NULL for invalid Kprobes. Then the NULL pointer does
not need to be transformed into -EINVAL later.

In addition, __disable_kprobe() should disable the child probe only when
the parent probe has been successfully disabled. Note that we could always
clear KPROBE_FLAG_DISABLED in case of error. It does not harm even when
p == orig_p. It keeps the code nesting on reasonable level.

The error handling is a bit complicated in __unregister_kprobe_top().
It is used in unregister_*probes() that are used in module removal callbacks.
Any error here could not stop the removal of the module and the related Kprobe
handlers. Therefore such an error is fatal. There is already a similar check
in the "disarmed:" goto target.

The only exception is when we try to unregister an invalid Kprobe. It does not
exist and therefore should not harm the system. This error was weak even before.

disable_kprobe() just passes the error as it did before.

disarm_all_kprobes() uses similar approach like arm_all_kprobes(). It keeps
the current behavior and does the best effort. It tries to disable as many
Kprobes as possible. It always reports success. It returns the last error
code. There is going to be separate patch that will improve this behavior.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 kernel/kprobes.c | 77 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a69d23add983..ba57147bd52c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -962,23 +962,26 @@ err_filter:
 }
 
 /* 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);
+		WARN(ret < 0, "Failed to remove kprobe-ftrace (%d)\n", ret);
 	}
+	if (!ret)
+		kprobe_ftrace_enabled--;
+
 	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
-			   (unsigned long)p->addr, 1, 0);
+				   (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 */
@@ -998,16 +1001,15 @@ 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);
 	/* Ditto */
 	mutex_lock(&text_mutex);
 	__disarm_kprobe(kp, reopt);
 	mutex_unlock(&text_mutex);
+	return 0;
 }
 
 /*
@@ -1585,11 +1587,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 static struct kprobe *__disable_kprobe(struct kprobe *p)
 {
 	struct kprobe *orig_p;
+	int err;
 
 	/* 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 */
@@ -1598,7 +1601,11 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
 
 		/* Try to disarm and disable this/parent probe */
 		if (p == orig_p || aggr_kprobe_disabled(orig_p)) {
-			disarm_kprobe(orig_p, true);
+			err = disarm_kprobe(orig_p, true);
+			if (err) {
+				p->flags &= ~KPROBE_FLAG_DISABLED;
+				return ERR_PTR(err);
+			}
 			orig_p->flags |= KPROBE_FLAG_DISABLED;
 		}
 	}
@@ -1615,8 +1622,17 @@ 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;
+	/*
+	 * We could not prevent Kprobe handlers from going. Therefore
+	 * we rather do not continue when the Kprobe is still enabled.
+	 * The only exception is an invalid Kprobe. It does not exist
+	 * and therefore could not harm the system.
+	 */
+	if (IS_ERR(ap)) {
+		if (PTR_ERR(ap) == -EINVAL)
+			return PTR_ERR(ap);
+		BUG();
+	}
 
 	if (ap == p)
 		/*
@@ -2012,12 +2028,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;
@@ -2367,34 +2385,41 @@ already_enabled:
 	return ret;
 }
 
-static void disarm_all_kprobes(void)
+static int disarm_all_kprobes(void)
 {
 	struct hlist_head *head;
 	struct kprobe *p;
 	unsigned int i;
+	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];
 		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)
+					ret = err;
+			}
 		}
 	}
+
+	kprobes_all_disarmed = true;
+	pr_info("Kprobes globally disabled\n");
+
 	mutex_unlock(&kprobe_mutex);
 
 	/* Wait for disarming all kprobes by optimizer */
 	wait_for_kprobe_optimizer();
+
+	return ret;
 }
 
 /*
@@ -2437,7 +2462,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
 	case 'n':
 	case 'N':
 	case '0':
-		disarm_all_kprobes();
+		err = disarm_all_kprobes();
 		break;
 	default:
 		return -EINVAL;
-- 
1.8.5.6