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