| Message ID | 168796344232.46347.7947681068822514750.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Steven Rostedt |
| Headers | show |
| Series | fprobe: Ensure running fprobe_exit_handler() finished before calling rethook_free() | expand |
On Wed, 28 Jun 2023 23:44:02 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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. > Steve, can you review this? without this fix, Jiri's patch may cause another timing issue. Thanks, > Fixes: 5f81018753df ("fprobe: Release rethook after the ftrace_ops is unregistered") > Cc: stable@vger.kernel.org > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > 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. >
On Wed, 28 Jun 2023 23:44:02 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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@vger.kernel.org > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > 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.
On Wed, 5 Jul 2023 21:26:57 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 28 Jun 2023 23:44:02 +0900 > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > 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? Yes. This is ensuring all handlers exit when returning from unregister_fprobe() so that the caller can release the data which will be accessed from the handlers. The entry handler is safe because unregister_ftrace_function() waits for the ftrace handlers. But that is not enough for the exit handler. With only Jiri's patch, following flow can happen; ------ CPU1 CPU2 call unregister_fprobe() ... __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() <- (*) (*) In this point, the exit handler is called after returning from unregister_fprobe(). ------ So, this patch changes 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; ------ I can also just put a synchronize_sched_rcu() right after rethook_free() to wait for all running fprobe_exit_handler() too. Thank you, > > -- Steve > > > > > > > Fixes: 5f81018753df ("fprobe: Release rethook after the ftrace_ops is > > unregistered") Cc: stable@vger.kernel.org > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > --- > > 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. >
On Thu, 6 Jul 2023 14:10:12 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > With only Jiri's patch, following flow can happen; > > ------ > CPU1 CPU2 > call unregister_fprobe() > ... > __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() <- (*) > > (*) In this point, the exit handler is called after returning from > unregister_fprobe(). > ------ > > So, this patch changes 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; > ------ > > I can also just put a synchronize_sched_rcu() right after rethook_free() > to wait for all running fprobe_exit_handler() too. > This makes more sense. Can you please add the above to the change log. Thanks, -- Steve
On Thu, 6 Jul 2023 09:56:24 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 6 Jul 2023 14:10:12 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > With only Jiri's patch, following flow can happen; > > > > ------ > > CPU1 CPU2 > > call unregister_fprobe() > > ... > > __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() <- (*) > > > > (*) In this point, the exit handler is called after returning from > > unregister_fprobe(). > > ------ > > > > So, this patch changes 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; > > ------ > > > > I can also just put a synchronize_sched_rcu() right after rethook_free() > > to wait for all running fprobe_exit_handler() too. > > > > This makes more sense. Can you please add the above to the change log. OK, let me update it. Thanks! > > Thanks, > > -- Steve
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.