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 <[email protected]>

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

kernel/trace/trace_event_profile.c +4 -1

index 11ba5bb..55a25c9 100644
@@ -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;
 		}
 	}

Quoted 11 lines · 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!

Quoted 1 line

Signed-off-by Li Zefan <[email protected]>

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

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · 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 <[email protected]>

Quoted 3 lines

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.

Quoted 16 lines · Li Zefan

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · 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 <[email protected]>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 26 lines · Peter Zijlstra

Quoted 16 lines · Li Zefan

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · 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 <[email protected]>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 26 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 7 lines

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)

Quoted 1 line

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

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

Quoted 34 lines

Quoted 20 lines

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

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)

Quoted 1 line

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

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

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

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)

Quoted 5 lines

Quoted 1 line

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

Quoted 48 lines

Quoted 28 lines

Quoted 28 lines

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Li Zefan <[email protected]>

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.

Quoted 7 lines

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.

Quoted 9 lines

Quoted 6 lines

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

Quoted 5 lines

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.

Quoted 48 lines

Quoted 48 lines

Quoted 28 lines

Quoted 28 lines

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Li Zefan <[email protected]>

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.

Quoted 7 lines

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.

Quoted 9 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 5 lines

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;						\
	}

Quoted 2 lines

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.

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.

Quoted 5 lines · Peter Zijlstra

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.

Quoted 8 lines · Ingo Molnar

Quoted 5 lines · Peter Zijlstra

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.

Quoted 3 lines

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.

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Ingo Molnar

Quoted 5 lines · Peter Zijlstra

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.

Quoted 2 lines

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?

Quoted 18 lines · Ingo Molnar

Quoted 15 lines · Peter Zijlstra

Quoted 11 lines · Ingo Molnar

Quoted 5 lines · Peter Zijlstra

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.

Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 6 lines · Ingo Molnar

Quoted 2 lines

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.

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Ingo Molnar

Quoted 2 lines

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.

Quoted 1 line

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.

CC Mathieu

Quoted 8 lines · Peter Zijlstra

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Ingo Molnar

Quoted 2 lines

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.

Quoted 7 lines

Quoted 1 line

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?

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

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

Quoted 2 lines · Peter Zijlstra

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.

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

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

Quoted 8 lines

 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*

Quoted 18 lines · Li Zefan

Quoted 9 lines · Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

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

Quoted 8 lines

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

}

in register_trace_##name() or thereabout.

Quoted 7 lines

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.

Quoted 1 line

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

In which case the tracepoint registration fails, right?

Quoted 36 lines · Peter Zijlstra

Quoted 18 lines · Li Zefan

Quoted 9 lines · Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

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

Quoted 8 lines

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

}

in register_trace_##name() or thereabout.

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 32 lines

Quoted 32 lines

Quoted 15 lines

Quoted 7 lines

Quoted 2 lines

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

Quoted 8 lines

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

}

in register_trace_##name() or thereabout.

Quoted 7 lines

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.

Quoted 1 line

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

In which case the tracepoint registration fails, right?

Quoted 4 lines

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?

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

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

Quoted 8 lines

 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*

Quoted 28 lines · Li Zefan

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

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

Quoted 8 lines

 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,

Quoted 1 line

*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

include/linux/tracepoint.h +3 -2

index 0341f2e..055275b 100644
@@ -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)	\

kernel/module.c +1 -1

index b182143..a8e69fa 100644
@@ -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);
 }

kernel/tracepoint.c +10 -3

index 06f165a..b150255 100644
@@ -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;
 	}

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 <[email protected]>

include/linux/tracepoint.h +3 -2

index 0341f2e..055275b 100644
@@ -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)	\

kernel/module.c +1 -1

index b182143..a8e69fa 100644
@@ -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);
 }

kernel/tracepoint.c +14 -3

index 06f165a..089e6f9 100644
@@ -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;
 	}

Quoted 11 lines · * Steven Rostedt ([email protected]) 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

Quoted 106 lines

Signed-off-by Steven Rostedt <[email protected]>

include/linux/tracepoint.h +3 -2

