[PATCH] tracing/profile: Fix profile_disable vs module_unload

[PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

If the correspoding module is unloaded before ftrace_profile_disable()
is called, event->profile_disable() won't be called, which can
cause oops:

  # insmod trace-events-sample.ko
  # perf record -f -a -e sample:foo_bar sleep 3 &
  # sleep 1
  # rmmod trace_events_sample
  # insmod trace-events-sample.ko
  OOPS!

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---

resend, CC LKML..

---
 kernel/trace/trace_event_profile.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index 11ba5bb..55a25c9 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -5,6 +5,7 @@
  *
  */
 
+#include <linux/module.h>
 #include "trace.h"
 
 int ftrace_profile_enable(int event_id)
@@ -14,7 +15,8 @@ int ftrace_profile_enable(int event_id)
 
 	mutex_lock(&event_mutex);
 	list_for_each_entry(event, &ftrace_events, list) {
-		if (event->id == event_id && event->profile_enable) {
+		if (event->id == event_id && event->profile_enable &&
+		    try_module_get(event->mod)) {
 			ret = event->profile_enable(event);
 			break;
 		}
@@ -32,6 +34,7 @@ void ftrace_profile_disable(int event_id)
 	list_for_each_entry(event, &ftrace_events, list) {
 		if (event->id == event_id) {
 			event->profile_disable(event);
+			module_put(event->mod);
 			break;
 		}
 	}
-- 
1.6.3

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Mon, 2009-08-24 at 12:19 +0800, Li Zefan wrote:
> If the correspoding module is unloaded before ftrace_profile_disable()
> is called, event->profile_disable() won't be called, which can
> cause oops:
> 
>   # insmod trace-events-sample.ko
>   # perf record -f -a -e sample:foo_bar sleep 3 &
>   # sleep 1
>   # rmmod trace_events_sample
>   # insmod trace-events-sample.ko
>   OOPS!
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>


Hrmm, feel fragile, why don't we check if all a modules tracepoints are
unused on unload?

> ---
>  kernel/trace/trace_event_profile.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
> index 11ba5bb..55a25c9 100644
> --- a/kernel/trace/trace_event_profile.c
> +++ b/kernel/trace/trace_event_profile.c
> @@ -5,6 +5,7 @@
>   *
>   */
>  
> +#include <linux/module.h>
>  #include "trace.h"
>  
>  int ftrace_profile_enable(int event_id)
> @@ -14,7 +15,8 @@ int ftrace_profile_enable(int event_id)
>  
>  	mutex_lock(&event_mutex);
>  	list_for_each_entry(event, &ftrace_events, list) {
> -		if (event->id == event_id && event->profile_enable) {
> +		if (event->id == event_id && event->profile_enable &&
> +		    try_module_get(event->mod)) {
>  			ret = event->profile_enable(event);
>  			break;
>  		}
> @@ -32,6 +34,7 @@ void ftrace_profile_disable(int event_id)
>  	list_for_each_entry(event, &ftrace_events, list) {
>  		if (event->id == event_id) {
>  			event->profile_disable(event);
> +			module_put(event->mod);
>  			break;
>  		}
>  	}

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

Peter Zijlstra wrote:
> On Mon, 2009-08-24 at 12:19 +0800, Li Zefan wrote:
>> If the correspoding module is unloaded before ftrace_profile_disable()
>> is called, event->profile_disable() won't be called, which can
>> cause oops:
>>
>>   # insmod trace-events-sample.ko
>>   # perf record -f -a -e sample:foo_bar sleep 3 &
>>   # sleep 1
>>   # rmmod trace_events_sample
>>   # insmod trace-events-sample.ko
>>   OOPS!
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> 
> Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> unused on unload?
> 

I don't think it's fragile. We are profiling via a module's
tracepoint, so we should pin the module, via module_get().
If event->profile_enable() has been calld, we should make
sure it's profile_disable() will be called.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Mon, 2009-08-24 at 14:22 +0800, Li Zefan wrote:
> Peter Zijlstra wrote:
> > On Mon, 2009-08-24 at 12:19 +0800, Li Zefan wrote:
> >> If the correspoding module is unloaded before ftrace_profile_disable()
> >> is called, event->profile_disable() won't be called, which can
> >> cause oops:
> >>
> >>   # insmod trace-events-sample.ko
> >>   # perf record -f -a -e sample:foo_bar sleep 3 &
> >>   # sleep 1
> >>   # rmmod trace_events_sample
> >>   # insmod trace-events-sample.ko
> >>   OOPS!
> >>
> >> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > 
> > Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> > unused on unload?
> > 
> 
> I don't think it's fragile. We are profiling via a module's
> tracepoint, so we should pin the module, via module_get().
> If event->profile_enable() has been calld, we should make
> sure it's profile_disable() will be called.

What I call fragile is that everyone registering a tracepoint callback
will now apparently need to worry about modules, _that_ is fragile.

Either make module unload look at tracepoint users, or place the
try_get_module() in the registration hooks so that regular users don't
need to worry about it.

But this is rediculous.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, 2009-08-24 at 14:22 +0800, Li Zefan wrote:
> > Peter Zijlstra wrote:
> > > On Mon, 2009-08-24 at 12:19 +0800, Li Zefan wrote:
> > >> If the correspoding module is unloaded before ftrace_profile_disable()
> > >> is called, event->profile_disable() won't be called, which can
> > >> cause oops:
> > >>
> > >>   # insmod trace-events-sample.ko
> > >>   # perf record -f -a -e sample:foo_bar sleep 3 &
> > >>   # sleep 1
> > >>   # rmmod trace_events_sample
> > >>   # insmod trace-events-sample.ko
> > >>   OOPS!
> > >>
> > >> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > > 
> > > 
> > > Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> > > unused on unload?
> > > 
> > 
> > I don't think it's fragile. We are profiling via a module's
> > tracepoint, so we should pin the module, via module_get().
> > If event->profile_enable() has been calld, we should make
> > sure it's profile_disable() will be called.
> 
> What I call fragile is that everyone registering a tracepoint 
> callback will now apparently need to worry about modules, _that_ 
> is fragile.
> 
> Either make module unload look at tracepoint users, or place the 
> try_get_module() in the registration hooks so that regular users 
> don't need to worry about it.

The bug found by Li needs to be fixed obviously.

I tend to agree with you that this does not appear to be the best 
place to do it: so you suggest to implicitly increase the module 
refcount on callback registr instead? (and releasing it when 
unregistering)

Same end result, slightly cleaner place to bump the refcount.

	Ingo

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Mon, 2009-08-24 at 11:24 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, 2009-08-24 at 14:22 +0800, Li Zefan wrote:
> > > Peter Zijlstra wrote:
> > > > On Mon, 2009-08-24 at 12:19 +0800, Li Zefan wrote:
> > > >> If the correspoding module is unloaded before ftrace_profile_disable()
> > > >> is called, event->profile_disable() won't be called, which can
> > > >> cause oops:
> > > >>
> > > >>   # insmod trace-events-sample.ko
> > > >>   # perf record -f -a -e sample:foo_bar sleep 3 &
> > > >>   # sleep 1
> > > >>   # rmmod trace_events_sample
> > > >>   # insmod trace-events-sample.ko
> > > >>   OOPS!
> > > >>
> > > >> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > > > 
> > > > 
> > > > Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> > > > unused on unload?
> > > > 
> > > 
> > > I don't think it's fragile. We are profiling via a module's
> > > tracepoint, so we should pin the module, via module_get().
> > > If event->profile_enable() has been calld, we should make
> > > sure it's profile_disable() will be called.
> > 
> > What I call fragile is that everyone registering a tracepoint 
> > callback will now apparently need to worry about modules, _that_ 
> > is fragile.
> > 
> > Either make module unload look at tracepoint users, or place the 
> > try_get_module() in the registration hooks so that regular users 
> > don't need to worry about it.
> 
> The bug found by Li needs to be fixed obviously.
> 
> I tend to agree with you that this does not appear to be the best 
> place to do it: so you suggest to implicitly increase the module 
> refcount on callback registr instead? (and releasing it when 
> unregistering)
> 
> Same end result, slightly cleaner place to bump the refcount.

Yes, because the user of tracepoints should never need to care about
modules.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Mon, 24 Aug 2009, Peter Zijlstra wrote:
> > > > > 
> > > > > 
> > > > > Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> > > > > unused on unload?
> > > > > 
> > > > 
> > > > I don't think it's fragile. We are profiling via a module's
> > > > tracepoint, so we should pin the module, via module_get().
> > > > If event->profile_enable() has been calld, we should make
> > > > sure it's profile_disable() will be called.
> > > 
> > > What I call fragile is that everyone registering a tracepoint 
> > > callback will now apparently need to worry about modules, _that_ 
> > > is fragile.
> > > 
> > > Either make module unload look at tracepoint users, or place the 
> > > try_get_module() in the registration hooks so that regular users 
> > > don't need to worry about it.
> > 
> > The bug found by Li needs to be fixed obviously.
> > 
> > I tend to agree with you that this does not appear to be the best 
> > place to do it: so you suggest to implicitly increase the module 
> > refcount on callback registr instead? (and releasing it when 
> > unregistering)
> > 
> > Same end result, slightly cleaner place to bump the refcount.
> 
> Yes, because the user of tracepoints should never need to care about
> modules.

I also agree with Peter on this.

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

>>>>>> If the correspoding module is unloaded before ftrace_profile_disable()
>>>>>> is called, event->profile_disable() won't be called, which can
>>>>>> cause oops:
>>>>>>
>>>>>>   # insmod trace-events-sample.ko
>>>>>>   # perf record -f -a -e sample:foo_bar sleep 3 &
>>>>>>   # sleep 1
>>>>>>   # rmmod trace_events_sample
>>>>>>   # insmod trace-events-sample.ko
>>>>>>   OOPS!
>>>>>>
>>>>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>>>>
>>>>> Hrmm, feel fragile, why don't we check if all a modules tracepoints are
>>>>> unused on unload?
>>>>>
>>>> I don't think it's fragile. We are profiling via a module's
>>>> tracepoint, so we should pin the module, via module_get().
>>>> If event->profile_enable() has been calld, we should make
>>>> sure it's profile_disable() will be called.
>>> What I call fragile is that everyone registering a tracepoint 
>>> callback will now apparently need to worry about modules, _that_ 
>>> is fragile.
>>>
>>> Either make module unload look at tracepoint users, or place the 
>>> try_get_module() in the registration hooks so that regular users 
>>> don't need to worry about it.
>> The bug found by Li needs to be fixed obviously.
>>
>> I tend to agree with you that this does not appear to be the best 
>> place to do it: so you suggest to implicitly increase the module 
>> refcount on callback registr instead? (and releasing it when 
>> unregistering)
>>
>> Same end result, slightly cleaner place to bump the refcount.
> 
> Yes, because the user of tracepoints should never need to care about
> modules.
> 

I'm afraid it is not feasible to bump module refcnt implicitly
in tracepoint_probe_register().

If a tracepoint is registered in module_init, and unregistered
in module_exit (see sample/tracepoints), the module is unloadable:

 insmod
 ->call mod->init()
   ->trace_reg_foo()
     ->module_get()

 rmmod
 ->check mod refcnt
 ->call mod->exit()
   ->trace_unreg_foo()
     ->module_put()

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 13:22 +0800, Li Zefan wrote:
> >>>>>> If the correspoding module is unloaded before ftrace_profile_disable()
> >>>>>> is called, event->profile_disable() won't be called, which can
> >>>>>> cause oops:
> >>>>>>
> >>>>>>   # insmod trace-events-sample.ko
> >>>>>>   # perf record -f -a -e sample:foo_bar sleep 3 &
> >>>>>>   # sleep 1
> >>>>>>   # rmmod trace_events_sample
> >>>>>>   # insmod trace-events-sample.ko
> >>>>>>   OOPS!
> >>>>>>
> >>>>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> >>>>>
> >>>>> Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> >>>>> unused on unload?
> >>>>>
> >>>> I don't think it's fragile. We are profiling via a module's
> >>>> tracepoint, so we should pin the module, via module_get().
> >>>> If event->profile_enable() has been calld, we should make
> >>>> sure it's profile_disable() will be called.
> >>> What I call fragile is that everyone registering a tracepoint 
> >>> callback will now apparently need to worry about modules, _that_ 
> >>> is fragile.
> >>>
> >>> Either make module unload look at tracepoint users, or place the 
> >>> try_get_module() in the registration hooks so that regular users 
> >>> don't need to worry about it.
> >> The bug found by Li needs to be fixed obviously.
> >>
> >> I tend to agree with you that this does not appear to be the best 
> >> place to do it: so you suggest to implicitly increase the module 
> >> refcount on callback registr instead? (and releasing it when 
> >> unregistering)
> >>
> >> Same end result, slightly cleaner place to bump the refcount.
> > 
> > Yes, because the user of tracepoints should never need to care about
> > modules.
> > 
> 
> I'm afraid it is not feasible to bump module refcnt implicitly
> in tracepoint_probe_register().
> 
> If a tracepoint is registered in module_init, and unregistered
> in module_exit (see sample/tracepoints), the module is unloadable:
> 
>  insmod
>  ->call mod->init()
>    ->trace_reg_foo()
>      ->module_get()
> 
>  rmmod
>  ->check mod refcnt
>  ->call mod->exit()
>    ->trace_unreg_foo()
>      ->module_put()

Not tracepoint_probe_{un,}register(), in {un,}register_trace_$call().

Basically avoid module unload when a tracepoint from that module has
registered callbacks.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

>>>>>>>> If the correspoding module is unloaded before ftrace_profile_disable()
>>>>>>>> is called, event->profile_disable() won't be called, which can
>>>>>>>> cause oops:
>>>>>>>>
>>>>>>>>   # insmod trace-events-sample.ko
>>>>>>>>   # perf record -f -a -e sample:foo_bar sleep 3 &
>>>>>>>>   # sleep 1
>>>>>>>>   # rmmod trace_events_sample
>>>>>>>>   # insmod trace-events-sample.ko
>>>>>>>>   OOPS!
>>>>>>>>
>>>>>>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>>>>>> Hrmm, feel fragile, why don't we check if all a modules tracepoints are
>>>>>>> unused on unload?
>>>>>>>
>>>>>> I don't think it's fragile. We are profiling via a module's
>>>>>> tracepoint, so we should pin the module, via module_get().
>>>>>> If event->profile_enable() has been calld, we should make
>>>>>> sure it's profile_disable() will be called.
>>>>> What I call fragile is that everyone registering a tracepoint 
>>>>> callback will now apparently need to worry about modules, _that_ 
>>>>> is fragile.
>>>>>
>>>>> Either make module unload look at tracepoint users, or place the 
>>>>> try_get_module() in the registration hooks so that regular users 
>>>>> don't need to worry about it.
>>>> The bug found by Li needs to be fixed obviously.
>>>>
>>>> I tend to agree with you that this does not appear to be the best 
>>>> place to do it: so you suggest to implicitly increase the module 
>>>> refcount on callback registr instead? (and releasing it when 
>>>> unregistering)
>>>>
>>>> Same end result, slightly cleaner place to bump the refcount.
>>> Yes, because the user of tracepoints should never need to care about
>>> modules.
>>>
>> I'm afraid it is not feasible to bump module refcnt implicitly
>> in tracepoint_probe_register().
>>
>> If a tracepoint is registered in module_init, and unregistered
>> in module_exit (see sample/tracepoints), the module is unloadable:
>>
>>  insmod
>>  ->call mod->init()
>>    ->trace_reg_foo()
>>      ->module_get()
>>
>>  rmmod
>>  ->check mod refcnt
>>  ->call mod->exit()
>>    ->trace_unreg_foo()
>>      ->module_put()
> 
> Not tracepoint_probe_{un,}register(), in {un,}register_trace_$call().
> 

Is there any difference?

	static inline int register_trace_##name(void (*probe)(proto))	\
	{								\
		int ret;						\
		void (*func)(void) = reg;				\
									\
		ret = tracepoint_probe_register(#name, (void *)probe);	\
		if (func && !ret)					\
			func();						\
		return ret;						\
	}

> Basically avoid module unload when a tracepoint from that module has
> registered callbacks.

TRACE_EVENT() won't prevent this. Instead at module unload, a module
notifier callback will be called to unregistread those tracepoint callbacks.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 14:33 +0800, Li Zefan wrote:
> >>>>>>>> If the correspoding module is unloaded before ftrace_profile_disable()
> >>>>>>>> is called, event->profile_disable() won't be called, which can
> >>>>>>>> cause oops:
> >>>>>>>>
> >>>>>>>>   # insmod trace-events-sample.ko
> >>>>>>>>   # perf record -f -a -e sample:foo_bar sleep 3 &
> >>>>>>>>   # sleep 1
> >>>>>>>>   # rmmod trace_events_sample
> >>>>>>>>   # insmod trace-events-sample.ko
> >>>>>>>>   OOPS!
> >>>>>>>>
> >>>>>>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> >>>>>>> Hrmm, feel fragile, why don't we check if all a modules tracepoints are
> >>>>>>> unused on unload?
> >>>>>>>
> >>>>>> I don't think it's fragile. We are profiling via a module's
> >>>>>> tracepoint, so we should pin the module, via module_get().
> >>>>>> If event->profile_enable() has been calld, we should make
> >>>>>> sure it's profile_disable() will be called.
> >>>>> What I call fragile is that everyone registering a tracepoint 
> >>>>> callback will now apparently need to worry about modules, _that_ 
> >>>>> is fragile.
> >>>>>
> >>>>> Either make module unload look at tracepoint users, or place the 
> >>>>> try_get_module() in the registration hooks so that regular users 
> >>>>> don't need to worry about it.
> >>>> The bug found by Li needs to be fixed obviously.
> >>>>
> >>>> I tend to agree with you that this does not appear to be the best 
> >>>> place to do it: so you suggest to implicitly increase the module 
> >>>> refcount on callback registr instead? (and releasing it when 
> >>>> unregistering)
> >>>>
> >>>> Same end result, slightly cleaner place to bump the refcount.
> >>> Yes, because the user of tracepoints should never need to care about
> >>> modules.
> >>>
> >> I'm afraid it is not feasible to bump module refcnt implicitly
> >> in tracepoint_probe_register().
> >>
> >> If a tracepoint is registered in module_init, and unregistered
> >> in module_exit (see sample/tracepoints), the module is unloadable:
> >>
> >>  insmod
> >>  ->call mod->init()
> >>    ->trace_reg_foo()
> >>      ->module_get()
> >>
> >>  rmmod
> >>  ->check mod refcnt
> >>  ->call mod->exit()
> >>    ->trace_unreg_foo()
> >>      ->module_put()
> > 
> > Not tracepoint_probe_{un,}register(), in {un,}register_trace_$call().
> > 
> 
> Is there any difference?
> 
> 	static inline int register_trace_##name(void (*probe)(proto))	\
> 	{								\
> 		int ret;						\
> 		void (*func)(void) = reg;				\
> 									\
> 		ret = tracepoint_probe_register(#name, (void *)probe);	\
> 		if (func && !ret)					\
> 			func();						\
> 		return ret;						\
> 	}

Ah, my bad, I was thikning tracepoint_probe_register() was the thing
that registered the tracepoint itself, not the callback.

Ok, then what's the problem?, don't do modules that consume their own
tracepoints, seems simple enough.

> > Basically avoid module unload when a tracepoint from that module has
> > registered callbacks.
> 
> TRACE_EVENT() won't prevent this. Instead at module unload, a module
> notifier callback will be called to unregistread those tracepoint callbacks.

Ugh, that's disgusting. That means every single tracepoint user again
needs to be aware of modules.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Ah, my bad, I was thikning tracepoint_probe_register() was the 
> thing that registered the tracepoint itself, not the callback.
> 
> Ok, then what's the problem?, don't do modules that consume their 
> own tracepoints, seems simple enough.

is this a reasonable restriction? I dont see any reason why the act 
of defining and providing a tracepoint should be exclusive of the 
ability to make use of it.

	Ingo

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 11:05 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Ah, my bad, I was thikning tracepoint_probe_register() was the 
> > thing that registered the tracepoint itself, not the callback.
> > 
> > Ok, then what's the problem?, don't do modules that consume their 
> > own tracepoints, seems simple enough.
> 
> is this a reasonable restriction? I dont see any reason why the act 
> of defining and providing a tracepoint should be exclusive of the 
> ability to make use of it.

It doesn't make sense to me, you don't need your own tracepoints because
you generate the events yourself, you already have them.

Furthermore, I much prefer this over spreading module gunk all over
tracepoint users.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, 2009-08-25 at 11:05 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Ah, my bad, I was thikning tracepoint_probe_register() was the 
> > > thing that registered the tracepoint itself, not the callback.
> > > 
> > > Ok, then what's the problem?, don't do modules that consume their 
> > > own tracepoints, seems simple enough.
> > 
> > is this a reasonable restriction? I dont see any reason why the 
> > act of defining and providing a tracepoint should be exclusive 
> > of the ability to make use of it.
> 
> It doesn't make sense to me, you don't need your own tracepoints 
> because you generate the events yourself, you already have them.

For a reasonable large subsystem/driver i can very well imagine this 
to happen: why should the subsystem add _another_ layer of callbacks 
if it can reuse the generic tracepoint code and register itself to 
those?

	Ingo

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 12:22 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, 2009-08-25 at 11:05 +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > Ah, my bad, I was thikning tracepoint_probe_register() was the 
> > > > thing that registered the tracepoint itself, not the callback.
> > > > 
> > > > Ok, then what's the problem?, don't do modules that consume their 
> > > > own tracepoints, seems simple enough.
> > > 
> > > is this a reasonable restriction? I dont see any reason why the 
> > > act of defining and providing a tracepoint should be exclusive 
> > > of the ability to make use of it.
> > 
> > It doesn't make sense to me, you don't need your own tracepoints 
> > because you generate the events yourself, you already have them.
> 
> For a reasonable large subsystem/driver i can very well imagine this 
> to happen: why should the subsystem add _another_ layer of callbacks 
> if it can reuse the generic tracepoint code and register itself to 
> those?

Then that subsystem would be non functioning when the kernel is build
without tracepoints.

Nothing should rely on tracepoint being present, they are and should
remain a debug feature, not a core hook thing.

Do you really wish to burden every tracepoint user with the extra logic
needed to deal with modules?

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, 2009-08-25 at 12:22 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Tue, 2009-08-25 at 11:05 +0200, Ingo Molnar wrote:
> > > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > > 
> > > > > Ah, my bad, I was thikning tracepoint_probe_register() was the 
> > > > > thing that registered the tracepoint itself, not the callback.
> > > > > 
> > > > > Ok, then what's the problem?, don't do modules that consume their 
> > > > > own tracepoints, seems simple enough.
> > > > 
> > > > is this a reasonable restriction? I dont see any reason why the 
> > > > act of defining and providing a tracepoint should be exclusive 
> > > > of the ability to make use of it.
> > > 
> > > It doesn't make sense to me, you don't need your own tracepoints 
> > > because you generate the events yourself, you already have them.
> > 
> > For a reasonable large subsystem/driver i can very well imagine 
> > this to happen: why should the subsystem add _another_ layer of 
> > callbacks if it can reuse the generic tracepoint code and 
> > register itself to those?
> 
> Then that subsystem would be non functioning when the kernel is 
> build without tracepoints.

... except if it selects all required core kernel functionality it 
relies on? Tracepoints and typed callbacks really go hand in hand 
and it would be nice to see some more synergy in that space. 
(instead of crappy, prioritized notifier chains for example.)

> Nothing should rely on tracepoint being present, they are and 
> should remain a debug feature, not a core hook thing.
> 
> Do you really wish to burden every tracepoint user with the extra 
> logic needed to deal with modules?

Not necessarily - i'm just outlining why i think that the 'dont 
allow subsystems to utilize tracepoint callbacks' is a restriction 
we should not live with voluntarily.

	Ingo

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 12:39 +0200, Ingo Molnar wrote:

> > Do you really wish to burden every tracepoint user with the extra 
> > logic needed to deal with modules?
> 
> Not necessarily - i'm just outlining why i think that the 'dont 
> allow subsystems to utilize tracepoint callbacks' is a restriction 
> we should not live with voluntarily.

Well, unless someone has a bright idea that's what it comes down to. And
not having to care about modules when using tracepoint wins hands down
for me.

The issue seems rather simple:

Either we force everybody who uses a tracepoint to care about modules,
be this by having to do try_get_module() themselves or by having to
listen to some notifier and have their callback forcibly dropped on
unload -- both suck IMO, suck very hard indeed.

Or by having modules that use their own tracepoint be stuck, because
once you block unlock when a tracepoint has callbacks, and it installed
a callback on itself, its not going to go away.

And since I don't care about modules at all and really wish they'd never
been invented...

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Tue, 2009-08-25 at 12:47 +0200, Peter Zijlstra wrote:
> On Tue, 2009-08-25 at 12:39 +0200, Ingo Molnar wrote:
> 
> > > Do you really wish to burden every tracepoint user with the extra 
> > > logic needed to deal with modules?
> > 
> > Not necessarily - i'm just outlining why i think that the 'dont 
> > allow subsystems to utilize tracepoint callbacks' is a restriction 
> > we should not live with voluntarily.
> 
> Well, unless someone has a bright idea that's what it comes down to. 

OK, I still think modules probing their own tracepoints its stupid [*],
but what you could do is iterate the tracepoint's callback list and see
if it has a callback outside of the module code section and then fail
the unload.

[*] in the really utterly fundamentally wrong stupid class.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

CC: Mathieu

Peter Zijlstra wrote:
> On Tue, 2009-08-25 at 12:47 +0200, Peter Zijlstra wrote:
>> On Tue, 2009-08-25 at 12:39 +0200, Ingo Molnar wrote:
>>
>>>> Do you really wish to burden every tracepoint user with the extra 
>>>> logic needed to deal with modules?
>>> Not necessarily - i'm just outlining why i think that the 'dont 
>>> allow subsystems to utilize tracepoint callbacks' is a restriction 
>>> we should not live with voluntarily.
>> Well, unless someone has a bright idea that's what it comes down to. 
> 
> OK, I still think modules probing their own tracepoints its stupid [*],
> but what you could do is iterate the tracepoint's callback list and see
> if it has a callback outside of the module code section and then fail
> the unload.
> 

I don't think this can work, what if a new callback is registered after
this check? Seems there's no syncronization to guarantee the result of
the check can remain valid during module unload.

And the other proposal that bumping module refcnt in register_tracepoint_xxx(),
I found it hard to do so, because register() won't know where the
tracepoint is. For example:

   register_tracepoint_foo() is in module bar, while
   DEFINE_TRACE(foo) and trace_foo() is in module foo.

Note, you're allowed to load bar without foo.

The simplest fix is the patch I sent, which you don't like. Maybe
Mathieu, the auther of tracepoint, or some one else has some idea?

> [*] in the really utterly fundamentally wrong stupid class.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 14:18 +0800, Li Zefan wrote:
> CC: Mathieu
> 
> Peter Zijlstra wrote:
> > On Tue, 2009-08-25 at 12:47 +0200, Peter Zijlstra wrote:
> >> On Tue, 2009-08-25 at 12:39 +0200, Ingo Molnar wrote:
> >>
> >>>> Do you really wish to burden every tracepoint user with the extra 
> >>>> logic needed to deal with modules?
> >>> Not necessarily - i'm just outlining why i think that the 'dont 
> >>> allow subsystems to utilize tracepoint callbacks' is a restriction 
> >>> we should not live with voluntarily.
> >> Well, unless someone has a bright idea that's what it comes down to. 
> > 
> > OK, I still think modules probing their own tracepoints its stupid [*],
> > but what you could do is iterate the tracepoint's callback list and see
> > if it has a callback outside of the module code section and then fail
> > the unload.
> > 
> 
> I don't think this can work, what if a new callback is registered after
> this check? Seems there's no syncronization to guarantee the result of
> the check can remain valid during module unload.

If that race can be solved using refcounts it can be solved another way
too.

> And the other proposal that bumping module refcnt in register_tracepoint_xxx(),
> I found it hard to do so, because register() won't know where the
> tracepoint is. For example:
> 
>    register_tracepoint_foo() is in module bar, while
>    DEFINE_TRACE(foo) and trace_foo() is in module foo.

You have that call->mod pointer, right? Once you're in the register bit
you should have access to the owning module, which can tell you if the
to be registered callback is part of that same module.

> Note, you're allowed to load bar without foo.
> 
> The simplest fix is the patch I sent, which you don't like. Maybe
> Mathieu, the auther of tracepoint, or some one else has some idea?

Yes, I think forcing every tracepoint consumer to know about modules is
insane.

Aahh, I see the bug, its only ftrace that knows about the module, not
tracepoints themselves, _that_ needs fixing.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:

> Aahh, I see the bug, its only ftrace that knows about the module, not
> tracepoints themselves, _that_ needs fixing.

These are the bits I hacked up until I figured out the above bug.

---
 include/linux/ftrace_event.h |    2 +-
 include/linux/module.h       |   18 ++++++++++++++----
 kernel/module.c              |    9 +--------
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index df5b085..d86d60c 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -122,7 +122,7 @@ struct ftrace_event_call {
 	struct list_head	fields;
 	int			filter_active;
 	struct event_filter	*filter;
-	void			*mod;
+	struct module		*mod;
 	void			*data;
 
 	atomic_t		profile_count;
diff --git a/include/linux/module.h b/include/linux/module.h
index 86863cd..09e2b88 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -409,16 +409,26 @@ struct module *__module_address(unsigned long addr);
 bool is_module_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
 
+static inline 
+int __within_module(unsigned long addr, void *start, unsigned long size)
+{
+	return ((void *)addr >= start && (void *)addr < start + size);
+}
+
 static inline int within_module_core(unsigned long addr, struct module *mod)
 {
-	return (unsigned long)mod->module_core <= addr &&
-	       addr < (unsigned long)mod->module_core + mod->core_size;
+	return __within_module(addr, mod->module_core, mod->core_size);
 }
 
 static inline int within_module_init(unsigned long addr, struct module *mod)
 {
-	return (unsigned long)mod->module_init <= addr &&
-	       addr < (unsigned long)mod->module_init + mod->init_size;
+	return __within_module(addr, mod->module_init, mod->init_size);
+}
+
+static inline int within_module_text(unsigned long addr, struct module *mod)
+{
+	return __within_module(addr, mod->module_init, mod->init_text_size) ||
+	       __within_module(addr, mod->core_init, mod->core_text_size);
 }
 
 /* Search for module by name: must hold module_mutex. */
diff --git a/kernel/module.c b/kernel/module.c
index b182143..822adb6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2493,11 +2493,6 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found
@@ -2912,9 +2907,7 @@ struct module *__module_text_address(unsigned long addr)
 {
 	struct module *mod = __module_address(addr);
 	if (mod) {
-		/* Make sure it's within the text section. */
-		if (!within(addr, mod->module_init, mod->init_text_size)
-		    && !within(addr, mod->module_core, mod->core_text_size))
+		if (!within_module_text(addr, mod))
 			mod = NULL;
 	}
 	return mod;

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:

> Aahh, I see the bug, its only ftrace that knows about the module, not
> tracepoints themselves, _that_ needs fixing.

You could possibly do something like:

 struct module *tp_mod = __module_address(&some_tp_symbol);
 struct module *cb_mod = __module_text_address(func);

 if (tp_mod && tp_mod != cb_mod) {
	ret = try_get_module(tp_mod);
	if (ret)
		goto fail;
 }

in register_trace_##name() or thereabout.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

Peter Zijlstra wrote:
> On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
> 
>> Aahh, I see the bug, its only ftrace that knows about the module, not
>> tracepoints themselves, _that_ needs fixing.
> 
> You could possibly do something like:
> 
>  struct module *tp_mod = __module_address(&some_tp_symbol);
>  struct module *cb_mod = __module_text_address(func);
> 
>  if (tp_mod && tp_mod != cb_mod) {
> 	ret = try_get_module(tp_mod);
> 	if (ret)
> 		goto fail;
>  }
> 
> in register_trace_##name() or thereabout.
> 

Actually I tried it, but it didn't work. As I said, You can't find
any tp symbol when registering tp callback. The same example again:

	In module bar, we have register_trace_foo()
	In module foo, we have DEFINE_TRACE(foo) and trace_foo().

bar doesn't know any symbol of foo, so it can't bump foo's refcnt,

*Note: you can load module bar without loading module foo*

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 15:10 +0800, Li Zefan wrote:
> Peter Zijlstra wrote:
> > On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
> > 
> >> Aahh, I see the bug, its only ftrace that knows about the module, not
> >> tracepoints themselves, _that_ needs fixing.
> > 
> > You could possibly do something like:
> > 
> >  struct module *tp_mod = __module_address(&some_tp_symbol);
> >  struct module *cb_mod = __module_text_address(func);
> > 
> >  if (tp_mod && tp_mod != cb_mod) {
> > 	ret = try_get_module(tp_mod);
> > 	if (ret)
> > 		goto fail;
> >  }
> > 
> > in register_trace_##name() or thereabout.
> > 
> 
> Actually I tried it, but it didn't work. As I said, You can't find
> any tp symbol when registering tp callback. The same example again:
> 
> 	In module bar, we have register_trace_foo()
> 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
> 
> bar doesn't know any symbol of foo, so it can't bump foo's refcnt,

Well, clearly it knows about register_trace_foo() which itself knows at
least one symbol that should be in module foo, right? How else could it
register a callback in that module (if it were loaded)?

It appears to use some intermediate code, in which case the intermediate
code knows about foo, which too solves our problem.

> *Note: you can load module bar without loading module foo*

In which case the tracepoint registration fails, right?

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

15:26, Peter Zijlstra wrote:
> On Wed, 2009-08-26 at 15:10 +0800, Li Zefan wrote:
>> Peter Zijlstra wrote:
>>> On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
>>>
>>>> Aahh, I see the bug, its only ftrace that knows about the module, not
>>>> tracepoints themselves, _that_ needs fixing.
>>> You could possibly do something like:
>>>
>>>  struct module *tp_mod = __module_address(&some_tp_symbol);
>>>  struct module *cb_mod = __module_text_address(func);
>>>
>>>  if (tp_mod && tp_mod != cb_mod) {
>>> 	ret = try_get_module(tp_mod);
>>> 	if (ret)
>>> 		goto fail;
>>>  }
>>>
>>> in register_trace_##name() or thereabout.
>>>
>> Actually I tried it, but it didn't work. As I said, You can't find
>> any tp symbol when registering tp callback. The same example again:
>>
>> 	In module bar, we have register_trace_foo()
>> 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
>>
>> bar doesn't know any symbol of foo, so it can't bump foo's refcnt,
> 
> Well, clearly it knows about register_trace_foo() which itself knows at
> least one symbol that should be in module foo, right? How else could it
> register a callback in that module (if it were loaded)?
> 
> It appears to use some intermediate code, in which case the intermediate
> code knows about foo, which too solves our problem.
> 
>> *Note: you can load module bar without loading module foo*
> 
> In which case the tracepoint registration fails, right?
> 

No, it won't fail. ;)

Instead, when foo is loaded, tracepoint_update_probe_range() will be
called, and the probe registered in bar will be added to the tracepoint.

Maybe we can do something in tracepoint_update_probe_range(). I'll try.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 15:31 +0800, Li Zefan wrote:
> 15:26, Peter Zijlstra wrote:
> > On Wed, 2009-08-26 at 15:10 +0800, Li Zefan wrote:
> >> Peter Zijlstra wrote:
> >>> On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
> >>>
> >>>> Aahh, I see the bug, its only ftrace that knows about the module, not
> >>>> tracepoints themselves, _that_ needs fixing.
> >>> You could possibly do something like:
> >>>
> >>>  struct module *tp_mod = __module_address(&some_tp_symbol);
> >>>  struct module *cb_mod = __module_text_address(func);
> >>>
> >>>  if (tp_mod && tp_mod != cb_mod) {
> >>> 	ret = try_get_module(tp_mod);
> >>> 	if (ret)
> >>> 		goto fail;
> >>>  }
> >>>
> >>> in register_trace_##name() or thereabout.
> >>>
> >> Actually I tried it, but it didn't work. As I said, You can't find
> >> any tp symbol when registering tp callback. The same example again:
> >>
> >> 	In module bar, we have register_trace_foo()
> >> 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
> >>
> >> bar doesn't know any symbol of foo, so it can't bump foo's refcnt,
> > 
> > Well, clearly it knows about register_trace_foo() which itself knows at
> > least one symbol that should be in module foo, right? How else could it
> > register a callback in that module (if it were loaded)?
> > 
> > It appears to use some intermediate code, in which case the intermediate
> > code knows about foo, which too solves our problem.
> > 
> >> *Note: you can load module bar without loading module foo*
> > 
> > In which case the tracepoint registration fails, right?
> > 
> 
> No, it won't fail. ;)
> 
> Instead, when foo is loaded, tracepoint_update_probe_range() will be
> called, and the probe registered in bar will be added to the tracepoint.

*blink*

so we'll succeed in registering a tracepoint we know isn't there?

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

>>>>>> Aahh, I see the bug, its only ftrace that knows about the module, not
>>>>>> tracepoints themselves, _that_ needs fixing.
>>>>> You could possibly do something like:
>>>>>
>>>>>  struct module *tp_mod = __module_address(&some_tp_symbol);
>>>>>  struct module *cb_mod = __module_text_address(func);
>>>>>
>>>>>  if (tp_mod && tp_mod != cb_mod) {
>>>>> 	ret = try_get_module(tp_mod);
>>>>> 	if (ret)
>>>>> 		goto fail;
>>>>>  }
>>>>>
>>>>> in register_trace_##name() or thereabout.
>>>>>
>>>> Actually I tried it, but it didn't work. As I said, You can't find
>>>> any tp symbol when registering tp callback. The same example again:
>>>>
>>>> 	In module bar, we have register_trace_foo()
>>>> 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
>>>>
>>>> bar doesn't know any symbol of foo, so it can't bump foo's refcnt,
>>> Well, clearly it knows about register_trace_foo() which itself knows at
>>> least one symbol that should be in module foo, right? How else could it
>>> register a callback in that module (if it were loaded)?
>>>
>>> It appears to use some intermediate code, in which case the intermediate
>>> code knows about foo, which too solves our problem.
>>>
>>>> *Note: you can load module bar without loading module foo*
>>> In which case the tracepoint registration fails, right?
>>>
>> No, it won't fail. ;)
>>
>> Instead, when foo is loaded, tracepoint_update_probe_range() will be
>> called, and the probe registered in bar will be added to the tracepoint.
> 
> *blink*
> 
> so we'll succeed in registering a tracepoint we know isn't there?
> 

Yeah, try this:

# cd sample/tracepoints/
# insmod tracepoint-probe-sample.ko	<--- load trace probe firstly
# insmod tracepoint-sample.ko           <--- load tracepoint secondly
# cat /proc/tracepoint-sample
cat: /proc/tracepoint-sample: Operation not permitted	<-- pls ignore this
# dmesg							<-- see the output of probe
Event is encountered with filename tracepoint-sample
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered
Event B is encountered

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Wed, 26 Aug 2009, Li Zefan wrote:

> Peter Zijlstra wrote:
> > On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
> > 
> >> Aahh, I see the bug, its only ftrace that knows about the module, not
> >> tracepoints themselves, _that_ needs fixing.
> > 
> > You could possibly do something like:
> > 
> >  struct module *tp_mod = __module_address(&some_tp_symbol);
> >  struct module *cb_mod = __module_text_address(func);
> > 
> >  if (tp_mod && tp_mod != cb_mod) {
> > 	ret = try_get_module(tp_mod);
> > 	if (ret)
> > 		goto fail;
> >  }
> > 
> > in register_trace_##name() or thereabout.
> > 
> 
> Actually I tried it, but it didn't work. As I said, You can't find
> any tp symbol when registering tp callback. The same example again:
> 
> 	In module bar, we have register_trace_foo()
> 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
> 
> bar doesn't know any symbol of foo, so it can't bump foo's refcnt,
> 
> *Note: you can load module bar without loading module foo*

WTF!!!!

We can register a trace point that is defined in another module without 
having that module?? How is that possible? That looks totally busted, and 
that is not a case that I think we need to worry about, except to prevent 
it from ever happening.

As for ref counts, would something like this work?

 (untested)

-- Steve

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 0341f2e..055275b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -109,8 +109,9 @@ struct tracepoint {
 #define EXPORT_TRACEPOINT_SYMBOL(name)					\
 	EXPORT_SYMBOL(__tracepoint_##name)
 
-extern void tracepoint_update_probe_range(struct tracepoint *begin,
-	struct tracepoint *end);
+extern void tracepoint_update_probe_range(struct module *,
+					  struct tracepoint *begin,
+					  struct tracepoint *end);
 
 #else /* !CONFIG_TRACEPOINTS */
 #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg)	\
diff --git a/kernel/module.c b/kernel/module.c
index b182143..a8e69fa 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2974,7 +2974,7 @@ void module_update_tracepoints(void)
 	mutex_lock(&module_mutex);
 	list_for_each_entry(mod, &modules, list)
 		if (!mod->taints)
-			tracepoint_update_probe_range(mod->tracepoints,
+			tracepoint_update_probe_range(mod, mod->tracepoints,
 				mod->tracepoints + mod->num_tracepoints);
 	mutex_unlock(&module_mutex);
 }
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 06f165a..b150255 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -274,7 +274,8 @@ static void disable_tracepoint(struct tracepoint *elem)
  * Updates the probe callback corresponding to a range of tracepoints.
  */
 void
-tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
+tracepoint_update_probe_range(struct module *mod,
+			      struct tracepoint *begin, struct tracepoint *end)
 {
 	struct tracepoint *iter;
 	struct tracepoint_entry *mark_entry;
@@ -286,9 +287,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 	for (iter = begin; iter < end; iter++) {
 		mark_entry = get_tracepoint(iter->name);
 		if (mark_entry) {
+			if (mod) {
+				if (!try_module_get(mod))
+					continue;
+			}
 			set_tracepoint(&mark_entry, iter,
 					!!mark_entry->refcount);
 		} else {
+			if (mod)
+				module_put(mod);
 			disable_tracepoint(iter);
 		}
 	}
@@ -301,7 +308,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 static void tracepoint_update_probes(void)
 {
 	/* Core kernel tracepoints */
-	tracepoint_update_probe_range(__start___tracepoints,
+	tracepoint_update_probe_range(NULL, __start___tracepoints,
 		__stop___tracepoints);
 	/* tracepoints in modules. */
 	module_update_tracepoints();
@@ -556,7 +563,7 @@ int tracepoint_module_notify(struct notifier_block *self,
 	switch (val) {
 	case MODULE_STATE_COMING:
 	case MODULE_STATE_GOING:
-		tracepoint_update_probe_range(mod->tracepoints,
+		tracepoint_update_probe_range(mod, mod->tracepoints,
 			mod->tracepoints + mod->num_tracepoints);
 		break;
 	}

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Wed, 26 Aug 2009, Li Zefan wrote:
> 
> > Peter Zijlstra wrote:
> > > On Wed, 2009-08-26 at 08:46 +0200, Peter Zijlstra wrote:
> > > 
> > >> Aahh, I see the bug, its only ftrace that knows about the module, not
> > >> tracepoints themselves, _that_ needs fixing.
> > > 
> > > You could possibly do something like:
> > > 
> > >  struct module *tp_mod = __module_address(&some_tp_symbol);
> > >  struct module *cb_mod = __module_text_address(func);
> > > 
> > >  if (tp_mod && tp_mod != cb_mod) {
> > > 	ret = try_get_module(tp_mod);
> > > 	if (ret)
> > > 		goto fail;
> > >  }
> > > 
> > > in register_trace_##name() or thereabout.
> > > 
> > 
> > Actually I tried it, but it didn't work. As I said, You can't find
> > any tp symbol when registering tp callback. The same example again:
> > 
> > 	In module bar, we have register_trace_foo()
> > 	In module foo, we have DEFINE_TRACE(foo) and trace_foo().
> > 
> > bar doesn't know any symbol of foo, so it can't bump foo's refcnt,
> > 
> > *Note: you can load module bar without loading module foo*
> 
> WTF!!!!
> 
> We can register a trace point that is defined in another module without 
> having that module?? How is that possible? That looks totally busted, and 
> that is not a case that I think we need to worry about, except to prevent 
> it from ever happening.
> 

Registering tracepoints even when no tracepoint definition is currently
visible is the intended allowed behavior. Let's say we need to trace
something happening in module init: if we disallow registering the tp
callback before the module is initialized, we run in a chicken and egg
problem.

So I am trying to figure out the problem source there. Is it that
modules containing the tp callbacks need to know if those are actually
connected to an instrumented module ? Or is it that the instrumented
module needs to know if a probe module is connected to is ? Or is it the
teardown of the probe module ? No refcount is needed there, because we
surround the probe call by preempt disable/enable, and we use
synchronize_sched() before removing the module which contains probe
callbacks.

Mathieu-trying-to-figure-out-what-this-whole-thread-is-about :)


> As for ref counts, would something like this work?
> 
>  (untested)
> 
> -- Steve
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 0341f2e..055275b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -109,8 +109,9 @@ struct tracepoint {
>  #define EXPORT_TRACEPOINT_SYMBOL(name)					\
>  	EXPORT_SYMBOL(__tracepoint_##name)
>  
> -extern void tracepoint_update_probe_range(struct tracepoint *begin,
> -	struct tracepoint *end);
> +extern void tracepoint_update_probe_range(struct module *,
> +					  struct tracepoint *begin,
> +					  struct tracepoint *end);
>  
>  #else /* !CONFIG_TRACEPOINTS */
>  #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg)	\
> diff --git a/kernel/module.c b/kernel/module.c
> index b182143..a8e69fa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2974,7 +2974,7 @@ void module_update_tracepoints(void)
>  	mutex_lock(&module_mutex);
>  	list_for_each_entry(mod, &modules, list)
>  		if (!mod->taints)
> -			tracepoint_update_probe_range(mod->tracepoints,
> +			tracepoint_update_probe_range(mod, mod->tracepoints,
>  				mod->tracepoints + mod->num_tracepoints);
>  	mutex_unlock(&module_mutex);
>  }
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 06f165a..b150255 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -274,7 +274,8 @@ static void disable_tracepoint(struct tracepoint *elem)
>   * Updates the probe callback corresponding to a range of tracepoints.
>   */
>  void
> -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> +tracepoint_update_probe_range(struct module *mod,
> +			      struct tracepoint *begin, struct tracepoint *end)
>  {
>  	struct tracepoint *iter;
>  	struct tracepoint_entry *mark_entry;
> @@ -286,9 +287,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  	for (iter = begin; iter < end; iter++) {
>  		mark_entry = get_tracepoint(iter->name);
>  		if (mark_entry) {
> +			if (mod) {
> +				if (!try_module_get(mod))
> +					continue;
> +			}
>  			set_tracepoint(&mark_entry, iter,
>  					!!mark_entry->refcount);
>  		} else {
> +			if (mod)
> +				module_put(mod);
>  			disable_tracepoint(iter);
>  		}
>  	}
> @@ -301,7 +308,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  static void tracepoint_update_probes(void)
>  {
>  	/* Core kernel tracepoints */
> -	tracepoint_update_probe_range(__start___tracepoints,
> +	tracepoint_update_probe_range(NULL, __start___tracepoints,
>  		__stop___tracepoints);
>  	/* tracepoints in modules. */
>  	module_update_tracepoints();
> @@ -556,7 +563,7 @@ int tracepoint_module_notify(struct notifier_block *self,
>  	switch (val) {
>  	case MODULE_STATE_COMING:
>  	case MODULE_STATE_GOING:
> -		tracepoint_update_probe_range(mod->tracepoints,
> +		tracepoint_update_probe_range(mod, mod->tracepoints,
>  			mod->tracepoints + mod->num_tracepoints);
>  		break;
>  	}

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

This patch solves the problem that Li originally reported. If something 
registers a trace point belonging to a module, then it ups the ref count 
of the module. This prevents a process from registering a probe to a 
tracepoint belonging to a module and then having the module disappear.

Doing the example with perf in Li's original post, now errors on the 
rmmod, with "ERROR: Module trace_events_sample is in use".

Mathieu, can I have your acked-by on this?

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 0341f2e..055275b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -109,8 +109,9 @@ struct tracepoint {
 #define EXPORT_TRACEPOINT_SYMBOL(name)					\
 	EXPORT_SYMBOL(__tracepoint_##name)
 
-extern void tracepoint_update_probe_range(struct tracepoint *begin,
-	struct tracepoint *end);
+extern void tracepoint_update_probe_range(struct module *,
+					  struct tracepoint *begin,
+					  struct tracepoint *end);
 
 #else /* !CONFIG_TRACEPOINTS */
 #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg)	\
diff --git a/kernel/module.c b/kernel/module.c
index b182143..a8e69fa 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2974,7 +2974,7 @@ void module_update_tracepoints(void)
 	mutex_lock(&module_mutex);
 	list_for_each_entry(mod, &modules, list)
 		if (!mod->taints)
-			tracepoint_update_probe_range(mod->tracepoints,
+			tracepoint_update_probe_range(mod, mod->tracepoints,
 				mod->tracepoints + mod->num_tracepoints);
 	mutex_unlock(&module_mutex);
 }
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 06f165a..089e6f9 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -54,6 +54,7 @@ static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
  */
 struct tracepoint_entry {
 	struct hlist_node hlist;
+	struct module *mod;
 	void **funcs;
 	int refcount;	/* Number of times armed. 0 if disarmed. */
 	char name[0];
@@ -221,6 +222,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
 	memcpy(&e->name[0], name, name_len);
 	e->funcs = NULL;
 	e->refcount = 0;
+	e->mod = NULL;	/* Will be assigned in tracepoint_update_probe_range */
 	hlist_add_head(&e->hlist, head);
 	return e;
 }
@@ -231,6 +233,8 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
  */
 static inline void remove_tracepoint(struct tracepoint_entry *e)
 {
+	if (e->mod)
+		module_put(e->mod);
 	hlist_del(&e->hlist);
 	kfree(e);
 }
@@ -274,7 +278,8 @@ static void disable_tracepoint(struct tracepoint *elem)
  * Updates the probe callback corresponding to a range of tracepoints.
  */
 void
-tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
+tracepoint_update_probe_range(struct module *mod,
+			      struct tracepoint *begin, struct tracepoint *end)
 {
 	struct tracepoint *iter;
 	struct tracepoint_entry *mark_entry;
@@ -286,9 +291,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 	for (iter = begin; iter < end; iter++) {
 		mark_entry = get_tracepoint(iter->name);
 		if (mark_entry) {
+			if (mod && !mark_entry->mod) {
+				if (!try_module_get(mod))
+					goto disable;
+				mark_entry->mod = mod;
+			}
 			set_tracepoint(&mark_entry, iter,
 					!!mark_entry->refcount);
 		} else {
+ disable:
 			disable_tracepoint(iter);
 		}
 	}
@@ -301,7 +312,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 static void tracepoint_update_probes(void)
 {
 	/* Core kernel tracepoints */
-	tracepoint_update_probe_range(__start___tracepoints,
+	tracepoint_update_probe_range(NULL, __start___tracepoints,
 		__stop___tracepoints);
 	/* tracepoints in modules. */
 	module_update_tracepoints();
@@ -556,7 +567,7 @@ int tracepoint_module_notify(struct notifier_block *self,
 	switch (val) {
 	case MODULE_STATE_COMING:
 	case MODULE_STATE_GOING:
-		tracepoint_update_probe_range(mod->tracepoints,
+		tracepoint_update_probe_range(mod, mod->tracepoints,
 			mod->tracepoints + mod->num_tracepoints);
 		break;
 	}

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> This patch solves the problem that Li originally reported. If something 
> registers a trace point belonging to a module, then it ups the ref count 
> of the module. This prevents a process from registering a probe to a 
> tracepoint belonging to a module and then having the module disappear.
> 
> Doing the example with perf in Li's original post, now errors on the 
> rmmod, with "ERROR: Module trace_events_sample is in use".
> 
> Mathieu, can I have your acked-by on this?
> 

Sorry, it looks buggy.

It does not deal with the fact that tracepoints with the same name and
arguments can be present in more than one module, or in a combination of
kernel core and modules.

The struct tracepoint_entry is specific to a a tracepoint name, used for
registration, but is eventually tied to all tracepoint instrumentation
instances for this tracepoint name.

Mathieu

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 0341f2e..055275b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -109,8 +109,9 @@ struct tracepoint {
>  #define EXPORT_TRACEPOINT_SYMBOL(name)					\
>  	EXPORT_SYMBOL(__tracepoint_##name)
>  
> -extern void tracepoint_update_probe_range(struct tracepoint *begin,
> -	struct tracepoint *end);
> +extern void tracepoint_update_probe_range(struct module *,
> +					  struct tracepoint *begin,
> +					  struct tracepoint *end);
>  
>  #else /* !CONFIG_TRACEPOINTS */
>  #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg)	\
> diff --git a/kernel/module.c b/kernel/module.c
> index b182143..a8e69fa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2974,7 +2974,7 @@ void module_update_tracepoints(void)
>  	mutex_lock(&module_mutex);
>  	list_for_each_entry(mod, &modules, list)
>  		if (!mod->taints)
> -			tracepoint_update_probe_range(mod->tracepoints,
> +			tracepoint_update_probe_range(mod, mod->tracepoints,
>  				mod->tracepoints + mod->num_tracepoints);
>  	mutex_unlock(&module_mutex);
>  }
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 06f165a..089e6f9 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -54,6 +54,7 @@ static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
>   */
>  struct tracepoint_entry {
>  	struct hlist_node hlist;
> +	struct module *mod;
>  	void **funcs;
>  	int refcount;	/* Number of times armed. 0 if disarmed. */
>  	char name[0];
> @@ -221,6 +222,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
>  	memcpy(&e->name[0], name, name_len);
>  	e->funcs = NULL;
>  	e->refcount = 0;
> +	e->mod = NULL;	/* Will be assigned in tracepoint_update_probe_range */
>  	hlist_add_head(&e->hlist, head);
>  	return e;
>  }
> @@ -231,6 +233,8 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
>   */
>  static inline void remove_tracepoint(struct tracepoint_entry *e)
>  {
> +	if (e->mod)
> +		module_put(e->mod);
>  	hlist_del(&e->hlist);
>  	kfree(e);
>  }
> @@ -274,7 +278,8 @@ static void disable_tracepoint(struct tracepoint *elem)
>   * Updates the probe callback corresponding to a range of tracepoints.
>   */
>  void
> -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> +tracepoint_update_probe_range(struct module *mod,
> +			      struct tracepoint *begin, struct tracepoint *end)
>  {
>  	struct tracepoint *iter;
>  	struct tracepoint_entry *mark_entry;
> @@ -286,9 +291,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  	for (iter = begin; iter < end; iter++) {
>  		mark_entry = get_tracepoint(iter->name);
>  		if (mark_entry) {
> +			if (mod && !mark_entry->mod) {
> +				if (!try_module_get(mod))
> +					goto disable;
> +				mark_entry->mod = mod;
> +			}
>  			set_tracepoint(&mark_entry, iter,
>  					!!mark_entry->refcount);
>  		} else {
> + disable:
>  			disable_tracepoint(iter);
>  		}
>  	}
> @@ -301,7 +312,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  static void tracepoint_update_probes(void)
>  {
>  	/* Core kernel tracepoints */
> -	tracepoint_update_probe_range(__start___tracepoints,
> +	tracepoint_update_probe_range(NULL, __start___tracepoints,
>  		__stop___tracepoints);
>  	/* tracepoints in modules. */
>  	module_update_tracepoints();
> @@ -556,7 +567,7 @@ int tracepoint_module_notify(struct notifier_block *self,
>  	switch (val) {
>  	case MODULE_STATE_COMING:
>  	case MODULE_STATE_GOING:
> -		tracepoint_update_probe_range(mod->tracepoints,
> +		tracepoint_update_probe_range(mod, mod->tracepoints,
>  			mod->tracepoints + mod->num_tracepoints);
>  		break;
>  	}
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > 
> > This patch solves the problem that Li originally reported. If something 
> > registers a trace point belonging to a module, then it ups the ref count 
> > of the module. This prevents a process from registering a probe to a 
> > tracepoint belonging to a module and then having the module disappear.
> > 
> > Doing the example with perf in Li's original post, now errors on the 
> > rmmod, with "ERROR: Module trace_events_sample is in use".
> > 
> > Mathieu, can I have your acked-by on this?
> > 
> 
> Sorry, it looks buggy.
> 
> It does not deal with the fact that tracepoints with the same name and
> arguments can be present in more than one module, or in a combination of
> kernel core and modules.
> 
> The struct tracepoint_entry is specific to a a tracepoint name, used for
> registration, but is eventually tied to all tracepoint instrumentation
> instances for this tracepoint name.
> 

Looking at the original post:
http://lkml.org/lkml/2009/8/24/5

the problem seems to be caused by the fact that the
trace_event_profile.c keeps some knowledge of modules in internal data
structures, but does not get notified of module unloads. Why don't we
fix that instead ?

A quick glance at it seems to indicate that it lazily discovers new
modules when the tracepoints are hit. Using module load/unload notifiers
would be more appropriate. Or maybe adding a notifier call to
tracepoint.c, calling notification callbacks for probe modules which
need to know when the connected tracepoints are changing
(when they are connected/disconnected) would probably be even more
appropriate. As a result, it would remove the dynamic verification cost
implied by lazy data structure lookup and check each time the probe is
fired.

Mathieu

> Mathieu
> 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 0341f2e..055275b 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -109,8 +109,9 @@ struct tracepoint {
> >  #define EXPORT_TRACEPOINT_SYMBOL(name)					\
> >  	EXPORT_SYMBOL(__tracepoint_##name)
> >  
> > -extern void tracepoint_update_probe_range(struct tracepoint *begin,
> > -	struct tracepoint *end);
> > +extern void tracepoint_update_probe_range(struct module *,
> > +					  struct tracepoint *begin,
> > +					  struct tracepoint *end);
> >  
> >  #else /* !CONFIG_TRACEPOINTS */
> >  #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg)	\
> > diff --git a/kernel/module.c b/kernel/module.c
> > index b182143..a8e69fa 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2974,7 +2974,7 @@ void module_update_tracepoints(void)
> >  	mutex_lock(&module_mutex);
> >  	list_for_each_entry(mod, &modules, list)
> >  		if (!mod->taints)
> > -			tracepoint_update_probe_range(mod->tracepoints,
> > +			tracepoint_update_probe_range(mod, mod->tracepoints,
> >  				mod->tracepoints + mod->num_tracepoints);
> >  	mutex_unlock(&module_mutex);
> >  }
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 06f165a..089e6f9 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -54,6 +54,7 @@ static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
> >   */
> >  struct tracepoint_entry {
> >  	struct hlist_node hlist;
> > +	struct module *mod;
> >  	void **funcs;
> >  	int refcount;	/* Number of times armed. 0 if disarmed. */
> >  	char name[0];
> > @@ -221,6 +222,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
> >  	memcpy(&e->name[0], name, name_len);
> >  	e->funcs = NULL;
> >  	e->refcount = 0;
> > +	e->mod = NULL;	/* Will be assigned in tracepoint_update_probe_range */
> >  	hlist_add_head(&e->hlist, head);
> >  	return e;
> >  }
> > @@ -231,6 +233,8 @@ static struct tracepoint_entry *add_tracepoint(const char *name)
> >   */
> >  static inline void remove_tracepoint(struct tracepoint_entry *e)
> >  {
> > +	if (e->mod)
> > +		module_put(e->mod);
> >  	hlist_del(&e->hlist);
> >  	kfree(e);
> >  }
> > @@ -274,7 +278,8 @@ static void disable_tracepoint(struct tracepoint *elem)
> >   * Updates the probe callback corresponding to a range of tracepoints.
> >   */
> >  void
> > -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> > +tracepoint_update_probe_range(struct module *mod,
> > +			      struct tracepoint *begin, struct tracepoint *end)
> >  {
> >  	struct tracepoint *iter;
> >  	struct tracepoint_entry *mark_entry;
> > @@ -286,9 +291,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> >  	for (iter = begin; iter < end; iter++) {
> >  		mark_entry = get_tracepoint(iter->name);
> >  		if (mark_entry) {
> > +			if (mod && !mark_entry->mod) {
> > +				if (!try_module_get(mod))
> > +					goto disable;
> > +				mark_entry->mod = mod;
> > +			}
> >  			set_tracepoint(&mark_entry, iter,
> >  					!!mark_entry->refcount);
> >  		} else {
> > + disable:
> >  			disable_tracepoint(iter);
> >  		}
> >  	}
> > @@ -301,7 +312,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> >  static void tracepoint_update_probes(void)
> >  {
> >  	/* Core kernel tracepoints */
> > -	tracepoint_update_probe_range(__start___tracepoints,
> > +	tracepoint_update_probe_range(NULL, __start___tracepoints,
> >  		__stop___tracepoints);
> >  	/* tracepoints in modules. */
> >  	module_update_tracepoints();
> > @@ -556,7 +567,7 @@ int tracepoint_module_notify(struct notifier_block *self,
> >  	switch (val) {
> >  	case MODULE_STATE_COMING:
> >  	case MODULE_STATE_GOING:
> > -		tracepoint_update_probe_range(mod->tracepoints,
> > +		tracepoint_update_probe_range(mod, mod->tracepoints,
> >  			mod->tracepoints + mod->num_tracepoints);
> >  		break;
> >  	}
> > 
> > 
> 
> -- 
> Mathieu Desnoyers
> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 12:46 -0400, Mathieu Desnoyers wrote:
> Registering tracepoints even when no tracepoint definition is currently
> visible is the intended allowed behavior. Let's say we need to trace
> something happening in module init: if we disallow registering the tp
> callback before the module is initialized, we run in a chicken and egg
> problem.
> 
> So I am trying to figure out the problem source there. Is it that
> modules containing the tp callbacks need to know if those are actually
> connected to an instrumented module ? Or is it that the instrumented
> module needs to know if a probe module is connected to is ? Or is it the
> teardown of the probe module ? No refcount is needed there, because we
> surround the probe call by preempt disable/enable, and we use
> synchronize_sched() before removing the module which contains probe
> callbacks.
> 
> Mathieu-trying-to-figure-out-what-this-whole-thread-is-about :)

OK, so the whole point seems to be that tracepoints have the funny thing
you describe above, whereas the things ftrace makes out of TRACE_EVENT()
get instantiated along with modules.

The reason why I rejected the initial patch (and I still think that that
fix is at the wrong layer) is that I, as a consumer of whatever
TRACE_EVENT() offers, should never need to consider modules.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Peter Zijlstra (peterz@infradead.org) wrote:
> On Wed, 2009-08-26 at 12:46 -0400, Mathieu Desnoyers wrote:
> > Registering tracepoints even when no tracepoint definition is currently
> > visible is the intended allowed behavior. Let's say we need to trace
> > something happening in module init: if we disallow registering the tp
> > callback before the module is initialized, we run in a chicken and egg
> > problem.
> > 
> > So I am trying to figure out the problem source there. Is it that
> > modules containing the tp callbacks need to know if those are actually
> > connected to an instrumented module ? Or is it that the instrumented
> > module needs to know if a probe module is connected to is ? Or is it the
> > teardown of the probe module ? No refcount is needed there, because we
> > surround the probe call by preempt disable/enable, and we use
> > synchronize_sched() before removing the module which contains probe
> > callbacks.
> > 
> > Mathieu-trying-to-figure-out-what-this-whole-thread-is-about :)
> 
> OK, so the whole point seems to be that tracepoints have the funny thing
> you describe above, whereas the things ftrace makes out of TRACE_EVENT()
> get instantiated along with modules.
> 
> The reason why I rejected the initial patch (and I still think that that
> fix is at the wrong layer) is that I, as a consumer of whatever
> TRACE_EVENT() offers, should never need to consider modules.
> 

Hrm, is it just me, or include/trace/ftrace.h fails to call the
following function after tracepoint unregistration ?

/*
 * tracepoint_synchronize_unregister must be called between the last tracepoint
 * probe unregistration and the end of module exit to make sure there is no
 * caller executing a probe when it is freed.
 */
static inline void tracepoint_synchronize_unregister(void)
{
        synchronize_sched();
}

This might solve all our problems.

Basically, it does not need to be called after each individual
tracepoint unregistration, but does need to be called before removal of
the module containing the probles (e.g. in module exit()).

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:

> * Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > 
> > > This patch solves the problem that Li originally reported. If something 
> > > registers a trace point belonging to a module, then it ups the ref count 
> > > of the module. This prevents a process from registering a probe to a 
> > > tracepoint belonging to a module and then having the module disappear.
> > > 
> > > Doing the example with perf in Li's original post, now errors on the 
> > > rmmod, with "ERROR: Module trace_events_sample is in use".
> > > 
> > > Mathieu, can I have your acked-by on this?
> > > 
> > 
> > Sorry, it looks buggy.

Well, it is not that buggy. It would only prevent modules from unloading 
if another tracepoint with the same name and parameters had a probe 
registered. More of a too big of a tent deal.

> > 
> > It does not deal with the fact that tracepoints with the same name and
> > arguments can be present in more than one module, or in a combination of
> > kernel core and modules.
> > 
> > The struct tracepoint_entry is specific to a a tracepoint name, used for
> > registration, but is eventually tied to all tracepoint instrumentation
> > instances for this tracepoint name.
> > 

Anyway, this prevents your tracepoints from doing the odd things of 
loading a probe before it exists. Well you can, but then you prevent the 
unload of the module that registered it. Fine, I chucked out that patch.

> 
> Looking at the original post:
> http://lkml.org/lkml/2009/8/24/5
> 
> the problem seems to be caused by the fact that the
> trace_event_profile.c keeps some knowledge of modules in internal data
> structures, but does not get notified of module unloads. Why don't we
> fix that instead ?
> 
> A quick glance at it seems to indicate that it lazily discovers new
> modules when the tracepoints are hit. Using module load/unload notifiers
> would be more appropriate. Or maybe adding a notifier call to
> tracepoint.c, calling notification callbacks for probe modules which
> need to know when the connected tracepoints are changing
> (when they are connected/disconnected) would probably be even more
> appropriate. As a result, it would remove the dynamic verification cost
> implied by lazy data structure lookup and check each time the probe is
> fired.

Peter is correct that he should not need to worry about modules, he 
doesn't build kernels with them ;-)

Here's another patch that moves the module ref count administration to the 
trace events themselves. This should satisfy both you and Peter.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1b1f742..3f7c5dc 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
  *
  */
 
+#ifdef MODULE
+# define event_trace_up_ref()					\
+	do {							\
+		if (!try_module_get(THIS_MODULE)) {		\
+			atomic_dec(&event_call->profile_count);	\
+			return -1;				\
+		}						\
+	} while (0)
+# define event_trace_down_ref() module_put(THIS_MODULE)
+#else
+# define event_trace_up_ref() do { } while (0)
+# define event_trace_down_ref() do { } while (0)
+#endif
+		
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
 									\
@@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
 {									\
 	int ret = 0;							\
 									\
-	if (!atomic_inc_return(&event_call->profile_count))		\
+	if (!atomic_inc_return(&event_call->profile_count)) {		\
+		event_trace_up_ref();					\
 		ret = register_trace_##call(ftrace_profile_##call);	\
+	}								\
 									\
 	return ret;							\
 }									\
 									\
 static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
 {									\
-	if (atomic_add_negative(-1, &event_call->profile_count))	\
+	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
 		unregister_trace_##call(ftrace_profile_##call);		\
+		event_trace_down_ref();					\
+	}								\
 }
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> 
> 
> On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:
> 
> > * Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote:
> > > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > > 
> > > > This patch solves the problem that Li originally reported. If something 
> > > > registers a trace point belonging to a module, then it ups the ref count 
> > > > of the module. This prevents a process from registering a probe to a 
> > > > tracepoint belonging to a module and then having the module disappear.
> > > > 
> > > > Doing the example with perf in Li's original post, now errors on the 
> > > > rmmod, with "ERROR: Module trace_events_sample is in use".
> > > > 
> > > > Mathieu, can I have your acked-by on this?
> > > > 
> > > 
> > > Sorry, it looks buggy.
> 
> Well, it is not that buggy. It would only prevent modules from unloading 
> if another tracepoint with the same name and parameters had a probe 
> registered. More of a too big of a tent deal.
> 
> > > 
> > > It does not deal with the fact that tracepoints with the same name and
> > > arguments can be present in more than one module, or in a combination of
> > > kernel core and modules.
> > > 
> > > The struct tracepoint_entry is specific to a a tracepoint name, used for
> > > registration, but is eventually tied to all tracepoint instrumentation
> > > instances for this tracepoint name.
> > > 
> 
> Anyway, this prevents your tracepoints from doing the odd things of 
> loading a probe before it exists. Well you can, but then you prevent the 
> unload of the module that registered it. Fine, I chucked out that patch.
> 

you should try adding the required tracepoint_synchronize_unregister()
call to ftrace_profile_disable_##call in ftrace.h. I expect this will
fix your problem.

Note that this is a bit slow to call it at each unregistration. Ideally,
a module containing tracepoint probes should call this synchronization
primitive only once at module unload.

Mathieu

> > 
> > Looking at the original post:
> > http://lkml.org/lkml/2009/8/24/5
> > 
> > the problem seems to be caused by the fact that the
> > trace_event_profile.c keeps some knowledge of modules in internal data
> > structures, but does not get notified of module unloads. Why don't we
> > fix that instead ?
> > 
> > A quick glance at it seems to indicate that it lazily discovers new
> > modules when the tracepoints are hit. Using module load/unload notifiers
> > would be more appropriate. Or maybe adding a notifier call to
> > tracepoint.c, calling notification callbacks for probe modules which
> > need to know when the connected tracepoints are changing
> > (when they are connected/disconnected) would probably be even more
> > appropriate. As a result, it would remove the dynamic verification cost
> > implied by lazy data structure lookup and check each time the probe is
> > fired.
> 
> Peter is correct that he should not need to worry about modules, he 
> doesn't build kernels with them ;-)
> 
> Here's another patch that moves the module ref count administration to the 
> trace events themselves. This should satisfy both you and Peter.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 1b1f742..3f7c5dc 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
>   *
>   */
>  
> +#ifdef MODULE
> +# define event_trace_up_ref()					\
> +	do {							\
> +		if (!try_module_get(THIS_MODULE)) {		\
> +			atomic_dec(&event_call->profile_count);	\
> +			return -1;				\
> +		}						\
> +	} while (0)
> +# define event_trace_down_ref() module_put(THIS_MODULE)
> +#else
> +# define event_trace_up_ref() do { } while (0)
> +# define event_trace_down_ref() do { } while (0)
> +#endif
> +		
>  #undef TRACE_EVENT
>  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
>  									\
> @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
>  {									\
>  	int ret = 0;							\
>  									\
> -	if (!atomic_inc_return(&event_call->profile_count))		\
> +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> +		event_trace_up_ref();					\
>  		ret = register_trace_##call(ftrace_profile_##call);	\
> +	}								\
>  									\
>  	return ret;							\
>  }									\
>  									\
>  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
>  {									\
> -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
>  		unregister_trace_##call(ftrace_profile_##call);		\
> +		event_trace_down_ref();					\
> +	}								\
>  }
>  
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:

> > Anyway, this prevents your tracepoints from doing the odd things of 
> > loading a probe before it exists. Well you can, but then you prevent the 
> > unload of the module that registered it. Fine, I chucked out that patch.
> > 
> 
> you should try adding the required tracepoint_synchronize_unregister()
> call to ftrace_profile_disable_##call in ftrace.h. I expect this will
> fix your problem.

Just did, and it did not solve the bug.

> 
> Note that this is a bit slow to call it at each unregistration. Ideally,
> a module containing tracepoint probes should call this synchronization
> primitive only once at module unload.

The bug looks like it is registering a probe, but not unregistering it 
before leaving the module. But when the module is loaded again, it now has 
a bad function to call when the tracepoint is hit.

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> > > Anyway, this prevents your tracepoints from doing the odd things of 
> > > loading a probe before it exists. Well you can, but then you prevent the 
> > > unload of the module that registered it. Fine, I chucked out that patch.
> > > 
> > 
> > you should try adding the required tracepoint_synchronize_unregister()
> > call to ftrace_profile_disable_##call in ftrace.h. I expect this will
> > fix your problem.
> 
> Just did, and it did not solve the bug.
> 
> > 
> > Note that this is a bit slow to call it at each unregistration. Ideally,
> > a module containing tracepoint probes should call this synchronization
> > primitive only once at module unload.
> 
> The bug looks like it is registering a probe, but not unregistering it 
> before leaving the module. But when the module is loaded again, it now has 
> a bad function to call when the tracepoint is hit.
> 
> -- Steve

Hrm, looking at #define _TRACE_PROFILE(call, proto, args) from ftrace.h
is interesting. The profile reg/unreg are actually visible as:

#define _TRACE_PROFILE_INIT(call)                                       \
        .profile_count = ATOMIC_INIT(-1),                               \
        .profile_enable = ftrace_profile_enable_##call,                 \
        .profile_disable = ftrace_profile_disable_##call,

So each time profiling is activated, we iterate on all tracepoints to
call their profile_enable. But they stay active even when the module is
unloaded. That's odd.

If _anything_ external to the module containing the probe takes
responsibility for registering the probe to a tracepoint, it should
unregister it before the module exits.

One simple quick-and-dirty to do this is to increment the refcount of
the module containing the probe before enabling it and decrementing the
refcount after disabling its probe. This is one of the solutions you
proposed so far, and it looks sane. I use something similar in LTTng
btw. Probe modules are refcounted when probes are connected to a
tracepoints by the "probe manager".

However, because you put the DEFINE_TRACE within the same module as your
actual code, you cannot unload the module unless you stop profiling. One
way to fix this is, as Peter proposed, use a different module for the
probe than the module you are probing. I agree with him. The module used
as probe for tracing is expected to be somehow tied to tracing sessions,
and I think it is OK to expect the probe module no to be unloadable as
long as a tracing session which uses it is active.

Mathieu


-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
>> This patch solves the problem that Li originally reported. If something 
>> registers a trace point belonging to a module, then it ups the ref count 
>> of the module. This prevents a process from registering a probe to a 
>> tracepoint belonging to a module and then having the module disappear.
>>
>> Doing the example with perf in Li's original post, now errors on the 
>> rmmod, with "ERROR: Module trace_events_sample is in use".
>>
>> Mathieu, can I have your acked-by on this?
>>
> 
> Sorry, it looks buggy.
> 

And the patch itself is buggy.

> It does not deal with the fact that tracepoints with the same name and
> arguments can be present in more than one module, or in a combination of
> kernel core and modules.
> 
> The struct tracepoint_entry is specific to a a tracepoint name, used for
> registration, but is eventually tied to all tracepoint instrumentation
> instances for this tracepoint name.
> 
> Mathieu
> 

...

>> -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>> +tracepoint_update_probe_range(struct module *mod,
>> +			      struct tracepoint *begin, struct tracepoint *end)
>>  {
>>  	struct tracepoint *iter;
>>  	struct tracepoint_entry *mark_entry;
>> @@ -286,9 +291,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>>  	for (iter = begin; iter < end; iter++) {
>>  		mark_entry = get_tracepoint(iter->name);
>>  		if (mark_entry) {
>> +			if (mod && !mark_entry->mod) {
>> +				if (!try_module_get(mod))
>> +					goto disable;

You can hit this code-path even when you unregister a probe,
so the right way is:
	module_get() when iter->state changed from 0 from 1
	module_put() when iter->state changed from 1 to 0

But still has some other problems. You don't fail the registration,
profile_enable() will return success even when the module is
being destructed.

>> +				mark_entry->mod = mod;
>> +			}
>>  			set_tracepoint(&mark_entry, iter,
>>  					!!mark_entry->refcount);
>>  		} else {
>> + disable:
>>  			disable_tracepoint(iter);
>>  		}
>>  	}

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Li Zefan]

> Peter is correct that he should not need to worry about modules, he 
> doesn't build kernels with them ;-)
> 
> Here's another patch that moves the module ref count administration to the 
> trace events themselves. This should satisfy both you and Peter.
> 

In fact I had this patch in my mind, but I thought Peter insist
on fixing it in tracepoint. So I'm fine with this. :)

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 1b1f742..3f7c5dc 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
>   *
>   */
>  
> +#ifdef MODULE
> +# define event_trace_up_ref()					\
> +	do {							\
> +		if (!try_module_get(THIS_MODULE)) {		\
> +			atomic_dec(&event_call->profile_count);	\
> +			return -1;				\

Should return -1 or -errno like -ENOENT?

> +		}						\
> +	} while (0)
> +# define event_trace_down_ref() module_put(THIS_MODULE)
> +#else
> +# define event_trace_up_ref() do { } while (0)
> +# define event_trace_down_ref() do { } while (0)
> +#endif
> +		
>  #undef TRACE_EVENT
>  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
>  									\
> @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
>  {									\
>  	int ret = 0;							\
>  									\
> -	if (!atomic_inc_return(&event_call->profile_count))		\
> +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> +		event_trace_up_ref();					\
>  		ret = register_trace_##call(ftrace_profile_##call);	\
> +	}								\
>  									\
>  	return ret;							\
>  }									\
>  									\
>  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
>  {									\
> -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
>  		unregister_trace_##call(ftrace_profile_##call);		\
> +		event_trace_down_ref();					\
> +	}								\
>  }
>  
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> 
> 

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Li Zefan wrote:

> > Peter is correct that he should not need to worry about modules, he 
> > doesn't build kernels with them ;-)
> > 
> > Here's another patch that moves the module ref count administration to the 
> > trace events themselves. This should satisfy both you and Peter.
> > 
> 
> In fact I had this patch in my mind, but I thought Peter insist
> on fixing it in tracepoint. So I'm fine with this. :)

Tracepoints I consider a more low level API. Using them takes more hand 
work and the user needs to know what they are doing and thus, must take 
into account modules.

This code is automatic, and a much higher level API. The user shoud not 
need to worry about modules, thus the protection belongs here.

If we try to make it so a module can not have register itself, then that 
will just complicate the TRACE_EVENT macros even more. And those are 
complex enough.

> 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > index 1b1f742..3f7c5dc 100644
> > --- a/include/trace/ftrace.h
> > +++ b/include/trace/ftrace.h
> > @@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
> >   *
> >   */
> >  
> > +#ifdef MODULE
> > +# define event_trace_up_ref()					\
> > +	do {							\
> > +		if (!try_module_get(THIS_MODULE)) {		\
> > +			atomic_dec(&event_call->profile_count);	\
> > +			return -1;				\
> 
> Should return -1 or -errno like -ENOENT?

Thanks, I was being lazy and really did not know what to have it return. 
I'll commit it with a -ENOENT.

-- Steve

> 
> > +		}						\
> > +	} while (0)
> > +# define event_trace_down_ref() module_put(THIS_MODULE)
> > +#else
> > +# define event_trace_up_ref() do { } while (0)
> > +# define event_trace_down_ref() do { } while (0)
> > +#endif
> > +		
> >  #undef TRACE_EVENT
> >  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
> >  									\
> > @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
> >  {									\
> >  	int ret = 0;							\
> >  									\
> > -	if (!atomic_inc_return(&event_call->profile_count))		\
> > +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> > +		event_trace_up_ref();					\
> >  		ret = register_trace_##call(ftrace_profile_##call);	\
> > +	}								\
> >  									\
> >  	return ret;							\
> >  }									\
> >  									\
> >  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
> >  {									\
> > -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> > +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
> >  		unregister_trace_##call(ftrace_profile_##call);		\
> > +		event_trace_down_ref();					\
> > +	}								\
> >  }
> >  
> >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > 
> > 
> 

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Wed, 2009-08-26 at 15:48 -0400, Steven Rostedt wrote:

> Here's another patch that moves the module ref count administration to the 
> trace events themselves. This should satisfy both you and Peter.


> +#ifdef MODULE
> +# define event_trace_up_ref()					\
> +	do {							\
> +		if (!try_module_get(THIS_MODULE)) {		\
> +			atomic_dec(&event_call->profile_count);	\
> +			return -1;				\
> +		}						\
> +	} while (0)
> +# define event_trace_down_ref() module_put(THIS_MODULE)
> +#else
> +# define event_trace_up_ref() do { } while (0)
> +# define event_trace_down_ref() do { } while (0)
> +#endif
> +		
>  #undef TRACE_EVENT
>  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
>  									\
> @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
>  {									\
>  	int ret = 0;							\
>  									\
> -	if (!atomic_inc_return(&event_call->profile_count))		\
> +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> +		event_trace_up_ref();					\
>  		ret = register_trace_##call(ftrace_profile_##call);	\
> +	}								\
>  									\
>  	return ret;							\
>  }									\
>  									\
>  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
>  {									\
> -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
>  		unregister_trace_##call(ftrace_profile_##call);		\
> +		event_trace_down_ref();					\
> +	}								\
>  }

Ha, almost, make that {un,}register_ftrace_##call() and stick the module
gunk in there, and I'm ok.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Thu, 27 Aug 2009, Li Zefan wrote:
> 
> > > Peter is correct that he should not need to worry about modules, he 
> > > doesn't build kernels with them ;-)
> > > 
> > > Here's another patch that moves the module ref count administration to the 
> > > trace events themselves. This should satisfy both you and Peter.
> > > 
> > 
> > In fact I had this patch in my mind, but I thought Peter insist
> > on fixing it in tracepoint. So I'm fine with this. :)
> 
> Tracepoints I consider a more low level API. Using them takes more hand 
> work and the user needs to know what they are doing and thus, must take 
> into account modules.
> 
> This code is automatic, and a much higher level API. The user shoud not 
> need to worry about modules, thus the protection belongs here.
> 
> If we try to make it so a module can not have register itself, then that 
> will just complicate the TRACE_EVENT macros even more. And those are 
> complex enough.
> 
> > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > > index 1b1f742..3f7c5dc 100644
> > > --- a/include/trace/ftrace.h
> > > +++ b/include/trace/ftrace.h
> > > @@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
> > >   *
> > >   */
> > >  
> > > +#ifdef MODULE
> > > +# define event_trace_up_ref()					\
> > > +	do {							\
> > > +		if (!try_module_get(THIS_MODULE)) {		\
> > > +			atomic_dec(&event_call->profile_count);	\
> > > +			return -1;				\
> > 
> > Should return -1 or -errno like -ENOENT?
> 
> Thanks, I was being lazy and really did not know what to have it return. 
> I'll commit it with a -ENOENT.
> 

Looks good. Just don't forget to eventually add the "synchronize" calls
between tracepoint unregistration and the removal of their module. There
is a race condition in the way you do it currently.

Thanks,

Mathieu

> -- Steve
> 
> > 
> > > +		}						\
> > > +	} while (0)
> > > +# define event_trace_down_ref() module_put(THIS_MODULE)
> > > +#else
> > > +# define event_trace_up_ref() do { } while (0)
> > > +# define event_trace_down_ref() do { } while (0)
> > > +#endif
> > > +		
> > >  #undef TRACE_EVENT
> > >  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
> > >  									\
> > > @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
> > >  {									\
> > >  	int ret = 0;							\
> > >  									\
> > > -	if (!atomic_inc_return(&event_call->profile_count))		\
> > > +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> > > +		event_trace_up_ref();					\
> > >  		ret = register_trace_##call(ftrace_profile_##call);	\
> > > +	}								\
> > >  									\
> > >  	return ret;							\
> > >  }									\
> > >  									\
> > >  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
> > >  {									\
> > > -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> > > +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
> > >  		unregister_trace_##call(ftrace_profile_##call);		\
> > > +		event_trace_down_ref();					\
> > > +	}								\
> > >  }
> > >  
> > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > 
> > > 
> > 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:

> Looks good. Just don't forget to eventually add the "synchronize" calls
> between tracepoint unregistration and the removal of their module. There
> is a race condition in the way you do it currently.

I'm trying to figure out the race here. What will disappear in the 
tracepoint? Could you give a brief example of the issue.

Thanks,

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> 
> > Looks good. Just don't forget to eventually add the "synchronize" calls
> > between tracepoint unregistration and the removal of their module. There
> > is a race condition in the way you do it currently.
> 
> I'm trying to figure out the race here. What will disappear in the 
> tracepoint? Could you give a brief example of the issue.

Sure,

Let's say we have a tracepoint in module instrumented.c, and a probe in
module probe.c. The probe is registered by module probe.c init through
the tracepoint infrastructure to connect to the tracepoint in
instrumented.c. Unregistration is done in probe.c module exit.

As the instrumented code get executed (let's say periodically), it calls
the connected probe. Preemption is disabled around the call.

If you unload the probe.c module, the module exit will unregister the
probe, but the probe code can still be in use by another CPU. You have
to wait for quiescent state with the tracepoint synchronize (which is
just a wrapper over synchronize_sched() before you are allowed to
complete module unload. Otherwise, you will end up reclaiming module
memory that is still used by probe execution.

A test-case for this would be to create a probe with a delay in it, and
an instrumented module calling the instrumentation in a loop. On a SMP
system, running the instrumented code and probe load/unload in loops
should trigger this race.

Mathieu


> 
> Thanks,
> 
> -- Steve
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:

> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > 
> > On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> > 
> > > Looks good. Just don't forget to eventually add the "synchronize" calls
> > > between tracepoint unregistration and the removal of their module. There
> > > is a race condition in the way you do it currently.
> > 
> > I'm trying to figure out the race here. What will disappear in the 
> > tracepoint? Could you give a brief example of the issue.
> 
> Sure,
> 
> Let's say we have a tracepoint in module instrumented.c, and a probe in
> module probe.c. The probe is registered by module probe.c init through
> the tracepoint infrastructure to connect to the tracepoint in
> instrumented.c. Unregistration is done in probe.c module exit.
> 
> As the instrumented code get executed (let's say periodically), it calls
> the connected probe. Preemption is disabled around the call.
> 
> If you unload the probe.c module, the module exit will unregister the
> probe, but the probe code can still be in use by another CPU. You have
> to wait for quiescent state with the tracepoint synchronize (which is
> just a wrapper over synchronize_sched() before you are allowed to
> complete module unload. Otherwise, you will end up reclaiming module
> memory that is still used by probe execution.
> 
> A test-case for this would be to create a probe with a delay in it, and
> an instrumented module calling the instrumentation in a loop. On a SMP
> system, running the instrumented code and probe load/unload in loops
> should trigger this race.

Thanks for the explanation. So let me see if I get this correct.

For this race to occur, the probe (code that hooks to the tracepoint) must 
be in module that does not contain the tracepoint. We don't even need more 
than one module, this could occur even with a core tracepoint. If a module 
registers it, if it unregisters before unloading, the tracepoint may be 
hit before the unregister and executing while the module is unloading.

I don't think we need to worry about this with the case of TRACE_EVENT and 
ftrace.h. The reason is that the trace point and probes are always in the 
same location. The MACROS set up the probe code with the modules. Thus, to 
remove the module, you must also remove the tracepoint itself along with 
the probe. If you can be executing in the probe, then you must have hit 
the trace point. If you hit the trace point, then you are executing code 
inside the module you are removing, which is a bug in the module code 
itself.

Using the ftrace.h MACROS limits the use of tracepoints and this race 
does not exist. I feel we are safe not needing to have the 
tracepoint_synchronize_unregister within the ftrace.h code.

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Peter Zijlstra wrote:

> On Wed, 2009-08-26 at 15:48 -0400, Steven Rostedt wrote:
> >  									\
> > @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
> >  {									\
> >  	int ret = 0;							\
> >  									\
> > -	if (!atomic_inc_return(&event_call->profile_count))		\
> > +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> > +		event_trace_up_ref();					\
> >  		ret = register_trace_##call(ftrace_profile_##call);	\
> > +	}								\
> >  									\
> >  	return ret;							\
> >  }									\
> >  									\
> >  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
> >  {									\
> > -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> > +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
> >  		unregister_trace_##call(ftrace_profile_##call);		\
> > +		event_trace_down_ref();					\
> > +	}								\
> >  }
> 
> Ha, almost, make that {un,}register_ftrace_##call() and stick the module
> gunk in there, and I'm ok.

Peter, that {un,}register_tace_##call is the tracepoint code. The 
DEFINE_TRACE creates those functions. This is the lowest level we can go 
in the ftrace.h file.

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> 
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > 
> > > On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> > > 
> > > > Looks good. Just don't forget to eventually add the "synchronize" calls
> > > > between tracepoint unregistration and the removal of their module. There
> > > > is a race condition in the way you do it currently.
> > > 
> > > I'm trying to figure out the race here. What will disappear in the 
> > > tracepoint? Could you give a brief example of the issue.
> > 
> > Sure,
> > 
> > Let's say we have a tracepoint in module instrumented.c, and a probe in
> > module probe.c. The probe is registered by module probe.c init through
> > the tracepoint infrastructure to connect to the tracepoint in
> > instrumented.c. Unregistration is done in probe.c module exit.
> > 
> > As the instrumented code get executed (let's say periodically), it calls
> > the connected probe. Preemption is disabled around the call.
> > 
> > If you unload the probe.c module, the module exit will unregister the
> > probe, but the probe code can still be in use by another CPU. You have
> > to wait for quiescent state with the tracepoint synchronize (which is
> > just a wrapper over synchronize_sched() before you are allowed to
> > complete module unload. Otherwise, you will end up reclaiming module
> > memory that is still used by probe execution.
> > 
> > A test-case for this would be to create a probe with a delay in it, and
> > an instrumented module calling the instrumentation in a loop. On a SMP
> > system, running the instrumented code and probe load/unload in loops
> > should trigger this race.
> 
> Thanks for the explanation. So let me see if I get this correct.
> 
> For this race to occur, the probe (code that hooks to the tracepoint) must 
> be in module that does not contain the tracepoint. We don't even need more 
> than one module, this could occur even with a core tracepoint. If a module 
> registers it, if it unregisters before unloading, the tracepoint may be 
> hit before the unregister and executing while the module is unloading.
> 
> I don't think we need to worry about this with the case of TRACE_EVENT and 
> ftrace.h. The reason is that the trace point and probes are always in the 
> same location. The MACROS set up the probe code with the modules. Thus, to 
> remove the module, you must also remove the tracepoint itself along with 
> the probe. If you can be executing in the probe, then you must have hit 
> the trace point. If you hit the trace point, then you are executing code 
> inside the module you are removing, which is a bug in the module code 
> itself.
> 
> Using the ftrace.h MACROS limits the use of tracepoints and this race 
> does not exist. I feel we are safe not needing to have the 
> tracepoint_synchronize_unregister within the ftrace.h code.
> 

Looks right. If you can guarantee that the probe is only called from
tracepoints located within the same module as the probe, you should be
safe without tracepoint_synchronize_unregister. It's worth adding a
comment in ftrace.h explaining that though.

Mathieu

> -- Steve
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> > 
> > For this race to occur, the probe (code that hooks to the tracepoint) must 
> > be in module that does not contain the tracepoint. We don't even need more 
> > than one module, this could occur even with a core tracepoint. If a module 
> > registers it, if it unregisters before unloading, the tracepoint may be 
> > hit before the unregister and executing while the module is unloading.
> > 
> > I don't think we need to worry about this with the case of TRACE_EVENT and 
> > ftrace.h. The reason is that the trace point and probes are always in the 
> > same location. The MACROS set up the probe code with the modules. Thus, to 
> > remove the module, you must also remove the tracepoint itself along with 
> > the probe. If you can be executing in the probe, then you must have hit 
> > the trace point. If you hit the trace point, then you are executing code 
> > inside the module you are removing, which is a bug in the module code 
> > itself.
> > 
> > Using the ftrace.h MACROS limits the use of tracepoints and this race 
> > does not exist. I feel we are safe not needing to have the 
> > tracepoint_synchronize_unregister within the ftrace.h code.
> > 
> 
> Looks right. If you can guarantee that the probe is only called from
> tracepoints located within the same module as the probe, you should be
> safe without tracepoint_synchronize_unregister. It's worth adding a
> comment in ftrace.h explaining that though.

Yeah, I tried to trick the code to see if I can get a probe in another 
module than the trace point, and I get nothing but linking errors. I'm 
sure if I work hard enough, I may trick it to do so, but if anyone does 
that, I'll slap a big fat NAK on it ;-)

OK, I modify my fix to add a comment to this effect.

Thanks,

-- Steve

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Peter Zijlstra]

On Thu, 2009-08-27 at 11:57 -0400, Steven Rostedt wrote:
> On Thu, 27 Aug 2009, Peter Zijlstra wrote:
> 
> > On Wed, 2009-08-26 at 15:48 -0400, Steven Rostedt wrote:
> > >  									\
> > > @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
> > >  {									\
> > >  	int ret = 0;							\
> > >  									\
> > > -	if (!atomic_inc_return(&event_call->profile_count))		\
> > > +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> > > +		event_trace_up_ref();					\
> > >  		ret = register_trace_##call(ftrace_profile_##call);	\
> > > +	}								\
> > >  									\
> > >  	return ret;							\
> > >  }									\
> > >  									\
> > >  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
> > >  {									\
> > > -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> > > +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
> > >  		unregister_trace_##call(ftrace_profile_##call);		\
> > > +		event_trace_down_ref();					\
> > > +	}								\
> > >  }
> > 
> > Ha, almost, make that {un,}register_ftrace_##call() and stick the module
> > gunk in there, and I'm ok.
> 
> Peter, that {un,}register_tace_##call is the tracepoint code. The 
> DEFINE_TRACE creates those functions. This is the lowest level we can go 
> in the ftrace.h file.

Did you miss the extra f?

You can wrap the regular {un,}register_trace_##call() things in
{un,}register_/F/trace_##call() functions which also take care of the
module stuff.

Your patch is virtually identical to the one that started this thread,
everybody using the TRACE_EVENT() things will still need to worry about
modules.

Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Steven Rostedt]

On Thu, 27 Aug 2009, Peter Zijlstra wrote:

> On Thu, 2009-08-27 at 11:57 -0400, Steven Rostedt wrote:
> > On Thu, 27 Aug 2009, Peter Zijlstra wrote:
> > 
> > > On Wed, 2009-08-26 at 15:48 -0400, Steven Rostedt wrote:
> > > >  									\
> > > > @@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
> > > >  {									\
> > > >  	int ret = 0;							\
> > > >  									\
> > > > -	if (!atomic_inc_return(&event_call->profile_count))		\
> > > > +	if (!atomic_inc_return(&event_call->profile_count)) {		\
> > > > +		event_trace_up_ref();					\
> > > >  		ret = register_trace_##call(ftrace_profile_##call);	\
> > > > +	}								\
> > > >  									\
> > > >  	return ret;							\
> > > >  }									\
> > > >  									\
> > > >  static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
> > > >  {									\
> > > > -	if (atomic_add_negative(-1, &event_call->profile_count))	\
> > > > +	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
> > > >  		unregister_trace_##call(ftrace_profile_##call);		\
> > > > +		event_trace_down_ref();					\
> > > > +	}								\
> > > >  }
> > > 
> > > Ha, almost, make that {un,}register_ftrace_##call() and stick the module
> > > gunk in there, and I'm ok.
> > 
> > Peter, that {un,}register_tace_##call is the tracepoint code. The 
> > DEFINE_TRACE creates those functions. This is the lowest level we can go 
> > in the ftrace.h file.
> 
> Did you miss the extra f?

I thought that was a typo since there is no register_ftrace_##call.

> 
> You can wrap the regular {un,}register_trace_##call() things in
> {un,}register_/F/trace_##call() functions which also take care of the
> module stuff.

Oh, you mean you want me to create a wrapper for it? Note, this does not
affect the normal trace "enable" code, because there's a module call back 
in trace_events.c that disables all the trace points in a module before 
unloading. And this in turn will unregister all events.

> Your patch is virtually identical to the one that started this thread,
> everybody using the TRACE_EVENT() things will still need to worry about
> modules.

That's not true. You are not using the TRACE_EVENT, you are modifying the 
TRACE_EVENT. That is different. I don't see how a user could hit this 
issue. You hit it because you modified what was in ftrace.h, and that does 
not count as a user. That's a developer ;-)

-- Steve

[tip:tracing/core] tracing/profile: fix profile_disable vs module_unload

[tip-bot for Li Zefan]

Commit-ID:  558e6547e4b8a2b13608a24a9d3679802f91c4c7
Gitweb:     http://git.kernel.org/tip/558e6547e4b8a2b13608a24a9d3679802f91c4c7
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Mon, 24 Aug 2009 12:19:47 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Sat, 12 Sep 2009 22:28:38 -0400

tracing/profile: fix profile_disable vs module_unload

If the correspoding module is unloaded before ftrace_profile_disable()
is called, event->profile_disable() won't be called, which can
cause oops:

  # insmod trace-events-sample.ko
  # perf record -f -a -e sample:foo_bar sleep 3 &
  # sleep 1
  # rmmod trace_events_sample
  # insmod trace-events-sample.ko
  OOPS!

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
LKML-Reference: <4A9214E3.2070807@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 kernel/trace/trace_event_profile.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index 11ba5bb..55a25c9 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -5,6 +5,7 @@
  *
  */
 
+#include <linux/module.h>
 #include "trace.h"
 
 int ftrace_profile_enable(int event_id)
@@ -14,7 +15,8 @@ int ftrace_profile_enable(int event_id)
 
 	mutex_lock(&event_mutex);
 	list_for_each_entry(event, &ftrace_events, list) {
-		if (event->id == event_id && event->profile_enable) {
+		if (event->id == event_id && event->profile_enable &&
+		    try_module_get(event->mod)) {
 			ret = event->profile_enable(event);
 			break;
 		}
@@ -32,6 +34,7 @@ void ftrace_profile_disable(int event_id)
 	list_for_each_entry(event, &ftrace_events, list) {
 		if (event->id == event_id) {
 			event->profile_disable(event);
+			module_put(event->mod);
 			break;
 		}
 	}