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

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

[Atsushi Tsuji]

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);
 
 void
 unregister_ftrace_function_probe_func(char *glob, struct ftrace_probe_ops *ops)
-- 
1.5.5.1

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

[Peter Zijlstra]

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.

Its unusual to export bits without an in-kernel user.

> 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);
>  
>  void
>  unregister_ftrace_function_probe_func(char *glob, struct ftrace_probe_ops *ops)

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

[Steven Rostedt]

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

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!

-- Steve

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

[Masami Hiramatsu]

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

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

[Steven Rostedt]

On Tue, 2009-09-15 at 10:01 -0400, Masami Hiramatsu wrote:
> Steven Rostedt wrote:

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

Yes ;-)

I don't want to disable preemption when I don't have to. The function
tracer that is called can. But actually, it's ever more that that. If
you only register a single function, it will call that function
directly. Then there will always be a race window between when the
function gets called and disabling preemption, even if the called
function disables preemption as the first thing it does.

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

Yes, currently a branch can only got to a single function ;-)

What ftrace does, is if you register a single function, the mcount
location will call that function directly. But if you register an second
function, it changes the mcount call site to call a wrapper that loops
to through all the functions that are registered.

Thus a module version would need to call that wrapper every time. Even
if only one function is called. And this wrapper would have to disable
preemption for the entire loop.


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

And it does not.

-- Steve

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

[Christoph Hellwig]

On Tue, Sep 15, 2009 at 07:06:32PM +0900, Atsushi Tsuji wrote:
> Export register_ and unresgister_ftrace_function_probe to modules. This can
> be used by SystemTap.

And can be added to the kernel treee once we merge a reasonably
rewritten version of it, if at all possible.

Remember that we do not add random crap for out of tree modules.

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

[Atsushi Tsuji]

Steven Rostedt wrote:
> On Tue, 2009-09-15 at 10:01 -0400, Masami Hiramatsu wrote:
>> Steven Rostedt wrote:
> 
>>> 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?
> 
> Yes ;-)
> 
> I don't want to disable preemption when I don't have to. The function
> tracer that is called can. But actually, it's ever more that that. If
> you only register a single function, it will call that function
> directly. Then there will always be a race window between when the
> function gets called and disabling preemption, even if the called
> function disables preemption as the first thing it does.

Thank you for detailed explanation.

I may be wrong, but I think function_trace_probe_call using 
register_ftrace_function_probe is almost enough for modules,
since it disables preemption while a probe is calling and it
called every time even if only one probe function is registered.
So is it enough to make a new registering function using
it and upping module ref count for module safe?  

Or should I make another handler for modules not using
function_trace_probe_call?

>>> 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.
> 
> And it does not.

I think the preemption is disabled in not register_ftrace_function
but register_ftrace_function_probe, is that wrong?

Thanks,
Atsushi

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

[Steven Rostedt]

On Wed, 2009-09-16 at 15:09 +0900, Atsushi Tsuji wrote:

> > I don't want to disable preemption when I don't have to. The function
> > tracer that is called can. But actually, it's ever more that that. If
> > you only register a single function, it will call that function
> > directly. Then there will always be a race window between when the
> > function gets called and disabling preemption, even if the called
> > function disables preemption as the first thing it does.
> 
> Thank you for detailed explanation.
> 
> I may be wrong, but I think function_trace_probe_call using 
> register_ftrace_function_probe is almost enough for modules,

Heh, I forgot about function_probe. Yeah, looking at that, it does seem
that it would be safe for modules.

> since it disables preemption while a probe is calling and it
> called every time even if only one probe function is registered.
> So is it enough to make a new registering function using
> it and upping module ref count for module safe?  

Yes, upping the module ref count for every function probe would be
required. Unfortunately, this would require adding another variable to
struct ftrace_func_probe, which I hate to do.


> 
> Or should I make another handler for modules not using
> function_trace_probe_call?

Maybe another handler might be better. But it may be similar to
function_probe.

> 
> >>> 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.
> > 
> > And it does not.
> 
> I think the preemption is disabled in not register_ftrace_function
> but register_ftrace_function_probe, is that wrong?

No that's correct. It's been a while since I worked on the probe code,
so I forgot about it :-)

-- Steve

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

[Atsushi Tsuji]

Steven Rostedt wrote:
> On Wed, 2009-09-16 at 15:09 +0900, Atsushi Tsuji wrote:
> 
>>> I don't want to disable preemption when I don't have to. The function
>>> tracer that is called can. But actually, it's ever more that that. If
>>> you only register a single function, it will call that function
>>> directly. Then there will always be a race window between when the
>>> function gets called and disabling preemption, even if the called
>>> function disables preemption as the first thing it does.
>> Thank you for detailed explanation.
>>
>> I may be wrong, but I think function_trace_probe_call using 
>> register_ftrace_function_probe is almost enough for modules,
> 
> Heh, I forgot about function_probe. Yeah, looking at that, it does seem
> that it would be safe for modules.
> 
>> since it disables preemption while a probe is calling and it
>> called every time even if only one probe function is registered.
>> So is it enough to make a new registering function using
>> it and upping module ref count for module safe?  
> 
> Yes, upping the module ref count for every function probe would be
> required. Unfortunately, this would require adding another variable to
> struct ftrace_func_probe, which I hate to do.
> 
> 
>> Or should I make another handler for modules not using
>> function_trace_probe_call?
> 
> Maybe another handler might be better. But it may be similar to
> function_probe.

I see. I'll try to make a new handler for modules.

Thanks,
Atsushi

> 
>>>>> 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.
>>> And it does not.
>> I think the preemption is disabled in not register_ftrace_function
>> but register_ftrace_function_probe, is that wrong?
> 
> No that's correct. It's been a while since I worked on the probe code,
> so I forgot about it :-)
> 
> -- Steve
> 
> 
> 
>