Re: PATCH? trace_remove_event_call() should fail if call is active

[Oleg Nesterov]

On 07/02, Steven Rostedt wrote:
>
> On Tue, 2013-07-02 at 21:34 +0200, Oleg Nesterov wrote:
> >
> > To simplify, lets ignore trace_module_remove_events() which calls
> > __trace_remove_event_call() too, although at first glance this
> > case should be fine too.
>
> We can't ignore modules. If it fails to be removed, then it will leak
> memory.

Ah, but I only meant "ignore for the moment", to simplify the discussion.
We can add the "force" argument for trace_module_remove_events(), or
we can add another do_for_each_event_file/FTRACE_EVENT_FL_ENABLED loop
into trace_remove_event_call() before __trace_remove_event_call().
This is trivial.

So please ignore modules ;)

> > It really seems to me that trace_remove_event_call() should not
> > succeed if this ftrace_event_call is "active". Even if we forget
> > about perf, ftrace_event_enable_disable(file, 0) doesn't guarantee
> > reg(call, TRACE_REG_UNREGISTER) if SOFT_MODE is set.
> >
> >
> >
> > If something like this patch can work then we can fix trace_kprobe.
> > unregister_trace_probe() can do unregister_probe_event() first and
> > abort if trace_remove_event_call() fails.
> >
> > As for release_all_trace_probes(), we lose the all-or-nothing
> > semantics, but probably this is fine.
> >
> > Or I misunderstood this logic completely?
>
> I'm actually worried about this change.

Me too.

But again. Any reason trace_remove_event_call() should succeed if
this this ftrace_event_call is active?

> But what if we simply add a new event file flag called
> FTRACE_EVENT_FL_SHUTDOWN. Which will not allow the event to be enabled
> anymore. What do you think about this patch?

It is too late for me to even try to read this patch...

But at first glance, how this patch can protect from the subsequent
perf_trace_event_open/perf_trace_add which can use this ->tp_event?

> We set the SHUTDOWN flag, which is done under the event_mutex, and then
> we can check the trace_probe_is_enabled(), as that gets set under the
> event_mutex, and only if the SHUTDOWN flag was not previously set. If we
> fail, then we just re-enable the event and return.

Heh. As a from-time-to-time reader of this code I am not worried ;)
This doesn't look simple. And if nothing else FL_SHUTDOWN doesn't
cover perf_trace_init() ?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/