Messages in this thread | | | Date | Thu, 11 Aug 2016 16:08:58 +0200 (CEST) | From | Miroslav Benes <> | Subject | Re: A bug in ftrace - dynamic fops |
| |
On Thu, 11 Aug 2016, Steven Rostedt wrote:
> On Thu, 11 Aug 2016 10:46:53 +0200 (CEST) > Miroslav Benes <mbenes@suse.cz> wrote: > > > On Tue, 9 Aug 2016, Steven Rostedt wrote: > > > > > On Tue, 9 Aug 2016 10:16:00 +0200 (CEST) > > > Miroslav Benes <mbenes@suse.cz> wrote: > > > > > > > > > > I agree it is kind of shooting oneself in the foot bug, because explicit > > > > call to a sleeping function may not be the brightest thing to do. However > > > > I see two (closely related) issues with this. > > > > > > > > 1. It is a change in behaviour. Ftrace silently relies on an atomicity of > > > > ops->func(). I don't see it documented anywhere, but it did not matter > > > > because the atomicity was always guaranteed as described above. Now there > > > > is a possibility to achieve a situation which breaks the assumption. It > > > > makes me worried. > > > > > > Why? It's something that a kernel developer should be aware of. I mean, > > > that ops->func can easily be called from *any* context, like irq, > > > softirq, or even an NMI. One who hooks into any function of the kernel > > > should understand that it has special requirements, just like we don't > > > document that you can't sleep in an NMI. > > > > > > And if you only hook to functions that can sleep, then great! You are > > > allowed to do that too. Just like calling a module function that can > > > sleep. You need to make sure nothing is calling your function when you > > > unload the module. I don't see anything that is deceptive here. > > > > At least the comment in ftrace_shutdown() is deceptive. > > Which comment? It may require an update to be less "deceptive".
/* * Dynamic ops may be freed, we must make sure that all * 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)) { schedule_on_each_cpu(ftrace_sync);
arch_ftrace_trampoline_free(ops);
if (ops->flags & FTRACE_OPS_FL_PER_CPU) per_cpu_ops_free(ops); }
I think the wording could be interpreted in a way that ftrace is responsible which is not true according to you.
Miroslav
| |