Return proper error code from register_kprobe()

PATCH In review Prashanth Nageshappa 2 participants 2 messages Feb 15, 2012

Subsystems (2)

Stable branch kprobes

lore

Share & tools

lore

Prashanth Nageshappa

author

2012-02-15 07:16 UTC #0 lore raw

PATCH v2 1/1 Return proper error code from register_kprobe()

Missed lkml in my previous posting, hence resending....

register_kprobe() aborts if the address of the new request falls in a prohibited area (such as ftrace pouch, __kprobes annotated functions, non-kernel text addresses, jump label text). We however don't return the right error on this abort, resulting in a silent failure - incorrect adding/reporting of kprobes ('perf probe do_fork+18' or 'perf probe mcount' for instance).

In V2 we are incorporating Masami Hiramatsu's feedback.

This patch fixes it by returning -EINVAL upon failure.

While we are here, rename the label used for exit to be more appropriate.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Prashanth K Nageshappa <[email protected]>

Cc [email protected]

---

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

kernel/kprobes.c +7 -5

Index: linux-14feb/kernel/kprobes.c
===================================================================
@@ -1334,8 +1334,10 @@ int __kprobes register_kprobe(struct kpr
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
 	    ftrace_text_reserved(p->addr, p->addr) ||
-	    jump_label_text_reserved(p->addr, p->addr))
-		goto fail_with_jump_label;
+	    jump_label_text_reserved(p->addr, p->addr)) {
+		ret = -EINVAL;
+		goto cannot_probe;
+	}
 
 	/* User can pass only KPROBE_FLAG_DISABLED to register_kprobe */
 	p->flags &= KPROBE_FLAG_DISABLED;

@@ -1352,7 +1354,7 @@ int __kprobes register_kprobe(struct kpr
 		 * its code to prohibit unexpected unloading.
 		 */
 		if (unlikely(!try_module_get(probed_mod)))
-			goto fail_with_jump_label;
+			goto cannot_probe;
 
 		/*
 		 * If the module freed .init.text, we couldn't insert

@@ -1361,7 +1363,7 @@ int __kprobes register_kprobe(struct kpr
 		if (within_module_init((unsigned long)p->addr, probed_mod) &&
 		    probed_mod->state != MODULE_STATE_COMING) {
 			module_put(probed_mod);
-			goto fail_with_jump_label;
+			goto cannot_probe;
 		}
 		/* ret will be updated by following code */
 	}

@@ -1409,7 +1411,7 @@ int __kprobes register_kprobe(struct kpr
 
 	return ret;
 
-fail_with_jump_label:
+cannot_probe:
 	preempt_enable();
 	jump_label_unlock();
 	return ret;

Andrew Morton

2012-02-15 21:23 UTC #1 lore raw

Quoted 2 lines · On Wed, 15 Feb 2012 12:46:03 +0530 Prashanth Nageshappa…

register_kprobe() aborts if the address of the new request falls in a prohibited area (such as ftrace pouch, __kprobes annotated functions,

I'm sitting here wondering what you meant to type when you typed "ftrace pouch". I'm stumped! But you're not allowed to tell us - that would take all the fun out of it.

Quoted 7 lines

non-kernel text addresses, jump label text). We however don't return the right error on this abort, resulting in a silent failure - incorrect adding/reporting of kprobes ('perf probe do_fork+18' or 'perf probe mcount' for instance).
In V2 we are incorporating Masami Hiramatsu's feedback.

Quoted 7 lines

This patch fixes it by returning -EINVAL upon failure.
While we are here, rename the label used for exit to be more appropriate.
Signed-off-by Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by Prashanth K Nageshappa <[email protected]>
Cc [email protected]

So we want this for 3.3 with a -stable backport.