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

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* 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