On Wed, 28 Jun 2023 23:44:02 +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.
I'm confused. I still don't understand why it is bad to call
unregister_ftrace_function() *before* rethook_free().
Can you show the race condition you are trying to avoid?
-- Steve
>
> Fixes: 5f81018753df ("fprobe: Release rethook after the ftrace_ops is
> unregistered") Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> ---
> include/linux/rethook.h | 1 +
> kernel/trace/fprobe.c | 3 +++
> kernel/trace/rethook.c | 13 +++++++++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> index c8ac1e5afcd1..bdbe6717f45a 100644
> --- a/include/linux/rethook.h
> +++ b/include/linux/rethook.h
> @@ -59,6 +59,7 @@ struct rethook_node {
> };
>
> struct rethook *rethook_alloc(void *data, rethook_handler_t handler);
> +void rethook_stop(struct rethook *rh);
> void rethook_free(struct rethook *rh);
> void rethook_add_node(struct rethook *rh, struct rethook_node *node);
> struct rethook_node *rethook_try_get(struct rethook *rh);
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 0121e8c0d54e..75517667b54f 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -364,6 +364,9 @@ int unregister_fprobe(struct fprobe *fp)
> fp->ops.saved_func != fprobe_kprobe_handler))
> return -EINVAL;
>
> + if (fp->rethook)
> + rethook_stop(fp->rethook);
> +
> ret = unregister_ftrace_function(&fp->ops);
> if (ret < 0)
> return ret;
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index 60f6cb2b486b..468006cce7ca 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -53,6 +53,19 @@ static void rethook_free_rcu(struct rcu_head *head)
> kfree(rh);
> }
>
> +/**
> + * rethook_stop() - Stop using a rethook.
> + * @rh: the struct rethook to stop.
> + *
> + * Stop using a rethook to prepare for freeing it. If you want to wait
> for
> + * all running rethook handler before calling rethook_free(), you need to
> + * call this first and wait RCU, and call rethook_free().
> + */
> +void rethook_stop(struct rethook *rh)
> +{
> + WRITE_ONCE(rh->handler, NULL);
> +}
> +
> /**
> * rethook_free() - Free struct rethook.
> * @rh: the struct rethook to be freed.
