On Wed, 16 Oct 2019 13:33:13 +0200 Miroslav Benes <[email protected]> wrote:
> Livepatch uses ftrace for redirection to new patched functions. It means > that if ftrace is disabled, all live patched functions are disabled as > well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. > It is not a problem per se, because only administrator can set sysctl > values, but it still may be surprising. > > Introduce PERMANENT ftrace_ops flag to amend this. If the > FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be > disabled by disabling ftrace_enabled. Equally, a callback with the flag > set cannot be registered if ftrace_enabled is disabled. > > Signed-off-by: Miroslav Benes <[email protected]> > Reviewed-by: Petr Mladek <[email protected]> > Reviewed-by: Kamalesh Babulal <[email protected]> > --- > I pulled in this patch as is, but then I realized we have a race. This race has been there before this patch series, and actually, been there since the dawn of ftrace. I realized that the user can modify ftrace_enabled out of lock context. That is, the ftrace_enabled is modified directly from the sysctl code, without taking the ftrace_lock mutex. Which means if the user was starting and stopping function tracing while playing with the ftrace_enabled switch, it could potentially cause an accounting failure. I'm going to apply this patch on top of yours. It reverses the role of how ftrace_enabled is set in the sysctl handler. Instead of having it directly modify ftrace_enabled, I have it modify a new variable called sysctl_ftrace_enabled. I no longer need the last_ftrace_enabled. This way we only need to set or disable ftrace_enabled on a change and if all conditions are met. Thoughts? -- Steve diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 8385cafe4f9f..aa2e2c7cef9e 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -79,6 +79,7 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val #ifdef CONFIG_FUNCTION_TRACER extern int ftrace_enabled; +extern int sysctl_ftrace_enabled; extern int ftrace_enable_sysctl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, @@ -638,6 +639,7 @@ static inline void tracer_disable(void) { #ifdef CONFIG_FUNCTION_TRACER ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; #endif } @@ -651,6 +653,7 @@ static inline int __ftrace_enabled_save(void) #ifdef CONFIG_FUNCTION_TRACER int saved_ftrace_enabled = ftrace_enabled; ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; return saved_ftrace_enabled; #else return 0; @@ -661,6 +664,7 @@ static inline void __ftrace_enabled_restore(int enabled) { #ifdef CONFIG_FUNCTION_TRACER ftrace_enabled = enabled; + sysctl_ftrace_enabled = enabled; #endif } diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 00fcea236eba..773fdfc6c645 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -648,7 +648,7 @@ static struct ctl_table kern_table[] = { #ifdef CONFIG_FUNCTION_TRACER { .procname = "ftrace_enabled", - .data = &ftrace_enabled, + .data = &sysctl_ftrace_enabled, .maxlen = sizeof(int), .mode = 0644, .proc_handler = ftrace_enable_sysctl, diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index dacb8b50a263..b55c9a4e2b5b 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -88,7 +88,7 @@ struct ftrace_ops ftrace_list_end __read_mostly = { /* ftrace_enabled is a method to turn ftrace on or off */ int ftrace_enabled __read_mostly; -static int last_ftrace_enabled; +int sysctl_ftrace_enabled __read_mostly; /* Current function tracing op */ struct ftrace_ops *function_trace_op __read_mostly = &ftrace_list_end; @@ -6221,7 +6221,7 @@ void __init ftrace_init(void) pr_info("ftrace: allocating %ld entries in %ld pages\n", count, count / ENTRIES_PER_PAGE + 1); - last_ftrace_enabled = ftrace_enabled = 1; + sysctl_ftrace_enabled = ftrace_enabled = 1; ret = ftrace_process_locs(NULL, __start_mcount_loc, @@ -6265,6 +6265,7 @@ struct ftrace_ops global_ops = { static int __init ftrace_nodyn_init(void) { ftrace_enabled = 1; + sysctl_ftrace_enabled = 1; return 0; } core_initcall(ftrace_nodyn_init); @@ -6714,6 +6715,7 @@ void ftrace_kill(void) { ftrace_disabled = 1; ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; ftrace_trace_function = ftrace_stub; } @@ -6796,10 +6798,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, ret = proc_dointvec(table, write, buffer, lenp, ppos); - if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) + if (ret || !write || (ftrace_enabled == !!sysctl_ftrace_enabled)) goto out; - if (ftrace_enabled) { + if (sysctl_ftrace_enabled) { + + ftrace_enabled = true; /* we are starting ftrace again */ if (rcu_dereference_protected(ftrace_ops_list, @@ -6810,19 +6814,21 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, } else { if (is_permanent_ops_registered()) { - ftrace_enabled = true; ret = -EBUSY; goto out; } + ftrace_enabled = false; + /* stopping ftrace calls (just send to ftrace_stub) */ ftrace_trace_function = ftrace_stub; ftrace_shutdown_sysctl(); } - last_ftrace_enabled = !!ftrace_enabled; out: + sysctl_ftrace_enabled = ftrace_enabled; + mutex_unlock(&ftrace_lock); return ret; }
