In kprobe_ftrace_handler, it accesses get kporbe without kprobe_mutex
held.
This makes some of synchronizing issue when we use kprobe API in
kernel-module.
Below is what i experienced:
CPU 0 CPU 1
<...> <In module code>
kprobe_ftrace_handler
get_kprobe
__this_cpu_write
unregister_kprobe
unload_module
< kprobe memory gone>
p->pre_handler <access invalid memory>
page_fault
kprobe_fault_handler
(In here, kprobe memory gone,
double page fault is happening inifinie).
Signed-off-by: Levi Yun <[email protected]>
---
arch/x86/kernel/kprobes/ftrace.c | 3 +++
include/linux/kprobes.h | 2 ++
kernel/kprobes.c | 2 +-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index dd2ec14adb77..76147ff6ed88 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -25,6 +25,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
if (bit < 0)
return;
+ mutex_lock(&kprobe_mutex);
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -57,7 +58,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
*/
__this_cpu_write(current_kprobe, NULL);
}
+
out:
+ mutex_unlock(&kprobe_mutex);
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 157168769fc2..4a18147ff6d6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -191,6 +191,8 @@ struct kprobe_blacklist_entry {
DECLARE_PER_CPU(struct kprobe *, current_kprobe);
DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+extern struct mutex kprobe_mutex;
+
extern void kprobe_busy_begin(void);
extern void kprobe_busy_end(void);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index dd58c0be9ce2..b65f055b6fa2 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -64,7 +64,7 @@ static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static bool kprobes_all_disarmed;
/* This protects 'kprobe_table' and 'optimizing_list' */
-static DEFINE_MUTEX(kprobe_mutex);
+DEFINE_MUTEX(kprobe_mutex);
static DEFINE_PER_CPU(struct kprobe *, kprobe_instance);
kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
--
2.35.1
On Mon, 2 May 2022 13:51:02 +0900
Levi Yun <[email protected]> wrote:
> In kprobe_ftrace_handler, it accesses get kporbe without kprobe_mutex
> held.
>
> This makes some of synchronizing issue when we use kprobe API in
> kernel-module.
NAK this, because get_kprobes() doesn't require the kprobe_mutex in
the preempt-disabled context. Please read the comment of get_kprobe().
/*
* This routine is called either:
* - under the 'kprobe_mutex' - during kprobe_[un]register().
* OR
* - with preemption disabled - from architecture specific code.
*/
struct kprobe *get_kprobe(void *addr)
Moreover, we can not use mutex inside kprobe handler because it runs
in the interrupt context.
Thank you,
>
> Below is what i experienced:
>
> CPU 0 CPU 1
> <...> <In module code>
> kprobe_ftrace_handler
> get_kprobe
> __this_cpu_write
> unregister_kprobe
> unload_module
> < kprobe memory gone>
> p->pre_handler <access invalid memory>
> page_fault
> kprobe_fault_handler
> (In here, kprobe memory gone,
> double page fault is happening inifinie).
>
> Signed-off-by: Levi Yun <[email protected]>
> ---
> arch/x86/kernel/kprobes/ftrace.c | 3 +++
> include/linux/kprobes.h | 2 ++
> kernel/kprobes.c | 2 +-
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
> index dd2ec14adb77..76147ff6ed88 100644
> --- a/arch/x86/kernel/kprobes/ftrace.c
> +++ b/arch/x86/kernel/kprobes/ftrace.c
> @@ -25,6 +25,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> if (bit < 0)
> return;
>
> + mutex_lock(&kprobe_mutex);
> p = get_kprobe((kprobe_opcode_t *)ip);
> if (unlikely(!p) || kprobe_disabled(p))
> goto out;
> @@ -57,7 +58,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> */
> __this_cpu_write(current_kprobe, NULL);
> }
> +
> out:
> + mutex_unlock(&kprobe_mutex);
> ftrace_test_recursion_unlock(bit);
> }
> NOKPROBE_SYMBOL(kprobe_ftrace_handler);
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 157168769fc2..4a18147ff6d6 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -191,6 +191,8 @@ struct kprobe_blacklist_entry {
> DECLARE_PER_CPU(struct kprobe *, current_kprobe);
> DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>
> +extern struct mutex kprobe_mutex;
> +
> extern void kprobe_busy_begin(void);
> extern void kprobe_busy_end(void);
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index dd58c0be9ce2..b65f055b6fa2 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -64,7 +64,7 @@ static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> static bool kprobes_all_disarmed;
>
> /* This protects 'kprobe_table' and 'optimizing_list' */
> -static DEFINE_MUTEX(kprobe_mutex);
> +DEFINE_MUTEX(kprobe_mutex);
> static DEFINE_PER_CPU(struct kprobe *, kprobe_instance);
>
> kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
> --
> 2.35.1
>
--
Masami Hiramatsu <[email protected]>