[PATCH -tip 0/5] kprobes: Remove BUG_ON from kprobes

[PATCH -tip 0/5] kprobes: Remove BUG_ON from kprobes

[Masami Hiramatsu]

Hi,

Here are patches to remove BUG_ON from kprobes.

Since BUG_ON() kills the machine, it must not be used
for debugging or assertion. These patches remove
meaningless BUG_ON or replace it with return errors.

Thank you,

---

Masami Hiramatsu (5):
      kprobes: Remove meaningless BUG_ON from disarming process
      kprobes: Remove meaningless BUG_ON from add_new_kprobe
      kprobes: Remove meaningless BUG_ON from reuse_unused_kprobe
      kprobes: Return error if fail to reuse kprobe instead of BUG_ON
      kprobes: Don't call BUG_ON if there is a kprobe in use on free list


 kernel/kprobes.c |   39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH -tip 1/5] kprobes: Remove meaningless BUG_ON from disarming process

[Masami Hiramatsu]

All aggr_probe at this line are already disarmed by
disable_kprobe() or checked by kprobe_disarmed().
So this BUG_ON() does meaningless check. Remove it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ab257be4d924..d1edd8d5641e 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1704,7 +1704,6 @@ static int __unregister_kprobe_top(struct kprobe *p)
 	return 0;
 
 disarmed:
-	BUG_ON(!kprobe_disarmed(ap));
 	hlist_del_rcu(&ap->hlist);
 	return 0;
 }

[PATCH -tip 2/5] kprobes: Remove meaningless BUG_ON from add_new_kprobe

[Masami Hiramatsu]

Before calling add_new_kprobe(), aggr_probe's GONE
flag and kprobe GONE flag are cleared. We don't need
to worry about that flag at this point.

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

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d1edd8d5641e..231569e1e2c8 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1259,8 +1259,6 @@ NOKPROBE_SYMBOL(cleanup_rp_inst);
 /* Add the new probe to ap->list */
 static int add_new_kprobe(struct kprobe *ap, struct kprobe *p)
 {
-	BUG_ON(kprobe_gone(ap) || kprobe_gone(p));
-
 	if (p->post_handler)
 		unoptimize_kprobe(ap, true);	/* Fall back to normal kprobe */
 

[PATCH -tip 3/5] kprobes: Remove meaningless BUG_ON from reuse_unused_kprobe

[Masami Hiramatsu]

Since reuse_unused_kprobe is called when the given kprobe
is unused, checking it inside again with BUG_ON is
meaningless. Remove it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 231569e1e2c8..277a6cbe83db 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -704,7 +704,6 @@ static void reuse_unused_kprobe(struct kprobe *ap)
 {
 	struct optimized_kprobe *op;
 
-	BUG_ON(!kprobe_unused(ap));
 	/*
 	 * Unused kprobe MUST be on the way of delayed unoptimizing (means
 	 * there is still a relative jump) and disabled.

[PATCH -tip 4/5] kprobes: Return error if fail to reuse kprobe instead of BUG_ON

[Masami Hiramatsu]

Make reuse_unused_kprobe() to return error code if
it fails to reuse unused kprobe for optprobe instead
of calling BUG_ON().

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

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 277a6cbe83db..63c342e5e6c3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -700,9 +700,10 @@ static void unoptimize_kprobe(struct kprobe *p, bool force)
 }
 
 /* Cancel unoptimizing for reusing */
-static void reuse_unused_kprobe(struct kprobe *ap)
+static int reuse_unused_kprobe(struct kprobe *ap)
 {
 	struct optimized_kprobe *op;
+	int ret;
 
 	/*
 	 * Unused kprobe MUST be on the way of delayed unoptimizing (means
@@ -713,8 +714,12 @@ static void reuse_unused_kprobe(struct kprobe *ap)
 	/* Enable the probe again */
 	ap->flags &= ~KPROBE_FLAG_DISABLED;
 	/* Optimize it again (remove from op->list) */
-	BUG_ON(!kprobe_optready(ap));
+	ret = kprobe_optready(ap);
+	if (ret)
+		return ret;
+
 	optimize_kprobe(ap);
+	return 0;
 }
 
 /* Remove optimized instructions */
@@ -939,11 +944,16 @@ static void __disarm_kprobe(struct kprobe *p, bool reopt)
 #define kprobe_disarmed(p)			kprobe_disabled(p)
 #define wait_for_kprobe_optimizer()		do {} while (0)
 
-/* There should be no unused kprobes can be reused without optimization */
-static void reuse_unused_kprobe(struct kprobe *ap)
+static int reuse_unused_kprobe(struct kprobe *ap)
 {
+	/*
+	 * If the optimized kprobe is NOT supported, the aggr kprobe is
+	 * released at the same time that the last aggregated kprobe is
+	 * unregistered.
+	 * Thus there should be no chance to reuse unused kprobe.
+	 */
 	printk(KERN_ERR "Error: There should be no unused kprobe here.\n");
-	BUG_ON(kprobe_unused(ap));
+	return -EINVAL;
 }
 
 static void free_aggr_kprobe(struct kprobe *p)
@@ -1315,9 +1325,12 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 			goto out;
 		}
 		init_aggr_kprobe(ap, orig_p);
-	} else if (kprobe_unused(ap))
+	} else if (kprobe_unused(ap)) {
 		/* This probe is going to die. Rescue it */
-		reuse_unused_kprobe(ap);
+		ret = reuse_unused_kprobe(ap);
+		if (ret)
+			goto out;
+	}
 
 	if (kprobe_gone(ap)) {
 		/*

[PATCH -tip 5/5] kprobes: Don't call BUG_ON if there is a kprobe in use on free list

[Masami Hiramatsu]

Instead of calling BUG_ON, if we find a kprobe in use on free kprobe
list, just remove it from the list and keep it on kprobe hash list
as same as other in-use kprobes.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 63c342e5e6c3..e3420364b415 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -546,8 +546,14 @@ static void do_free_cleaned_kprobes(void)
 	struct optimized_kprobe *op, *tmp;
 
 	list_for_each_entry_safe(op, tmp, &freeing_list, list) {
-		BUG_ON(!kprobe_unused(&op->kp));
 		list_del_init(&op->list);
+		if (!kprobe_unused(&op->kp)) {
+			/*
+			 * This must not happen, but if there is a kprobe
+			 * still in use, keep it on kprobes hash list.
+			 */
+			continue;
+		}
 		free_aggr_kprobe(&op->kp);
 	}
 }

