Re: A bug in ftrace - dynamic fops

[Miroslav Benes]

On Mon, 8 Aug 2016, Steven Rostedt wrote:

> On Mon, 8 Aug 2016 10:57:45 +0200 (CEST)
> Miroslav Benes <mbenes@xxxxxxx> 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