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

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

* 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