index 0341f2e..055275b 100644
@@ -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)	\

kernel/module.c +1 -1

index b182143..a8e69fa 100644
@@ -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);
 }

kernel/tracepoint.c +14 -3

index 06f165a..089e6f9 100644
@@ -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;
 	}

Signature · 2 lines

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

Quoted 7 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 6 lines · * Steven Rostedt ([email protected]) 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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 9 lines

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.

Quoted 8 lines · Mathieu Desnoyers

Quoted 7 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 6 lines · * Steven Rostedt ([email protected]) 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.

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 9 lines

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 ?

Quoted 9 lines

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 <[email protected]>

include/trace/ftrace.h +20 -2

index 1b1f742..3f7c5dc 100644
@@ -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)

Quoted 4 lines · * Steven Rostedt ([email protected]) 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.

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.

Quoted 10 lines · Mathieu Desnoyers

Quoted 4 lines · * Steven Rostedt ([email protected]) 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.

Quoted 4 lines

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

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 <[email protected]>

include/trace/ftrace.h +0 -0

index 1b1f742..3f7c5dc 100644

include/trace/ftrace.h +14 -2

+		}						\
+	} 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)

Quoted 6 lines

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

Quoted 11 lines

Signed-off-by Steven Rostedt <[email protected]>

include/trace/ftrace.h +0 -0

index 1b1f742..3f7c5dc 100644
@@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
  *
  */
 

include/trace/ftrace.h static inline int ftrace_get_offsets_##call( \ · line 398

Quoted 6 lines

include/trace/ftrace.h +6 -0

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

Quoted 37 lines

include/trace/ftrace.h +14 -2

+		}						\
+	} 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)

Quoted 9 lines · Li Zefan

Quoted 6 lines

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.

Quoted 11 lines

Quoted 11 lines

Signed-off-by Steven Rostedt <[email protected]>

include/trace/ftrace.h +0 -0

index 1b1f742..3f7c5dc 100644
@@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
  *
  */
 

Quoted 8 lines

include/trace/ftrace.h static inline int ftrace_get_offsets_##call( \ · line 398

Quoted 6 lines

include/trace/ftrace.h +6 -0

+#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

Quoted 38 lines

Quoted 37 lines

include/trace/ftrace.h +14 -2

+		}						\
+	} 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)

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.

Quoted 3 lines · Mathieu Desnoyers

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

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Mathieu Desnoyers

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.

Quoted 28 lines · Mathieu Desnoyers

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Mathieu Desnoyers

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.

Quoted 4 lines

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

Quoted 16 lines

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.

Quoted 4 lines

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.

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

include/trace/ftrace.h +17 -1

+#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;							\

Quoted 17 lines · Steven Rostedt

include/trace/ftrace.h +3 -2

 									\

@@ -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))	\

include/trace/ftrace.h static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \ · line 430

Quoted 5 lines

include/trace/ftrace.h +3 -0

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

Quoted 18 lines · Peter Zijlstra

Quoted 17 lines · Steven Rostedt

include/trace/ftrace.h +3 -2

 									\

@@ -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))	\

Quoted 8 lines

include/trace/ftrace.h static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \ · line 430

Quoted 5 lines

include/trace/ftrace.h +3 -0

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

Quoted 29 lines · Steven Rostedt

Quoted 26 lines · Peter Zijlstra

Quoted 12 lines · Steven Rostedt

include/trace/ftrace.h +3 -1

 									\

@@ -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;							\

include/trace/ftrace.h static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \ · line 430

Quoted 10 lines

include/trace/ftrace.h +3 -1

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

Quoted 3 lines

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.

Quoted 5 lines · * Steven Rostedt ([email protected]) 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.

Quoted 5 lines

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

Quoted 5 lines

kernel/tracepoint.c +2 -1

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

kernel/tracepoint.c tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end) · line 296

Quoted 8 lines

kernel/tracepoint.c +3 -0

 	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;

Quoted 9 lines

kernel/tracepoint.c +3 -0

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

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

Quoted 15 lines · Mathieu Desnoyers

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.

Quoted 1 line

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.