Re: [PATCH -tip 5/5] kprobes: Don't call BUG_ON if there is a kprobe in use on free list

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Instead of calling BUG_ON, if we find a kprobe in use on free kprobe
> list, just remove it from the list and keep it on kprobe hash list
> as same as other in-use kprobes.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 63c342e5e6c3..e3420364b415 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -546,8 +546,14 @@ static void do_free_cleaned_kprobes(void)
>  	struct optimized_kprobe *op, *tmp;
>  
>  	list_for_each_entry_safe(op, tmp, &freeing_list, list) {
> -		BUG_ON(!kprobe_unused(&op->kp));
>  		list_del_init(&op->list);
> +		if (!kprobe_unused(&op->kp)) {
> +			/*
> +			 * This must not happen, but if there is a kprobe
> +			 * still in use, keep it on kprobes hash list.
> +			 */
> +			continue;

If this is an 'impossible' code path then I think it would make sense to add a WARN_ON_ONCE() 
here.

Thanks,

	Ingo

Re: [PATCH -tip 5/5] kprobes: Don't call BUG_ON if there is a kprobe in use on free list

[Masami Hiramatsu]

On Mon, 10 Sep 2018 14:23:56 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Instead of calling BUG_ON, if we find a kprobe in use on free kprobe
> > list, just remove it from the list and keep it on kprobe hash list
> > as same as other in-use kprobes.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/kprobes.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 63c342e5e6c3..e3420364b415 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -546,8 +546,14 @@ static void do_free_cleaned_kprobes(void)
> >  	struct optimized_kprobe *op, *tmp;
> >  
> >  	list_for_each_entry_safe(op, tmp, &freeing_list, list) {
> > -		BUG_ON(!kprobe_unused(&op->kp));
> >  		list_del_init(&op->list);
> > +		if (!kprobe_unused(&op->kp)) {
> > +			/*
> > +			 * This must not happen, but if there is a kprobe
> > +			 * still in use, keep it on kprobes hash list.
> > +			 */
> > +			continue;
> 
> If this is an 'impossible' code path then I think it would make sense to add a WARN_ON_ONCE() 
> here.

I agree. This means something goes wrong. That is enough reason to warn user.

Thank you!

> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>