On Mon, 10 Jul 2023 18:04:22 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Fri, 7 Jul 2023 23:03:19 +0900
> "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> >
> > Ensure running fprobe_exit_handler() has finished before
> > calling rethook_free() in the unregister_fprobe() so that caller can free
> > the fprobe right after unregister_fprobe().
> >
> > unregister_fprobe() ensured that all running fprobe_entry/exit_handler()
> > have finished by calling unregister_ftrace_function() which synchronizes
> > RCU. But commit 5f81018753df ("fprobe: Release rethook after the ftrace_ops
> > is unregistered") changed to call rethook_free() after
> > unregister_ftrace_function(). So call rethook_stop() to make rethook
> > disabled before unregister_ftrace_function() and ensure it again.
> >
> > Here is the possible code flow that can call the exit handler after
> > unregister_fprobe().
> >
> > ------
> > CPU1 CPU2
> > call unregister_fprobe(fp)
> > ...
> > __fprobe_handler()
> > rethook_hook() on probed function
> > unregister_ftrace_function()
> > return from probed function
> > rethook hooks
> > find rh->handler == fprobe_exit_handler
> > call fprobe_exit_handler()
> > rethook_free():
> > set rh->handler = NULL;
> > return from unreigster_fprobe;
> > call fp->exit_handler() <- (*)
> > ------
> >
> > (*) At this point, the exit handler is called after returning from
> > unregister_fprobe().
> >
> > This fixes it as following;
> > ------
> > CPU1 CPU2
> > call unregister_fprobe()
> > ...
> > rethook_stop():
> > set rh->handler = NULL;
> > __fprobe_handler()
> > rethook_hook() on probed function
> > unregister_ftrace_function()
> > return from probed function
> > rethook hooks
> > find rh->handler == NULL
> > return from rethook
> > rethook_free()
> > return from unreigster_fprobe;
> > ------
> >
> >
> > Fixes: 5f81018753df ("fprobe: Release rethook after the ftrace_ops is unregistered")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
>
> Looks good.
>
> Reviewed-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
Thank you :)
>
>
> > ---
> > Changes in v2:
> > - Update changelog to add a problematic code flow.
>
> Nit, for making forensic analysis easier in the future, I now always add a
> link to the previous version. That is:
>
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/168796344232.46347.7947681068822514750.stgit@devnote2/
> - Update changelog to add a problematic code flow.
OK, I'll add it for an isolated patch too.
Thanks!
>
> -- Steve
>
>
> > ---
> > include/linux/rethook.h | 1 +
> > kernel/trace/fprobe.c | 3 +++
> > kernel/trace/rethook.c | 13 +++++++++++++
> > 3 files changed, 17 insertions(+)
> >
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
