A bug in ftrace - dynamic fops

A bug in ftrace - dynamic fops

[Miroslav Benes]

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

Re: A bug in ftrace - dynamic fops

[Steven Rostedt]

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

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

Re: A bug in ftrace - dynamic fops

[Steven Rostedt]

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

Re: A bug in ftrace - dynamic fops

[Miroslav Benes]

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.

But well, I understood your opinion from the first reply. I just didn't 
agree with it and that's why I expressed it. 

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

Fair enough. I can live with that.

Miroslav

Re: A bug in ftrace - dynamic fops

[Steven Rostedt]

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

-- Steve

> 
> But well, I understood your opinion from the first reply. I just didn't 
> agree with it and that's why I expressed it. 
> 

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

Re: A bug in ftrace - dynamic fops

[Steven Rostedt]

On Thu, 11 Aug 2016 16:08:58 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

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

OK, then I should update it to be a bit more clear, that it is only
worried about its own infrastructure and not what goes on within
ops->func().

I take feedback like yours seriously. If you are confused by it, then
others may be too.

Thus, I think there should be two points documented a bit better.

#1, ops->func() is special, and can be called from any context
including NMI. That means unless you take special care about exactly
what functions are going to be traced (via the filter hashes, which are
not even supported when DYNAMIC_FTRACE is not set), then the
ops->func() must be treated with the same care as an NMI handler would
be. (no locking, no sleeping, etc).

#2, the only synchronization that ftrace will take care of is its own.
That is, the dynamic trampolines have race conditions in the
implementation. The above comment is all about handling its own race
conditions, and doesn't care about what goes on within ops->func().
Although it does give a utility if ops->func() is not recursion safe,
but other than than, like all other function hooks, any synchronization
within the hooks must be taken care of by the caller to
(un)register_ftrace_function(). That includes hooks doing strange
things (like sleeping) and then freeing the ops after the registering.

Although, with #1, #2 may not be needed.

-- Steve

Re: A bug in ftrace - dynamic fops

[Miroslav Benes]

On Thu, 11 Aug 2016, Steven Rostedt wrote:

> On Thu, 11 Aug 2016 16:08:58 +0200 (CEST)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> >         /*
> >          * 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.
> 
> OK, then I should update it to be a bit more clear, that it is only
> worried about its own infrastructure and not what goes on within
> ops->func().
> 
> I take feedback like yours seriously. If you are confused by it, then
> others may be too.
> 
> Thus, I think there should be two points documented a bit better.
> 
> #1, ops->func() is special, and can be called from any context
> including NMI. That means unless you take special care about exactly
> what functions are going to be traced (via the filter hashes, which are
> not even supported when DYNAMIC_FTRACE is not set), then the
> ops->func() must be treated with the same care as an NMI handler would
> be. (no locking, no sleeping, etc).
> 
> #2, the only synchronization that ftrace will take care of is its own.
> That is, the dynamic trampolines have race conditions in the
> implementation. The above comment is all about handling its own race
> conditions, and doesn't care about what goes on within ops->func().
> Although it does give a utility if ops->func() is not recursion safe,
> but other than than, like all other function hooks, any synchronization
> within the hooks must be taken care of by the caller to
> (un)register_ftrace_function(). That includes hooks doing strange
> things (like sleeping) and then freeing the ops after the registering.
> 
> Although, with #1, #2 may not be needed.

May not, but it would not hurt to include it as well. There are some good 
points there and it makes clear that what I originally assumed to be a 
wanted behaviour is merely a side effect.

So such two comments in the code or in the documentation would more than 
satisfy me.

Thanks a lot,
Miroslav