Re: [PATCH 2/2] tracing: Export ftrace API for kernel modules

[Masami Hiramatsu <mhiramat@redhat.com>]

Steven Rostedt wrote:
> On Tue, 2009-09-15 at 19:06 +0900, Atsushi Tsuji wrote:
>> Export register_ and unresgister_ftrace_function_probe to modules. This can
>> be used by SystemTap.
>>
>> Signed-off-by: Atsushi Tsuji <a-tsuji@bk.jp.nec.com>
>> ---
>>  kernel/trace/ftrace.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 5ef8f59..9c32291 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -2042,6 +2042,7 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>>  
>>  	return count;
>>  }
>> +EXPORT_SYMBOL_GPL(register_ftrace_function_probe);
>>  
>>  enum {
>>  	PROBE_TEST_FUNC		= 1,
>> @@ -2108,6 +2109,7 @@ unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>>  	__unregister_ftrace_function_probe(glob, ops, data,
>>  					  PROBE_TEST_FUNC | PROBE_TEST_DATA);
>>  }
>> +EXPORT_SYMBOL_GPL(unregister_ftrace_function_probe);
> 
> I have to NAK this as is. There's a reason I never exported these to
> modules, and that is because they are not module safe. There's nothing
> to stop a module from registering a probe and then being removed. Then
> the function probe will still be called causing a kernel panic.
> 
> 
> Now I know of two ways to fix this.
> 
> 1) The simple way. Up the module ref count so once it registers a
> function it can never be disabled. Of course there's the "force module
> unload" but people should not do that anyway.
> 
> 2) Create a second hook handler for modules. That is the function caller
> for modules will go to a wrapper first. This wrapper could disable
> interrupts or grab a lock or something that would prevent a module from
> being unloaded as the hooks are being called. Perhaps even disabling
> preemption while calling the hooks will be enough (this is not something
> I want the normal function caller to do).

I think this is better solution.
Out of curiously, is disabling preemption so harmful?

> And the current function
> tracer will optimize that if only one function is registered to mcount,
> then the mcount caller will call that function directly.

Would you mean that current mcount hook supports only one handler?

> 
> It will still need to up the mod ref count when a probe is added, but it
> can also remove it.
> 
> 
> The problem with the current method, is that a probe can be executing at
> anytime. Here's an example if we did it your way.
> 
> 1. module installed
> 2. module adds probe
> 3. function X in kernel calls probe but gets preempted.
> 4. module removes probe
> 5. module unistalled
> 6. function X in kernel continues to run probe but probe no longer
> exists --- Oops!

Agreed, if mcount doesn't disable preemption, this will happen.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com