Re: [PATCH] kprobes: Fix possible warn in __arm_kprobe_ftrace()
From: Zheng Yejian
Date: Sun Apr 07 2024 - 23:29:14 EST
On 2024/4/8 10:50, Masami Hiramatsu (Google) wrote:
On Sun, 7 Apr 2024 11:59:04 +0800
Zheng Yejian <zhengyejian1@xxxxxxxxxx> wrote:
There is once warn in __arm_kprobe_ftrace() on:
ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 0, 0);
if (WARN_ONCE(..., "Failed to arm kprobe-ftrace at %pS (error %d)\n", ...)
return ret;
This warning is due to 'p->addr' is not a valid ftrace_location and
that invalid 'p->addr' was bypassed in check_kprobe_address_safe():
Ah, this is a guard warning to detect changes on ftrace_set_filter() and/or
preparation steps. (A kind of debug message.)
The ftrace address check is done by check_ftrace_location() at the beginning of
check_kprobe_address_safe(). At that point, ftrace_location(addr) == addr should
return true if the module is loaded. But there is a small window that we check
the ftrace_location() and get the module refcount, even if the "addr" was ftrace
location, the module is unloaded and failed to get the module refcount and fail
to register the kprobe.
Thanks, I'll complete the description in V2.
check_kprobe_address_safe() {
...
// 1. p->addr is in some module, this check passed
is_module_text_address((unsigned long) p->addr)
...
// 2. If the moudle is removed, the *probed_mod is NULL, but then
// the return value would still be 0 !!!
*probed_mod = __module_text_address((unsigned long) p->addr);
...
}
So adjust the module text check to fix it.
This seems just optimizing code (skip the 2nd module search), right?
Yes, optimze more than fix, but this can still avoid latter warn like in
__arm_kprobe_ftrace()
Also some comments needs to be updated.
Will do in V2.
Signed-off-by: Zheng Yejian <zhengyejian1@xxxxxxxxxx>
---
kernel/kprobes.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9d9095e81792..e0612cc3e2a3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1567,10 +1567,16 @@ static int check_kprobe_address_safe(struct kprobe *p,
jump_label_lock();
preempt_disable();
/* Ensure the address is in a text area, and find a module if exists. */
+ *probed_mod = NULL;
+ if (!core_kernel_text((unsigned long) p->addr)) {
+ *probed_mod = __module_text_address((unsigned long) p->addr);
+ if (!(*probed_mod)) {
+ ret = -EINVAL;
+ goto out;
+ }
nit: this should get the module refcount soon after getting probed_mod.
(I think this should be an atomic operation, but we don't have such interface.)
Yes, it's better to get the module refcount right here, but suppose the
module is expected to be unloaded soon, we don't really need to make the
registration succeed.
+ }
/* Ensure it is not in reserved area nor out of text */
/* Ensure it is not in reserved area. */
- if (!(core_kernel_text((unsigned long) p->addr) ||
- is_module_text_address((unsigned long) p->addr)) ||
Note that this part is doing same thing. If the probe address is !kernel_text
and !module_text, it will be rejected.
Yes.
- in_gate_area_no_mm((unsigned long) p->addr) ||
+ if (in_gate_area_no_mm((unsigned long) p->addr) ||
within_kprobe_blacklist((unsigned long) p->addr) ||
jump_label_text_reserved(p->addr, p->addr) ||
static_call_text_reserved(p->addr, p->addr) ||
@@ -1581,7 +1587,6 @@ static int check_kprobe_address_safe(struct kprobe *p,
}
/* Check if 'p' is probing a module. */
/* Get module refcount and reject __init functions for loaded modules. */
- *probed_mod = __module_text_address((unsigned long) p->addr);
if (*probed_mod) {
/*
* We must hold a refcount of the probed module while updating
--
2.25.1
Thank you,
--
Thank you,
Zheng Yejian