On Wed, 16 Oct 2019 13:33:13 +0200 Miroslav Benes mbenes@suse.cz 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 mbenes@suse.cz Reviewed-by: Petr Mladek pmladek@suse.com Reviewed-by: Kamalesh Babulal kamalesh@linux.vnet.ibm.com
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; }