| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aNFRRa3m6Qm8zzQu@krava>
Date: Mon, 22 Sep 2025 15:38:13 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Jiri Olsa <olsajiri@...il.com>, Steven Rostedt <rostedt@...dmis.org>,
Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...nel.org>,
Menglong Dong <menglong8.dong@...il.com>, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
x86@...nel.org, hpa@...or.com, kees@...nel.org,
samitolvanen@...gle.com, rppt@...nel.org, luto@...nel.org,
ast@...nel.org, andrii@...nel.org, linux-kernel@...r.kernel.org,
bpf@...r.kernel.org
Subject: Re: [PATCH] tracing: fgraph: Protect return handler from recursion
loop
On Mon, Sep 22, 2025 at 03:16:55PM +0900, Masami Hiramatsu wrote:
> On Sat, 20 Sep 2025 09:45:15 +0200
> Jiri Olsa <olsajiri@...il.com> wrote:
>
> > On Fri, Sep 19, 2025 at 11:27:46AM -0400, Steven Rostedt wrote:
> > > On Fri, 19 Sep 2025 20:57:36 +0900
> > > "Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> > >
> > > > From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > > >
> > > > function_graph_enter_regs() prevents itself from recursion by
> > > > ftrace_test_recursion_trylock(), but __ftrace_return_to_handler(),
> > > > which is called at the exit, does not prevent such recursion.
> > > > Therefore, while it can prevent recursive calls from
> > > > fgraph_ops::entryfunc(), it is not able to prevent recursive calls
> > > > to fgraph from fgraph_ops::retfunc(), resulting in a recursive loop.
> > > > This can lead an unexpected recursion bug reported by Menglong.
> > > >
> > > > is_endbr() is called in __ftrace_return_to_handler -> fprobe_return
> > > > -> kprobe_multi_link_exit_handler -> is_endbr.
> > >
> > > So basically its if the handler for the return part calls something that it
> > > is tracing, it can trigger the recursion?
> > >
> > > >
> > > > To fix this issue, acquire ftrace_test_recursion_trylock() in the
> > > > __ftrace_return_to_handler() after unwind the shadow stack to mark
> > > > this section must prevent recursive call of fgraph inside user-defined
> > > > fgraph_ops::retfunc().
> > > >
> > > > This is essentially a fix to commit 4346ba160409 ("fprobe: Rewrite
> > > > fprobe on function-graph tracer"), because before that fgraph was
> > > > only used from the function graph tracer. Fprobe allowed user to run
> > > > any callbacks from fgraph after that commit.
> > >
> > > I would actually say it's because before this commit, the return handler
> > > callers never called anything that the entry handlers didn't already call.
> > > If there was recursion, the entry handler would catch it (and the entry
> > > tells fgraph if the exit handler should be called).
> > >
> > > The difference here is with fprobes, you can have the exit handler calling
> > > functions that the entry handler does not, which exposes more cases where
> > > recursion could happen.
> >
> > so IIUC we have return kprobe multi probe on is_endbr and now we do:
> >
> > is_endbr()
> > { -> function_graph_enter_regs installs return probe
> > ...
> > } -> __ftrace_return_to_handler
> > fprobe_return
> > kprobe_multi_link_exit_handler
> > is_endbr
> > { -> function_graph_enter_regs installs return probe
> > ...
> > } -> __ftrace_return_to_handler
> > fprobe_return
> > kprobe_multi_link_exit_handler
> > is_endbr
> > { -> function_graph_enter_regs installs return probe
> > ...
> > } -> __ftrace_return_to_handler
> > ... recursion
> >
> >
> > with the fix:
> >
> > is_endbr()
> > { -> function_graph_enter_regs installs return probe
> > ...
> > } -> __ftrace_return_to_handler
> > fprobe_return
> > kprobe_multi_link_exit_handler
> > ...
> > is_endbr
> > { -> function_graph_enter_regs
> > ftrace_test_recursion_trylock fails and we do NOT install return probe
> > ...
> > }
> >
> >
> > there's is_endbr call also in kprobe_multi_link_handler, but it won't
> > trigger recursion, because function_graph_enter_regs already uses
> > ftrace_test_recursion_trylock
> >
> >
> > if above is correct then the fix looks good to me
> >
> > Acked-by: Jiri Olsa <jolsa@...nel.org>
>
> Hi Jiri,
>
> I found ftrace_test_recursion_trylock() allows one nest level, can you
> make sure it is OK?
hum, I recall being surprised by that already in the past,
I thought we fixed that somehow, will check later today
thanks,
jirka
>
> Thank you,
>
> >
> > thanks,
> > jirka
> >
> >
> > >
> > > >
> > > > Reported-by: Menglong Dong <menglong8.dong@...il.com>
> > > > Closes: https://lore.kernel.org/all/20250918120939.1706585-1-dongml2@chinatelecom.cn/
> > > > Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
> > > > Cc: stable@...r.kernel.org
> > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > > > ---
> > > > kernel/trace/fgraph.c | 12 ++++++++++++
> > > > 1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > > > index 1e3b32b1e82c..08dde420635b 100644
> > > > --- a/kernel/trace/fgraph.c
> > > > +++ b/kernel/trace/fgraph.c
> > > > @@ -815,6 +815,7 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> > > > unsigned long bitmap;
> > > > unsigned long ret;
> > > > int offset;
> > > > + int bit;
> > > > int i;
> > > >
> > > > ret_stack = ftrace_pop_return_trace(&trace, &ret, frame_pointer, &offset);
> > > > @@ -829,6 +830,15 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> > > > if (fregs)
> > > > ftrace_regs_set_instruction_pointer(fregs, ret);
> > > >
> > > > + bit = ftrace_test_recursion_trylock(trace.func, ret);
> > > > + /*
> > > > + * This must be succeeded because the entry handler returns before
> > > > + * modifying the return address if it is nested. Anyway, we need to
> > > > + * avoid calling user callbacks if it is nested.
> > > > + */
> > > > + if (WARN_ON_ONCE(bit < 0))
> > >
> > > I'm not so sure we need the warn on here. We should probably hook it to the
> > > recursion detection infrastructure that the function tracer has.
> > >
> > > The reason I would say not to have the warn on, is because we don't have a
> > > warn on for recursion happening at the entry handler. Because this now is
> > > exposed by fprobe allowing different routines to be called at exit than
> > > what is used in entry, it can easily be triggered.
> > >
> > > -- Steve
> > >
> > >
> > >
> > > > + goto out;
> > > > +
> > > > #ifdef CONFIG_FUNCTION_GRAPH_RETVAL
> > > > trace.retval = ftrace_regs_get_return_value(fregs);
> > > > #endif
> > > > @@ -852,6 +862,8 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs, unsigned long frame_pointe
> > > > }
> > > > }
> > > >
> > > > + ftrace_test_recursion_unlock(bit);
> > > > +out:
> > > > /*
> > > > * The ftrace_graph_return() may still access the current
> > > > * ret_stack structure, we need to make sure the update of
> > >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists