Message-ID: <6196970.lOV4Wx5bFT@7940hx>
Date: Mon, 22 Sep 2025 15:13:38 +0800
From: menglong.dong@...ux.dev
To: Peter Zijlstra <peterz@...radead.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>,
 Jiri Olsa <jolsa@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>, X86 ML <x86@...nel.org>,
 "H. Peter Anvin" <hpa@...or.com>, Kees Cook <kees@...nel.org>,
 Sami Tolvanen <samitolvanen@...gle.com>, Mike Rapoport <rppt@...nel.org>,
 Andy Lutomirski <luto@...nel.org>, Masami Hiramatsu <mhiramat@...nel.org>,
 Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
 LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH] x86/ibt: make is_endbr() notrace

On 2025/9/22 14:52 Peter Zijlstra <peterz@...radead.org> write:
> On Fri, Sep 19, 2025 at 09:13:15AM +0800, Menglong Dong wrote:
> 
> > Ok, let me describe the problem in deetail.
> > 
> > First of all, it has nothing to do with kprobe. The bpf program of type
> > kprobe-multi based on fprobe, and fprobe base on fgraph. So it's all
> > about the ftrace, which means __fentry__.
> 
> Well, that's not confusing at all. Something called kprobe-multi not
> being related to kprobes :-(
> 
> > Second, let me explain the recur detection of the kprobe-multi. Let's
> > take the is_endbr() for example. When it is hooked by the bpf program
> > of type kretprobe-multi, following calling chain will happen:
> > 
> >   is_endbr -> __ftrace_return_to_handler -> fprobe_return ->
> >   kprobe_multi_link_exit_handler -> ftrace_get_entry_ip ->
> >   arch_ftrace_get_symaddr -> is_endbr
> > 
> > Look, is_endbr() is called again during the ftrace handler, so it will
> > trigger the ftrace handler(__ftrace_return_to_handler) again, which
> > causes recurrence.
> 
> Right.
> 
> > Such recurrence can be detected. In kprobe_multi_link_prog_run(),
> > the percpu various "bpf_prog_active" will be increased by 1 before we
> > run the bpf progs, and decrease by 1 after the bpf progs finish. If the
> > kprobe_multi_link_prog_run() is triggered again during bpf progs run,
> > it will check if bpf_prog_active is zero, and return directly if it is not.
> > Therefore, recurrence can't happen within the "bpf_prog_active" protection.
> 
> As I think Masami already said, the problem is the layer. You're trying
> to fix an ftrace problem at the bpf layer.

Yeah, I see. And Masami has already posted a series for this
problem in:

https://lore.kernel.org/bpf/175852291163.307379.14414635977719513326.stgit@devnote2/

> 
> > However, the calling to is_endbr() is not within that scope, which makes
> > the recurrence happen.
> 
> Sorta, I'm still sketchy on the whole kprobe-multi thing.
> 
> Anyway, I don't mind making is_endbr() invisible to tracing, that might
> just have security benefits too. But I think first the ftrace folks need
> to figure out how to best kill that recursion, because I don't think
> is_endbr is particularly special here.

So, does this patch seem useful after all?

OK, I'll send a V2 base on your following suggestion.

Thanks!
Menglong Dong

> 
> It is just one more function that can emit a __fentry__ site.
> 
> Anyway, something like the below would do:
> 
> Note that without making __is_endbr() __always_inline, you run the risk
> of the compiler being retarded (they often are in the face of
> KASAN/UBSAN like) and deciding to out-of-line that function, resulting
> in yet another __fentry__ site.
> 
> An added advantage of noinstr is that it is validated by objtool to
> never call to !noinstr code. As such, you can be sure there is no
> instrumentation in it.
> 
> (the below hasn't been near a compiler)
> 
> ---
>  arch/x86/include/asm/ibt.h    | 2 +-
>  arch/x86/kernel/alternative.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> index 5e45d6424722..54937a527042 100644
> --- a/arch/x86/include/asm/ibt.h
> +++ b/arch/x86/include/asm/ibt.h
> @@ -65,7 +65,7 @@ static __always_inline __attribute_const__ u32 gen_endbr_poison(void)
>  	return 0xd6401f0f; /* nopl -42(%rax) */
>  }
>  
> -static inline bool __is_endbr(u32 val)
> +static __always_inline bool __is_endbr(u32 val)
>  {
>  	if (val == gen_endbr_poison())
>  		return true;
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 69fb818df2ee..f791e7abd466 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1108,7 +1108,7 @@ void __init_or_module noinline apply_returns(s32 *start, s32 *end) { }
>  
>  #ifdef CONFIG_X86_KERNEL_IBT
>  
> -__noendbr bool is_endbr(u32 *val)
> +__noendbr noinstr bool is_endbr(u32 *val)
>  {
>  	u32 endbr;
>  
> 
> 

Powered by blists - more mailing lists