On Mon, 8 Aug 2016, Steven Rostedt wrote:
> On Mon, 8 Aug 2016 10:57:45 +0200 (CEST)
> Miroslav Benes <mbe...@suse.cz> wrote:
>
> > Hi Steven,
> >
> > I am afraid there is a bug in the current mainline's ftrace when dynamic
> > fops are involved.
>
> I'm sorry but I don't see it.
>
> >
> > ftrace_shutdown() relies on schedule_on_each_cpu() which should ensure
> > that no task stays in a ftrace handler. This was sufficient for a long
> > time because every handler was called with the preemption disabled thanks
> > to ftrace_ops_list_func (or ftrace_ops_assist_func). Dynamic trampolines
> > did not change the behaviour because !PREEMPT was required (commit
> > 12cce594fa8f ("ftrace/x86: Allow !CONFIG_PREEMPT dynamic ops to use
> > allocated trampolines")).
> >
> > Situation changed with the commit 00ccbf2f5b75 ("ftrace/x86: Let dynamic
> > trampolines call ops->func even for dynamic fops"). The purpose of the
> > patch is clear - to call ops->func whenever possible and thus gain an
> > advantage of dynamic trampolines. But it also allows the handler (that
> > very ops->func) to sleep because no atomic context is enforced. This
> > breaks the assumption for schedule_on_each_cpu() in ftrace_shutdown() and
> > one can crash the kernel quite easily.
>
> Not when CONFIG_PREEMPT is not enabled. This is because the kernel does
> not preempt unless it specifically asks to be preempted. If ops->func()
> calls a mutex, or sleeps, then *THAT* is a bug! ops->func() is more
> sensitive than interrupt handlers, and all ops->func()s must use extra
> care. This sounds like a "doctor it hurts me when I do this" bug (where
> the doctor replies "well, don't do that").
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.
2. Previously if someone called a function which could sleep he was
immediately warned not to do so via "sleeping in atomic context" BUG. Now
he wouldn't know. That's because in_atomic() and might_sleep()
infrastructure does not work in ops->func(). in_atomic() gives 0 even if
it is an atomic context in fact. But well, the comment for in_atomic() in
linux/preempt.h warns about exactly this situation I guess.
Anyway, it is your call.
> If something registers an ops->func() that can sleep, then it MUST limit
> the functions that it can be registered to, and also must handle any
> synchronization of the ops itself being freed. This isn't the ftrace
> infrastructure's responsibility.
>
> >
> > It suffices to register dynamic fops with FTRACE_OPS_FL_RECURSION_SAFE set
> > (because otherwise ftrace_ops_assist_func() is used which also disables
> > preemption), sleep in the handler and meanwhile remove it.
> >
> > I can imagine two reasonable solutions...
> >
> > 1. introduce something similar to ftrace_ops_assist_func() which would
> > just disable preemption before calling ops->func and enable it afterwards.
>
> With CONFIG_PREEMPT disabled, how can ops->func() sleep? It can't,
> unless it specifically calls a function that can. I don't see the bug
> you mention here.
>
> >
> > or
> >
> > 2. implement the whole thing through RCU_TASKS. This would also enable
> > dynamic trampolines for PREEMPT kernels.
>
> That said, this has been on my todo list for too long, and will soon be
> implemented. I want CONFIG_PREEMPT to allow dynamic trampolines for
> dynamic ops too. Not to mention, RCU_TASKS was specifically written for
> me to do this. I dropped the ball on this one. :-p
That is great to hear. Looking forward to seeing that.
Thanks,
Miroslav
