From: Steven Rostedt <[email protected]>
Li Zefan discovered that doing the following:
# 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
Would cause an OOPS. This was because the registering of the profiler
registers inside the module and does not unregister when unloaded.
This patch adds an increment of the module refcount when a profile
registers a tracepoint, to prevent a module from being unloaded
while being profiled.
Reported-by: Li Zefan <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/trace/ftrace.h | 22 ++++++++++++++++++++--
1 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 308bafd..59e09f9 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -400,6 +400,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 -ENOENT; \
+ } \
+ } 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) \
\
@@ -409,16 +423,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)
--
1.6.3.3
--
On Fri, 2009-09-11 at 09:54 -0400, Steven Rostedt wrote:
> +#ifdef MODULE
> +# define event_trace_up_ref() \
> + do { \
> + if (!try_module_get(THIS_MODULE)) { \
> + atomic_dec(&event_call->profile_count); \
> + return -ENOENT; \
> + } \
> + } 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
That's like truely gruesomely ugly.
At the very least write it like:
int event_trace_up_ref(struct ftrace_event_call *call)
{
if (!try_module_get(THIS_MODULE)) {
atomic_dev(&call->profile_count);
return -ENOENT;
}
return 0;
}
On Fri, 2009-09-11 at 16:04 +0200, Peter Zijlstra wrote:
> On Fri, 2009-09-11 at 09:54 -0400, Steven Rostedt wrote:
>
> > +#ifdef MODULE
> > +# define event_trace_up_ref() \
> > + do { \
> > + if (!try_module_get(THIS_MODULE)) { \
> > + atomic_dec(&event_call->profile_count); \
> > + return -ENOENT; \
> > + } \
> > + } 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
>
> That's like truely gruesomely ugly.
>
> At the very least write it like:
>
> int event_trace_up_ref(struct ftrace_event_call *call)
> {
> if (!try_module_get(THIS_MODULE)) {
> atomic_dev(&call->profile_count);
> return -ENOENT;
> }
> return 0;
> }
Or we can go with Li's original patch, that was less ugly.
I still think tracepoints/markers should sort this out, because we now
have a sematic difference between the two wrt modules.
On Fri, 2009-09-11 at 16:04 +0200, Peter Zijlstra wrote:
> On Fri, 2009-09-11 at 09:54 -0400, Steven Rostedt wrote:
>
> > +#ifdef MODULE
> > +# define event_trace_up_ref() \
> > + do { \
> > + if (!try_module_get(THIS_MODULE)) { \
> > + atomic_dec(&event_call->profile_count); \
> > + return -ENOENT; \
> > + } \
> > + } 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
>
> That's like truely gruesomely ugly.
>
> At the very least write it like:
>
> int event_trace_up_ref(struct ftrace_event_call *call)
> {
> if (!try_module_get(THIS_MODULE)) {
> atomic_dev(&call->profile_count);
> return -ENOENT;
> }
> return 0;
> }
OK, I'll replace the MACROS with static inline, rebase and resend.
Ingo, don't pull that yet.
-- Steve
On Fri, 2009-09-11 at 16:09 +0200, Peter Zijlstra wrote:
> On Fri, 2009-09-11 at 16:04 +0200, Peter Zijlstra wrote:
> > On Fri, 2009-09-11 at 09:54 -0400, Steven Rostedt wrote:
> >
> > > +#ifdef MODULE
> > > +# define event_trace_up_ref() \
> > > + do { \
> > > + if (!try_module_get(THIS_MODULE)) { \
> > > + atomic_dec(&event_call->profile_count); \
> > > + return -ENOENT; \
> > > + } \
> > > + } 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
> >
> > That's like truely gruesomely ugly.
> >
> > At the very least write it like:
> >
> > int event_trace_up_ref(struct ftrace_event_call *call)
> > {
> > if (!try_module_get(THIS_MODULE)) {
> > atomic_dev(&call->profile_count);
> > return -ENOENT;
> > }
> > return 0;
> > }
>
> Or we can go with Li's original patch, that was less ugly.
I can go back to Li's original patch, but the talk on that was
"fragile". If you no longer feel that way, then I'll use his instead.
For now, I'll pull out this patch altogether, and resubmit the pull
request without it. I'd like the other changes to not be held up by
this.
>
> I still think tracepoints/markers should sort this out, because we now
> have a sematic difference between the two wrt modules.
I originally tried to do it in the tracepoint logic, but that broke a
lot of assumptions about tracepoints that Mathieu pointed out. This is
not a normal use of tracepoints. It is expected that if you register a
probe in a module, you will unregister it before exiting.
I can't remember all the details, but at the end, it seemed that the fix
belonged at the ftrace level.
-- Steve
On Fri, 2009-09-11 at 10:33 -0400, Steven Rostedt wrote:
> > Or we can go with Li's original patch, that was less ugly.
>
> I can go back to Li's original patch, but the talk on that was
> "fragile". If you no longer feel that way, then I'll use his instead.
>
> For now, I'll pull out this patch altogether, and resubmit the pull
> request without it. I'd like the other changes to not be held up by
> this.
Right, I still think its at the wrong level,. see below.
> >
> > I still think tracepoints/markers should sort this out, because we now
> > have a sematic difference between the two wrt modules.
>
> I originally tried to do it in the tracepoint logic, but that broke a
> lot of assumptions about tracepoints that Mathieu pointed out. This is
> not a normal use of tracepoints. It is expected that if you register a
> probe in a module, you will unregister it before exiting.
>
> I can't remember all the details, but at the end, it seemed that the fix
> belonged at the ftrace level.
Right, Mathieu thinks its sane to be able to attach to
tracepoints/markers before they exist, so you can put them in module
init code. I disagree.
ftrace doesn't mirror this behaviour, that is the source of the problem.
If it did the ftrace structures wouldn't go away on unload and there
wouldn't be no crash.
But if you want to maintain this disparity between the two frameworks
then yes Li's patch, or yours (they're identical) seems the way to solve
it.
Still think its daft though.
On Fri, 2009-09-11 at 16:09 +0200, Peter Zijlstra wrote:
> On Fri, 2009-09-11 at 16:04 +0200, Peter Zijlstra wrote:
> > On Fri, 2009-09-11 at 09:54 -0400, Steven Rostedt wrote:
> >
> > > +#ifdef MODULE
> > > +# define event_trace_up_ref() \
> > > + do { \
> > > + if (!try_module_get(THIS_MODULE)) { \
> > > + atomic_dec(&event_call->profile_count); \
> > > + return -ENOENT; \
> > > + } \
> > > + } 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
> >
> > That's like truely gruesomely ugly.
> >
> > At the very least write it like:
> >
> > int event_trace_up_ref(struct ftrace_event_call *call)
> > {
> > if (!try_module_get(THIS_MODULE)) {
> > atomic_dev(&call->profile_count);
> > return -ENOENT;
> > }
> > return 0;
> > }
>
> Or we can go with Li's original patch, that was less ugly.
Here's another version of the patch (less ugly). I forgot that the
try_module_get should handle code compiled with !CONFIG_MODULE.
This should be less ugly.
-- Steve
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 308bafd..6d5edd1 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -409,16 +409,23 @@ 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)) { \
+ if (!try_module_get(THIS_MODULE)) \
+ goto out_fail; \
ret = register_trace_##call(ftrace_profile_##call); \
+ } \
\
return ret; \
+ out_fail: \
+ atomic_dec(&event_call->profile_count); \
} \
\
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); \
+ module_put(THIS_MODULE) \
+ } \
}
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
On Fri, 2009-09-11 at 16:37 +0200, Peter Zijlstra wrote:
> > I originally tried to do it in the tracepoint logic, but that broke a
> > lot of assumptions about tracepoints that Mathieu pointed out. This is
> > not a normal use of tracepoints. It is expected that if you register a
> > probe in a module, you will unregister it before exiting.
> >
> > I can't remember all the details, but at the end, it seemed that the fix
> > belonged at the ftrace level.
>
> Right, Mathieu thinks its sane to be able to attach to
> tracepoints/markers before they exist, so you can put them in module
> init code. I disagree.
>
> ftrace doesn't mirror this behaviour, that is the source of the problem.
> If it did the ftrace structures wouldn't go away on unload and there
> wouldn't be no crash.
>
> But if you want to maintain this disparity between the two frameworks
> then yes Li's patch, or yours (they're identical) seems the way to solve
> it.
I'll take Li's patch then. He wrote it first.
>
> Still think its daft though.
>
Well, it needs to be fixed, and I don't want to put up with a flamewar
about changing the way tracepoints currently work. I'll take Li's patch
and send out another pull request.
-- Steve
* Peter Zijlstra ([email protected]) wrote:
> On Fri, 2009-09-11 at 10:33 -0400, Steven Rostedt wrote:
> > > Or we can go with Li's original patch, that was less ugly.
> >
> > I can go back to Li's original patch, but the talk on that was
> > "fragile". If you no longer feel that way, then I'll use his instead.
> >
> > For now, I'll pull out this patch altogether, and resubmit the pull
> > request without it. I'd like the other changes to not be held up by
> > this.
>
> Right, I still think its at the wrong level,. see below.
>
> > >
> > > I still think tracepoints/markers should sort this out, because we now
> > > have a sematic difference between the two wrt modules.
> >
> > I originally tried to do it in the tracepoint logic, but that broke a
> > lot of assumptions about tracepoints that Mathieu pointed out. This is
> > not a normal use of tracepoints. It is expected that if you register a
> > probe in a module, you will unregister it before exiting.
> >
> > I can't remember all the details, but at the end, it seemed that the fix
> > belonged at the ftrace level.
>
> Right, Mathieu thinks its sane to be able to attach to
> tracepoints/markers before they exist, so you can put them in module
> init code. I disagree.
I know we disagree on this topic, but AFAIK you did not propose any
solution to solve the problem of tracepoints in module init code. This
leaves a tracepoint user who need to trace module init with no way to do
it, because he will run in the chicken and egg problem of having to load
the module to make the tracepoint appear, but needing to trace module
load. A similar problem applies to kernel boot tracing for tracepoints
in init code of the kernel image.
Maybe you have a solution for this in mind ? Or is it simply to reduce
tracepoint instrumentation coverage to exclude init functions ? You not
caring about this use case does not mean no one care about it.
>
> ftrace doesn't mirror this behaviour, that is the source of the problem.
> If it did the ftrace structures wouldn't go away on unload and there
> wouldn't be no crash.
Ftrace and lttng need to perform refcounting the probe module
independently of this specific behavior, because they offer an interface
located outside of tracepoint or probe module to manage the
tracepoints/probes connected. Ftrace does it in ftrace, LTTng does it
within the markers. It therefore make sense to keep a global registry of
loaded probes, and to refcount the probe module before connecting a
probe to a tracepoint. I think the solution Li proposed is good.
Thanks,
Mathieu
>
> But if you want to maintain this disparity between the two frameworks
> then yes Li's patch, or yours (they're identical) seems the way to solve
> it.
>
> Still think its daft though.
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68