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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>