From: "Steven Rostedt (Google)" <[email protected]>
Song Shuai reported:
The list func (ftrace_ops_list_func) will be patched first
before the transition between old and new calls are set,
which fixed the race described in this commit `59338f75`.
While ftrace_trace_function changes from the list func to a
ftrace_ops func, like unregistering the klp_ops to leave the only
global_ops in ftrace_ops_list, the ftrace_[regs]_call will be
replaced with the list func although it already exists. So there
should be a condition to avoid this.
And suggested using another variable to keep track of what the ftrace
function is set to. But this could be simplified by using a helper
function that does the same with a static variable.
Link: https://lore.kernel.org/lkml/[email protected]/
Reported-by: Song Shuai <[email protected]>
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
kernel/trace/ftrace.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 65a5d36463e0..d04552c0c275 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2763,6 +2763,19 @@ void __weak ftrace_arch_code_modify_post_process(void)
{
}
+static int update_ftrace_func(ftrace_func_t func)
+{
+ static ftrace_func_t save_func;
+
+ /* Avoid updating if it hasn't changed */
+ if (func == save_func)
+ return 0;
+
+ save_func = func;
+
+ return ftrace_update_ftrace_func(func);
+}
+
void ftrace_modify_all_code(int command)
{
int update = command & FTRACE_UPDATE_TRACE_FUNC;
@@ -2783,7 +2796,7 @@ void ftrace_modify_all_code(int command)
* traced.
*/
if (update) {
- err = ftrace_update_ftrace_func(ftrace_ops_list_func);
+ err = update_ftrace_func(ftrace_ops_list_func);
if (FTRACE_WARN_ON(err))
return;
}
@@ -2799,7 +2812,7 @@ void ftrace_modify_all_code(int command)
/* If irqs are disabled, we are in stop machine */
if (!irqs_disabled())
smp_call_function(ftrace_sync_ipi, NULL, 1);
- err = ftrace_update_ftrace_func(ftrace_trace_function);
+ err = update_ftrace_func(ftrace_trace_function);
if (FTRACE_WARN_ON(err))
return;
}
--
2.35.1
On Wed, 23 Nov 2022 10:29:28 +0800
Song Shuai <[email protected]> wrote:
> > @@ -2783,7 +2796,7 @@ void ftrace_modify_all_code(int command)
> > * traced.
> > */
> > if (update) {
> > - err = ftrace_update_ftrace_func(ftrace_ops_list_func);
> > + err = update_ftrace_func(ftrace_ops_list_func);
> > if (FTRACE_WARN_ON(err))
> > return;
> > }
> > @@ -2799,7 +2812,7 @@ void ftrace_modify_all_code(int command)
> > /* If irqs are disabled, we are in stop machine */
> > if (!irqs_disabled())
> > smp_call_function(ftrace_sync_ipi, NULL, 1);
> > - err = ftrace_update_ftrace_func(ftrace_trace_function);
> > + err = update_ftrace_func(ftrace_trace_function);
> The helper function should only serve the ftrace_ops_list_func.
>
> And ftrace_trace_function is always different in this function with
> FTRACE_UPDATE_TRACE_FUNC command.
>
> So this place should be left as it is.
But it is needed to update the static variable. Without this change,
then save_func will get set to ftrace_ops_list_func and never change
after that and then the update to ftrace_ops_list_func will stop happening.
-- Steve
Steven Rostedt <[email protected]> 于2022年11月23日周三 03:17写道：
>
> On Wed, 23 Nov 2022 10:29:28 +0800
> Song Shuai <[email protected]> wrote:
>
> > > @@ -2783,7 +2796,7 @@ void ftrace_modify_all_code(int command)
> > > * traced.
> > > */
> > > if (update) {
> > > - err = ftrace_update_ftrace_func(ftrace_ops_list_func);
> > > + err = update_ftrace_func(ftrace_ops_list_func);
> > > if (FTRACE_WARN_ON(err))
> > > return;
> > > }
> > > @@ -2799,7 +2812,7 @@ void ftrace_modify_all_code(int command)
> > > /* If irqs are disabled, we are in stop machine */
> > > if (!irqs_disabled())
> > > smp_call_function(ftrace_sync_ipi, NULL, 1);
> > > - err = ftrace_update_ftrace_func(ftrace_trace_function);
> > > + err = update_ftrace_func(ftrace_trace_function);
> > The helper function should only serve the ftrace_ops_list_func.
> >
> > And ftrace_trace_function is always different in this function with
> > FTRACE_UPDATE_TRACE_FUNC command.
> >
> > So this place should be left as it is.
>
> But it is needed to update the static variable. Without this change,
> then save_func will get set to ftrace_ops_list_func and never change
> after that and then the update to ftrace_ops_list_func will stop happening.
>
> -- Steve
Yes, you're right. Thx for your explanation.
-- Song
On Tue, 22 Nov 2022 18:09:05 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> Song Shuai reported:
>
> The list func (ftrace_ops_list_func) will be patched first
> before the transition between old and new calls are set,
> which fixed the race described in this commit `59338f75`.
>
> While ftrace_trace_function changes from the list func to a
> ftrace_ops func, like unregistering the klp_ops to leave the only
> global_ops in ftrace_ops_list, the ftrace_[regs]_call will be
> replaced with the list func although it already exists. So there
> should be a condition to avoid this.
>
> And suggested using another variable to keep track of what the ftrace
> function is set to. But this could be simplified by using a helper
> function that does the same with a static variable.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
>
This looks good to me.
Reviewed-by: Masami Hiramatsu (Google) <[email protected]>
Thank you,
> Reported-by: Song Shuai <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
> kernel/trace/ftrace.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 65a5d36463e0..d04552c0c275 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2763,6 +2763,19 @@ void __weak ftrace_arch_code_modify_post_process(void)
> {
> }
>
> +static int update_ftrace_func(ftrace_func_t func)
> +{
> + static ftrace_func_t save_func;
> +
> + /* Avoid updating if it hasn't changed */
> + if (func == save_func)
> + return 0;
> +
> + save_func = func;
> +
> + return ftrace_update_ftrace_func(func);
> +}
> +
> void ftrace_modify_all_code(int command)
> {
> int update = command & FTRACE_UPDATE_TRACE_FUNC;
> @@ -2783,7 +2796,7 @@ void ftrace_modify_all_code(int command)
> * traced.
> */
> if (update) {
> - err = ftrace_update_ftrace_func(ftrace_ops_list_func);
> + err = update_ftrace_func(ftrace_ops_list_func);
> if (FTRACE_WARN_ON(err))
> return;
> }
> @@ -2799,7 +2812,7 @@ void ftrace_modify_all_code(int command)
> /* If irqs are disabled, we are in stop machine */
> if (!irqs_disabled())
> smp_call_function(ftrace_sync_ipi, NULL, 1);
> - err = ftrace_update_ftrace_func(ftrace_trace_function);
> + err = update_ftrace_func(ftrace_trace_function);
> if (FTRACE_WARN_ON(err))
> return;
> }
> --
> 2.35.1
>
--
Masami Hiramatsu (Google) <[email protected]>