tracing/kprobe: Fix to support kretprobe events on unloaded modules

PATCH In review Masami Hiramatsu 3 participants 4 messages Jan 27, 2021 – Jan 30, 2021 2 messages not in the archive

Subsystems (2)

Tracing kprobes

lore

Share & tools

lore

Masami Hiramatsu

author maintainer

2021-01-27 15:43 UTC #0 lore raw

PATCH 1/1 tracing/kprobe: Fix to support kretprobe events on unloaded modules

Fix kprobe_on_func_entry() returns error code instead of false so that register_kretprobe() can return an appropriate error code.

append_trace_kprobe() expects the kprobe registration returns -ENOENT when the target symbol is not found, and it checks whether the target module is unloaded or not. If the target module doesn't exist, it defers to probe the target symbol until the module is loaded.

However, since register_kretprobe() returns -EINVAL instead of -ENOENT in that case, it always fail on putting the kretprobe event on unloaded modules. e.g.

Kprobe event:
/sys/kernel/debug/tracing # echo p xfs:xfs_end_io >> kprobe_events

[   16.515574] trace_kprobe: This probe might be able to register after target module is loaded. Continue.

Kretprobe event: (p -> r)
/sys/kernel/debug/tracing # echo r xfs:xfs_end_io >> kprobe_events
sh: write error: Invalid argument /sys/kernel/debug/tracing # cat error_log

[   41.122514] trace_kprobe: error: Failed to register probe event
  Command: r xfs:xfs_end_io
             ^

To fix this bug, change kprobe_on_func_entry() to detect symbol lookup failure and return -ENOENT in that case. Otherwise it returns -EINVAL or 0 (succeeded, given address is on the entry).

Reported-by Jianlin Lv <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 include/linux/kprobes.h     |    2 +-
 kernel/kprobes.c            |   34 +++++++++++++++++++++++++---------
 kernel/trace/trace_kprobe.c |   10 ++++++----
 3 files changed, 32 insertions(+), 14 deletions(-)

include/linux/kprobes.h +1 -1

index b3a36b0cfc81..1883a4a9f16a 100644
@@ -266,7 +266,7 @@ extern void kprobes_inc_nmissed_count(struct kprobe *p);
 extern bool arch_within_kprobe_blacklist(unsigned long addr);
 extern int arch_populate_kprobe_blacklist(void);
 extern bool arch_kprobe_on_func_entry(unsigned long offset);
-extern bool kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
+extern int kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
 
 extern bool within_kprobe_blacklist(unsigned long addr);
 extern int kprobe_add_ksym_blacklist(unsigned long entry);

kernel/kprobes.c +25 -9

index f7fb5d135930..1a5bc321e0a5 100644
@@ -1954,29 +1954,45 @@ bool __weak arch_kprobe_on_func_entry(unsigned long offset)
 	return !offset;
 }
 
