Re: [PATCH] kprobes: Call check_ftrace_location() on CONFIG_KPROBES_ON_FTRACE

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri,  5 Dec 2025 17:29:33 +0800
"qingwei.hu" <qingwei.hu@bytedance.com> wrote:

> From: Qingwei Hu <qingwei.hu@bytedance.com>
> 
> There is a possible configuration dependency:
> 
>   KPROBES_ON_FTRACE [=n]
>        ^----- KPROBES [=y]
>          |--- HAVE_KPROBES_ON_FTRACE [=n]
>          |--- DYNAMIC_FTRACE_WITH_REGS [=n]
>                 ^----- FTRACE [=y]
>                   |--- DYNAMIC_FTRACE [=y]
>                   |--- HAVE_DYNAMIC_FTRACE_WITH_REGS [=n]
> 
> With DYNAMIC_FTRACE=y, ftrace_location() is meaningful and may
> return the same address as the probe target.
> 
> However, when KPROBES_ON_FTRACE=n, the current implementation
> returns -EINVAL after calling check_ftrace_location(), causing
> the validation to fail.
> 
> Adjust the logic so that ftrace-based checks are performed only
> when CONFIG_KPROBES_ON_FTRACE is enabled, ensuring correct
> kprobe behavior even when KPROBES_ON_FTRACE=n.

It is a bit complicated but CONFIG_KPROBES_ON_FTRACE is a hidden
static option set by architecture. If it is not enabled, that
feature is not usable on the architecture. And as Steve mentioned
we can not put a software breakpoint code on where the ftrace will
modify. IOW, ftrace reserves the instruction address.
Thus, we will check whether it is reserved by ftrace, and if so,
it returns -EINVAL, or if architecture already implements the
kprobes on ftrace feature (it requires to implement a ftrace handler
which mimics kprobe), it uses that.

So I can not accept this.

Thank you,

> 
> Signed-off-by: Qingwei Hu <qingwei.hu@bytedance.com>
> ---
>  kernel/kprobes.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ab8f9fc1f0d1..f4aa4ba1ca9c 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1512,19 +1512,15 @@ static inline int warn_kprobe_rereg(struct kprobe *p)
>  	return 0;
>  }
>  
> -static int check_ftrace_location(struct kprobe *p)
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +static void check_ftrace_location(struct kprobe *p)
>  {
>  	unsigned long addr = (unsigned long)p->addr;
>  
> -	if (ftrace_location(addr) == addr) {
> -#ifdef CONFIG_KPROBES_ON_FTRACE
> +	if (ftrace_location(addr) == addr)
>  		p->flags |= KPROBE_FLAG_FTRACE;
> -#else
> -		return -EINVAL;
> -#endif
> -	}
> -	return 0;
>  }
> +#endif
>  
>  static bool is_cfi_preamble_symbol(unsigned long addr)
>  {
> @@ -1540,11 +1536,9 @@ static bool is_cfi_preamble_symbol(unsigned long addr)
>  static int check_kprobe_address_safe(struct kprobe *p,
>  				     struct module **probed_mod)
>  {
> -	int ret;
> -
> -	ret = check_ftrace_location(p);
> -	if (ret)
> -		return ret;
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +	check_ftrace_location(p);
> +#endif
>  
>  	guard(jump_label_lock)();
>  
> -- 
> 2.39.5
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>