linux-kernel - Re: [PATCH] tracing/profile: Fix profile_disable vs module

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090826222949.GB16363@Krystal>
Date:	Wed, 26 Aug 2009 18:29:49 -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

* Steven Rostedt (rostedt@...dmis.org) wrote:
> 
> On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@...dmis.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
--
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/