Re: A bug in ftrace - dynamic fops

[Miroslav Benes]

On Thu, 11 Aug 2016, Steven Rostedt wrote:

> On Thu, 11 Aug 2016 10:46:53 +0200 (CEST)
> Miroslav Benes <mbenes@xxxxxxx> wrote:
> 
> > On Tue, 9 Aug 2016, Steven Rostedt wrote:
> > 
> > > On Tue, 9 Aug 2016 10:16:00 +0200 (CEST)
> > > Miroslav Benes <mbenes@xxxxxxx> 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