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(-)
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index b3a36b0cfc81..1883a4a9f16a 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -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);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f7fb5d135930..1a5bc321e0a5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -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);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e6fba1798771..56c7fbff7bd7 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -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;
}
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).
>
> 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
On Wed, 27 Jan 2021 17:29:50 -0500
Steven Rostedt <[email protected]> wrote:
> 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).
> >
> > 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,
--
Masami Hiramatsu <[email protected]>
> -----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,
>
> 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.
> >
> > 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
>
> 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
> >
> > Based on the current patch, The following changes will make the error
> > log more accurate.
> >
> > @@ -828,9 +828,10 @@
> > 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))
> > + 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;
> >
> > 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.
>
>
> --
> 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.