-bool kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset)
+/**
+ * kprobe_on_func_entry() -- check whether given address is function entry
+ * @addr: Target address
+ * @sym:  Target symbol name
+ * @offset: The offset from the symbol or the address
+ *
+ * This checks whether the given @addr+@offset or @sym+@offset is on the
+ * function entry address or not.
+ * This returns 0 if it is the function entry, or -EINVAL if it is not.
+ * And also it returns -ENOENT if it fails the symbol or address lookup.
+ * Caller must pass @addr or @sym (either one must be NULL), or this
+ * returns -EINVAL.
+ */
+int kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset)
 {
 	kprobe_opcode_t *kp_addr = _kprobe_addr(addr, sym, offset);
 
 	if (IS_ERR(kp_addr))
-		return false;
+		return PTR_ERR(kp_addr);
 
-	if (!kallsyms_lookup_size_offset((unsigned long)kp_addr, NULL, &offset) ||
-						!arch_kprobe_on_func_entry(offset))
-		return false;
+	if (!kallsyms_lookup_size_offset((unsigned long)kp_addr, NULL, &offset))
+		return -ENOENT;
 
-	return true;
+	if (!arch_kprobe_on_func_entry(offset))
+		return -EINVAL;
+
+	return 0;
 }
 
 int register_kretprobe(struct kretprobe *rp)
 {
-	int ret = 0;
+	int ret;
 	struct kretprobe_instance *inst;
 	int i;
 	void *addr;
 
-	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
-		return -EINVAL;
+	ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
+	if (ret)
+		return ret;
 
 	if (kretprobe_blacklist_size) {
 		addr = kprobe_addr(&rp->kp);

kernel/trace/trace_kprobe.c +6 -4

index e6fba1798771..56c7fbff7bd7 100644
@@ -221,9 +221,9 @@ bool trace_kprobe_on_func_entry(struct trace_event_call *call)
 {
 	struct trace_kprobe *tk = trace_kprobe_primary_from_call(call);
 
-	return tk ? kprobe_on_func_entry(tk->rp.kp.addr,
+	return tk ? (kprobe_on_func_entry(tk->rp.kp.addr,
 			tk->rp.kp.addr ? NULL : tk->rp.kp.symbol_name,
-			tk->rp.kp.addr ? 0 : tk->rp.kp.offset) : false;
+			tk->rp.kp.addr ? 0 : tk->rp.kp.offset) == 0) : false;
 }
 
 bool trace_kprobe_error_injectable(struct trace_event_call *call)

@@ -828,9 +828,11 @@ static int trace_kprobe_create(int argc, const char *argv[])
 		}
 		if (is_return)
 			flags |= TPARG_FL_RETURN;
-		if (kprobe_on_func_entry(NULL, symbol, offset))
+		ret = kprobe_on_func_entry(NULL, symbol, offset);
+		if (ret == 0)
 			flags |= TPARG_FL_FENTRY;
-		if (offset && is_return && !(flags & TPARG_FL_FENTRY)) {
+		/* Defer the ENOENT case until register kprobe */
+		if (ret == -EINVAL && is_return) {
 			trace_probe_log_err(0, BAD_RETPROBE);
 			goto parse_error;
 		}

Steven Rostedt

maintainer

2021-01-27 23:14 UTC #1 lore raw

Quoted 24 lines · On Thu, 28 Jan 2021 00:37:51 +0900 Masami Hiramatsu <[email protected]> wrote:

Fix kprobe_on_func_entry() returns error code instead of false so that register_kretprobe() can return an appropriate error code.
append_trace_kprobe() expects the kprobe registration returns -ENOENT when the target symbol is not found, and it checks whether the target module is unloaded or not. If the target module doesn't exist, it defers to probe the target symbol until the module is loaded.
However, since register_kretprobe() returns -EINVAL instead of -ENOENT in that case, it always fail on putting the kretprobe event on unloaded modules. e.g.
Kprobe event:
/sys/kernel/debug/tracing # echo p xfs:xfs_end_io >> kprobe_events
[   16.515574] trace_kprobe: This probe might be able to register after target module is loaded. Continue.
Kretprobe event: (p -> r)
/sys/kernel/debug/tracing # echo r xfs:xfs_end_io >> kprobe_events
sh: write error: Invalid argument /sys/kernel/debug/tracing # cat error_log
[   41.122514] trace_kprobe: error: Failed to register probe event
  Command: r xfs:xfs_end_io
             ^

Quoted 6 lines

To fix this bug, change kprobe_on_func_entry() to detect symbol lookup failure and return -ENOENT in that case. Otherwise it returns -EINVAL or 0 (succeeded, given address is on the entry).
Reported-by Jianlin Lv <[email protected]>
Signed-off-by Masami Hiramatsu <[email protected]>

Is this something that should go to stable? And if so, can you supply a Fixes tag?

Thanks Masami,

-- Steve

Masami Hiramatsu

author maintainer

2021-01-28 00:12 UTC #2 lore raw

Quoted 31 lines · On Wed, 27 Jan 2021 17:29:50 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 28 lines · On Thu, 28 Jan 2021 00:37:51 +0900 Masami Hiramatsu <[email protected]> wrote:
Fix kprobe_on_func_entry() returns error code instead of false so that register_kretprobe() can return an appropriate error code.
append_trace_kprobe() expects the kprobe registration returns -ENOENT when the target symbol is not found, and it checks whether the target module is unloaded or not. If the target module doesn't exist, it defers to probe the target symbol until the module is loaded.
However, since register_kretprobe() returns -EINVAL instead of -ENOENT in that case, it always fail on putting the kretprobe event on unloaded modules. e.g.
Kprobe event:
/sys/kernel/debug/tracing # echo p xfs:xfs_end_io >> kprobe_events
[   16.515574] trace_kprobe: This probe might be able to register after target module is loaded. Continue.
Kretprobe event: (p -> r)
/sys/kernel/debug/tracing # echo r xfs:xfs_end_io >> kprobe_events
sh: write error: Invalid argument /sys/kernel/debug/tracing # cat error_log
[   41.122514] trace_kprobe: error: Failed to register probe event
  Command: r xfs:xfs_end_io
             ^
To fix this bug, change kprobe_on_func_entry() to detect symbol lookup failure and return -ENOENT in that case. Otherwise it returns -EINVAL or 0 (succeeded, given address is on the entry).

Quoted 5 lines

Quoted 2 lines
Reported-by Jianlin Lv <[email protected]>
Signed-off-by Masami Hiramatsu <[email protected]>
Is this something that should go to stable? And if so, can you supply a Fixes tag?

Yes, I thought that had not supported previously, but that's wrong. I found below commit introduced -ENOENT check in trace_kprobe side.

Fixes 59158ec4aef7 ("tracing/kprobes: Check the probe on unloaded module correctly")

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

A message that is not in the archive (sent off-list or not yet received): AM6PR08MB3589FAFEE0CDE17DEC647FAC98B99@AM6PR08MB3589.eurprd08.prod.outlook.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jianlin Lv

2021-01-30 10:10 UTC #5 lore raw

Quoted 30 lines

-----Original Message-----
From: Masami Hiramatsu <[email protected]>
Sent: Friday, January 29, 2021 9:45 PM
To: Jianlin Lv <[email protected]>
Cc: Steven Rostedt <[email protected]>; Oleg Nesterov <[email protected]>; Ingo Molnar <[email protected]>; linux- [email protected]
Subject: Re: [PATCH] tracing/kprobe: Fix to support kretprobe events on unloaded modules
Hi Jianlin,
Quoted 5 lines · On Fri, 29 Jan 2021 07:45:32 +0000 Jianlin Lv <[email protected]> wrote:
I have verified this patch on server and it solves the kretprobe issue on unloaded modules very well.
Quoted 8 lines
One comments is this patch did not cover the issue I reported before,
https://lore.kernel.org/lkml/[email protected]
om/ if echo wrong function symbol to tracefs, error_log show as :
# echo 'r:myprobe ERROR_SYMBOL_XXX ret=%x0' >> kprobe_events # cat
error_log
[   87.746191] trace_kprobe: error: Invalid probed address or symbol
  Command: r:myprobe ERROR_SYMBOL_XXX ret=%x0

Quoted 5 lines

This is the correct error message because the symbol is not exist, a bad symbol.
Thank you,

Yes, it would be nice if errors could be detected before registering Kprobe.

Of course, this patch is looks good for me.

Jianlin

Quoted 29 lines

Quoted 17 lines
Based on the current patch, The following changes will make the error log more accurate.
(quoted diff) +0 -0
@@ -828,9 +828,10 @@
static int trace_kprobe_create(int argc, const char *argv[]) if (is_return) flags |= TPARG_FL_RETURN;
kernel/trace/trace_kprobe.c +4 -2
-    if (kprobe_on_func_entry(NULL, symbol, offset))
+    if (!kprobe_on_func_entry(NULL, symbol, offset))
        flags |= TPARG_FL_FENTRY;
-    if (offset && is_return && !(flags & TPARG_FL_FENTRY)) {
+    /* Check whether symbol is really bad or from a module */
+    if (!strchr(symbol, ':') && is_return && !(flags &
+ TPARG_FL_FENTRY)) {
        trace_probe_log_err(0, BAD_RETPROBE);
        goto parse_error;
Quoted 6 lines
If you don`t mind, I can send v5 of my patch after this patch be merged.
Jianlin
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Quoted 2 lines

--
Masami Hiramatsu <[email protected]>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.