Re: [PATCH v3 1/3] ftrace: Introduce PERMANENT ftrace_ops flag
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2019-10-16 13:49:00
Also in:
linux-kselftest, lkml
Subsystem:
function hooks (ftrace), proc sysctl, the rest, tracing · Maintainers:
Steven Rostedt, Masami Hiramatsu, Kees Cook, Joel Granados, Linus Torvalds
On Wed, 16 Oct 2019 13:33:13 +0200 Miroslav Benes [off-list ref] 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 <redacted> ---
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; }