| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090826181758.GA30248@Krystal>
Date: Wed, 26 Aug 2009 14:17:58 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Li Zefan <lizf@...fujitsu.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...e.hu>,
Frederic Weisbecker <fweisbec@...il.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload
* Mathieu Desnoyers (mathieu.desnoyers@...ymtl.ca) wrote:
> * Steven Rostedt (rostedt@...dmis.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@...dmis.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists