On Thu, Apr 06, 2017 at 12:42:38PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <[email protected]> > > The function tracer needs to be more careful than other subsystems when it > comes to freeing data. Especially if that data is actually executable code. > When a single function is traced, a trampoline can be dynamically allocated > which is called to jump to the function trace callback. When the callback is > no longer needed, the dynamic allocated trampoline needs to be freed. This > is where the issues arise. The dynamically allocated trampoline must not be > used again. As function tracing can trace all subsystems, including > subsystems that are used to serialize aspects of freeing (namely RCU), it > must take extra care when doing the freeing. > > Before synchronize_rcu_tasks() was around, there was no way for the function > tracer to know that nothing was using the dynamically allocated trampoline > when CONFIG_PREEMPT was enabled. That's because a task could be indefinitely > preempted while sitting on the trampoline. Now with synchronize_rcu_tasks(), > it will wait till all tasks have either voluntarily scheduled (not on the > trampoline) or goes into userspace (not on the trampoline). Then it is safe > to free the trampoline even with CONFIG_PREEMPT set. > > Cc: "Paul E. McKenney" <[email protected]>
One question below. Other than that: Acked-by: "Paul E. McKenney" <[email protected]> > Signed-off-by: Steven Rostedt (VMware) <[email protected]> > --- > kernel/trace/Kconfig | 3 ++- > kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------ > 2 files changed, 20 insertions(+), 25 deletions(-) > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig > index d4a06e714645..67b463b4f169 100644 > --- a/kernel/trace/Kconfig > +++ b/kernel/trace/Kconfig > @@ -134,7 +134,8 @@ config FUNCTION_TRACER > select KALLSYMS > select GENERIC_TRACER > select CONTEXT_SWITCH_TRACER > - select GLOB > + select GLOB Does GLOB really want to be selected in production environments? I could understand "select GLOB if WE_ARE_TESTING" or some such. > + select TASKS_RCU if PREEMPT > help > Enable the kernel to trace every kernel function. This is done > by using a compiler feature to insert a small, 5-byte No-Operation > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 8efd9fe7aec0..34f63e78d661 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -2808,18 +2808,28 @@ static int ftrace_shutdown(struct ftrace_ops *ops, > int command) > * callers are done before leaving this function. > * The same goes for freeing the per_cpu data of the per_cpu > * ops. > - * > - * Again, normal synchronize_sched() is not good enough. > - * We need to do a hard force of sched synchronization. > - * This is because we use preempt_disable() to do RCU, but > - * the function tracers can be called where RCU is not watching > - * (like before user_exit()). We can not rely on the RCU > - * infrastructure to do the synchronization, thus we must do it > - * ourselves. > */ > if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) { > + /* > + * We need to do a hard force of sched synchronization. > + * This is because we use preempt_disable() to do RCU, but > + * the function tracers can be called where RCU is not watching > + * (like before user_exit()). We can not rely on the RCU > + * infrastructure to do the synchronization, thus we must do it > + * ourselves. > + */ > schedule_on_each_cpu(ftrace_sync); > > + /* > + * When the kernel is preeptive, tasks can be preempted > + * while on a ftrace trampoline. Just scheduling a task on > + * a CPU is not good enough to flush them. Calling > + * synchornize_rcu_tasks() will wait for those tasks to > + * execute and either schedule voluntarily or enter user space. > + */ > + if (IS_ENABLED(CONFIG_PREEMPT)) > + synchronize_rcu_tasks(); > + > arch_ftrace_trampoline_free(ops); > > if (ops->flags & FTRACE_OPS_FL_PER_CPU) > @@ -5366,22 +5376,6 @@ void __weak arch_ftrace_update_trampoline(struct > ftrace_ops *ops) > > static void ftrace_update_trampoline(struct ftrace_ops *ops) > { > - > -/* > - * Currently there's no safe way to free a trampoline when the kernel > - * is configured with PREEMPT. That is because a task could be preempted > - * when it jumped to the trampoline, it may be preempted for a long time > - * depending on the system load, and currently there's no way to know > - * when it will be off the trampoline. If the trampoline is freed > - * too early, when the task runs again, it will be executing on freed > - * memory and crash. > - */ > -#ifdef CONFIG_PREEMPT > - /* Currently, only non dynamic ops can have a trampoline */ > - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) > - return; > -#endif > - > arch_ftrace_update_trampoline(ops); > } > > -- > 2.10.2 > >
