Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457623 > unrolled thread

A bug in ftrace - dynamic fops

Back to article view | Back to linux.kernel

Contents

  A bug in ftrace - dynamic fops Miroslav Benes <mbenes@suse.cz> - 2016-08-08 11:00 +0200
    Re: A bug in ftrace - dynamic fops Steven Rostedt <rostedt@goodmis.org> - 2016-08-08 17:00 +0200
      Re: A bug in ftrace - dynamic fops Miroslav Benes <mbenes@suse.cz> - 2016-08-09 10:20 +0200
        Re: A bug in ftrace - dynamic fops Steven Rostedt <rostedt@goodmis.org> - 2016-08-09 14:20 +0200

#1457623 — A bug in ftrace - dynamic fops

Hi Steven,

I am afraid there is a bug in the current mainline's ftrace when dynamic 
fops are involved.

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.

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.

or

2. implement the whole thing through RCU_TASKS. This would also enable 
dynamic trampolines for PREEMPT kernels.

Revert of 00ccbf2f5b75 commit would be solution as well but there is a 
drawback.

Regards,
Miroslav

[toc] | [next] | [standalone]

#1457852

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

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

-- Steve



> 
> Revert of 00ccbf2f5b75 commit would be solution as well but there is a 
> drawback.
> 
> Regards,
> Miroslav

[toc] | [prev] | [next] | [standalone]

#1458489

On Mon, 8 Aug 2016, Steven Rostedt wrote:

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

[toc] | [prev] | [next] | [standalone]

#1458658

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.


> 
> 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.

It will warn if you hook to a function that can sleep. And if you never
do, then there's nothing wrong. If the only functions you hook to can
sleep, then it is fine for you to sleep in your code too. But if you
do, you must synchronize that logic. You must make sure all functions
are out of the sleep when you unresgister. Just like you must make sure
all functions are out of a sleeping function in a module. This is
kernel programming 101.

I never saw a need to have sleeping functions being called by
ops->func() and I don't know of a case that would. If there is a
legitimate case (not hypothetical) and then I could add a way to
postpone freeing of an ops if need be.

Because note, that TASK_RCU will only be called when CONFIG_PREEMPT is
enabled. It would be overkill to do it for !CONFIG_PREEMPT, thus it
will not solve what you want here.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web