[PATCH] kprobe: sync issue's on ftraced-kprobe.

[PATCH] kprobe: sync issue's on ftraced-kprobe.

[Levi Yun]

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 <ppbuk5246@gmail.com>
---
 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

Re: [PATCH] kprobe: sync issue's on ftraced-kprobe.

[Masami Hiramatsu]

On Mon,  2 May 2022 13:51:02 +0900
Levi Yun <ppbuk5246@gmail.com> 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 <ppbuk5246@gmail.com>
> ---
>  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 <mhiramat@kernel.org>

Re: [PATCH] kprobe: sync issue's on ftraced-kprobe.

[Yun Levi]

> 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.

Actually, I think it's in the preemption disabled in situation.
What I mentioned is actually it seems inside the kprobe handler which
installed via ftrace isn't interrupt context.

#ifdef CONFIG_KPROBES_ON_FTRACE
static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 >---.func = kprobe_ftrace_handler,
 >---.flags = FTRACE_OPS_FL_SAVE_REGS,
 };

In case of optimized kprobe it disables preemption when it enter
"optimized callback"
And the trampoline isn't the interrupted context.
It's also the same to install kprobe via ftraced .
When I see the ftraced patched code, I couldn't find any interrupt and
preemption disable code.
But it seems to allow preemption in ftarce handler.
(In the ftrace_reg_caller, ftace_reg_end, and other ftrace trampoline code also)

What I understand is break point based kprobe is only interrupted
disabled status
But optimized kprobe and ftarced kprobe isn't interrupted disabled status.
For this, optimized kprobe disable disable preemption in "optimized_callback"

But ftrace kprobe isn't preemption disabled nor interrupt-disabled context.

That's why I sent patches and I experienced the problem above.

Am I missing something?

Thx.