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

Groups > linux.kernel > #1460279 > unrolled thread

Re: A bug in ftrace - dynamic fops

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

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

#1460279 — Re: A bug in ftrace - dynamic fops

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

[toc] | [next] | [standalone]

#1460549

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

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

#1460574

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

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

#1460585

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

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

#1460554

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web