| Message ID | 20240407035904.2556645-1-zhengyejian1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | kprobes: Fix possible warn in __arm_kprobe_ftrace() | expand |
On Sun, 7 Apr 2024 11:59:04 +0800 Zheng Yejian <zhengyejian1@huawei.com> 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. > 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? Also some comments needs to be updated. > > Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> > --- > 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.) > + } > /* 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. > - 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,
On 2024/4/8 10:50, Masami Hiramatsu (Google) wrote: > On Sun, 7 Apr 2024 11:59:04 +0800 > Zheng Yejian <zhengyejian1@huawei.com> 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@huawei.com> >> --- >> 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
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(); + *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; + } + } /* Ensure it is not in reserved area nor out of text */ - if (!(core_kernel_text((unsigned long) p->addr) || - is_module_text_address((unsigned long) p->addr)) || - 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. */ - *probed_mod = __module_text_address((unsigned long) p->addr); if (*probed_mod) { /* * We must hold a refcount of the probed module while updating
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(): 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. Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> --- kernel/kprobes.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)