Hello.
On top of "[PATCH v2 0/6] tracing: open/delete fixes" series.
Perhaps this series should be updated, Masami has some concerns.
But it seems that you both mostly agree with these changes, so
let me send the "last" patch for review.
The last change in kernel/trace/trace_events.c we need to close the
problems with create/use/delete (at least those which I know ;).
Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven
already has the patches.
Changes:
- remove the TRACE_EVENT_FL_REF_MASK (which we do not have)
check.
We rely on the previous changes, we do not care if someone
have an opened file, event_enable_write/etc can do nothing
but fail after we do remove_event_file_dir().
- Add the comment and a note into the changelog to explain
why we still need to check FTRACE_EVENT_FL_ENABLED.
Oleg.
include/linux/ftrace_event.h | 2 +-
kernel/trace/trace_events.c | 35 +++++++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 3 deletions(-)
Change trace_remove_event_call(call) to return the error if this
call is active. This is what the callers assume but can't verify
outside of the tracing locks. Both trace_kprobe.c/trace_uprobe.c
need the additional changes, unregister_trace_probe() should abort
if trace_remove_event_call() fails.
The caller is going to free this call/file so we must ensure that
nobody can use them after trace_remove_event_call() succeeds.
debugfs should be fine after the previous changes and event_remove()
does TRACE_REG_UNREGISTER, but still there are 2 reasons why we need
the additional checks:
- There could be a perf_event(s) attached to this tp_event, so the
patch checks ->perf_refcount.
- TRACE_REG_UNREGISTER can be suppressed by FTRACE_EVENT_FL_SOFT_MODE,
so we simply check FTRACE_EVENT_FL_ENABLED protected by event_mutex.
Signed-off-by: Oleg Nesterov <[email protected]>
---
include/linux/ftrace_event.h | 2 +-
kernel/trace/trace_events.c | 35 +++++++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 4372658..f98ab06 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -332,7 +332,7 @@ extern int trace_define_field(struct ftrace_event_call *call, const char *type,
const char *name, int offset, int size,
int is_signed, int filter_type);
extern int trace_add_event_call(struct ftrace_event_call *call);
-extern void trace_remove_event_call(struct ftrace_event_call *call);
+extern int trace_remove_event_call(struct ftrace_event_call *call);
#define is_signed_type(type) (((type)(-1)) < (type)1)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 79a2743..3450496 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1709,16 +1709,47 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
destroy_preds(call);
}
+static int probe_remove_event_call(struct ftrace_event_call *call)
+{
+ struct trace_array *tr;
+ struct ftrace_event_file *file;
+
+#ifdef CONFIG_PERF_EVENTS
+ if (call->perf_refcount)
+ return -EBUSY;
+#endif
+ do_for_each_event_file(tr, file) {
+ if (file->event_call != call)
+ continue;
+ /*
+ * We can't rely on ftrace_event_enable_disable(enable => 0)
+ * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
+ * TRACE_REG_UNREGISTER.
+ */
+ if (file->flags & FTRACE_EVENT_FL_ENABLED)
+ return -EBUSY;
+ break;
+ } while_for_each_event_file();
+
+ __trace_remove_event_call(call);
+
+ return 0;
+}
+
/* Remove an event_call */
-void trace_remove_event_call(struct ftrace_event_call *call)
+int trace_remove_event_call(struct ftrace_event_call *call)
{
+ int ret;
+
mutex_lock(&trace_types_lock);
mutex_lock(&event_mutex);
down_write(&trace_event_sem);
- __trace_remove_event_call(call);
+ ret = probe_remove_event_call(call);
up_write(&trace_event_sem);
mutex_unlock(&event_mutex);
mutex_unlock(&trace_types_lock);
+
+ return ret;
}
#define for_each_event(event, start, end) \
--
1.5.5.1
(2013/07/30 2:50), Oleg Nesterov wrote:
> Change trace_remove_event_call(call) to return the error if this
> call is active. This is what the callers assume but can't verify
> outside of the tracing locks. Both trace_kprobe.c/trace_uprobe.c
> need the additional changes, unregister_trace_probe() should abort
> if trace_remove_event_call() fails.
>
> The caller is going to free this call/file so we must ensure that
> nobody can use them after trace_remove_event_call() succeeds.
> debugfs should be fine after the previous changes and event_remove()
> does TRACE_REG_UNREGISTER, but still there are 2 reasons why we need
> the additional checks:
>
> - There could be a perf_event(s) attached to this tp_event, so the
> patch checks ->perf_refcount.
>
> - TRACE_REG_UNREGISTER can be suppressed by FTRACE_EVENT_FL_SOFT_MODE,
> so we simply check FTRACE_EVENT_FL_ENABLED protected by event_mutex.
>
This looks good to me :)
Reviewed-by: Masami Hiramatsu <[email protected]>
Thanks! I'll update trace_kprobes.c too.
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---
> include/linux/ftrace_event.h | 2 +-
> kernel/trace/trace_events.c | 35 +++++++++++++++++++++++++++++++++--
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 4372658..f98ab06 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -332,7 +332,7 @@ extern int trace_define_field(struct ftrace_event_call *call, const char *type,
> const char *name, int offset, int size,
> int is_signed, int filter_type);
> extern int trace_add_event_call(struct ftrace_event_call *call);
> -extern void trace_remove_event_call(struct ftrace_event_call *call);
> +extern int trace_remove_event_call(struct ftrace_event_call *call);
>
> #define is_signed_type(type) (((type)(-1)) < (type)1)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 79a2743..3450496 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1709,16 +1709,47 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
> destroy_preds(call);
> }
>
> +static int probe_remove_event_call(struct ftrace_event_call *call)
> +{
> + struct trace_array *tr;
> + struct ftrace_event_file *file;
> +
> +#ifdef CONFIG_PERF_EVENTS
> + if (call->perf_refcount)
> + return -EBUSY;
> +#endif
> + do_for_each_event_file(tr, file) {
> + if (file->event_call != call)
> + continue;
> + /*
> + * We can't rely on ftrace_event_enable_disable(enable => 0)
> + * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
> + * TRACE_REG_UNREGISTER.
> + */
> + if (file->flags & FTRACE_EVENT_FL_ENABLED)
> + return -EBUSY;
> + break;
> + } while_for_each_event_file();
> +
> + __trace_remove_event_call(call);
> +
> + return 0;
> +}
> +
> /* Remove an event_call */
> -void trace_remove_event_call(struct ftrace_event_call *call)
> +int trace_remove_event_call(struct ftrace_event_call *call)
> {
> + int ret;
> +
> mutex_lock(&trace_types_lock);
> mutex_lock(&event_mutex);
> down_write(&trace_event_sem);
> - __trace_remove_event_call(call);
> + ret = probe_remove_event_call(call);
> up_write(&trace_event_sem);
> mutex_unlock(&event_mutex);
> mutex_unlock(&trace_types_lock);
> +
> + return ret;
> }
>
> #define for_each_event(event, start, end) \
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On Mon, 2013-07-29 at 19:50 +0200, Oleg Nesterov wrote:
>
> +static int probe_remove_event_call(struct ftrace_event_call *call)
> +{
> + struct trace_array *tr;
> + struct ftrace_event_file *file;
> +
> +#ifdef CONFIG_PERF_EVENTS
> + if (call->perf_refcount)
> + return -EBUSY;
> +#endif
> + do_for_each_event_file(tr, file) {
> + if (file->event_call != call)
> + continue;
> + /*
> + * We can't rely on ftrace_event_enable_disable(enable => 0)
> + * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
> + * TRACE_REG_UNREGISTER.
> + */
> + if (file->flags & FTRACE_EVENT_FL_ENABLED)
> + return -EBUSY;
> + break;
I'm going to modify the patch to include a comment here about the break
being used to go to the next trace_array and not leaving the loop.
-- Steve
> + } while_for_each_event_file();
> +
> + __trace_remove_event_call(call);
> +
> + return 0;
> +}
> +
> /* Remove an event_call */
> -void trace_remove_event_call(struct ftrace_event_call *call)
> +int trace_remove_event_call(struct ftrace_event_call *call)
> {
> + int ret;
> +
> mutex_lock(&trace_types_lock);
> mutex_lock(&event_mutex);
> down_write(&trace_event_sem);
> - __trace_remove_event_call(call);
> + ret = probe_remove_event_call(call);
> up_write(&trace_event_sem);
> mutex_unlock(&event_mutex);
> mutex_unlock(&trace_types_lock);
> +
> + return ret;
> }
>
> #define for_each_event(event, start, end) \
On Mon, 2013-07-29 at 19:50 +0200, Oleg Nesterov wrote:
> Hello.
>
> On top of "[PATCH v2 0/6] tracing: open/delete fixes" series.
>
> Perhaps this series should be updated, Masami has some concerns.
> But it seems that you both mostly agree with these changes, so
> let me send the "last" patch for review.
>
> The last change in kernel/trace/trace_events.c we need to close the
> problems with create/use/delete (at least those which I know ;).
>
> Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven
> already has the patches.
Can you refresh my memory. My INBOX is quite full. What were the
subjects for these patches? This fix has lots of different threads, not
to mention versions.
Thanks,
-- Steve
>
> Changes:
>
> - remove the TRACE_EVENT_FL_REF_MASK (which we do not have)
> check.
>
> We rely on the previous changes, we do not care if someone
> have an opened file, event_enable_write/etc can do nothing
> but fail after we do remove_event_file_dir().
>
> - Add the comment and a note into the changelog to explain
> why we still need to check FTRACE_EVENT_FL_ENABLED.
>
> Oleg.
>
> include/linux/ftrace_event.h | 2 +-
> kernel/trace/trace_events.c | 35 +++++++++++++++++++++++++++++++++--
> 2 files changed, 34 insertions(+), 3 deletions(-)
On Wed, 2013-07-31 at 12:50 -0400, Steven Rostedt wrote:
> On Mon, 2013-07-29 at 19:50 +0200, Oleg Nesterov wrote:
> >
> > +static int probe_remove_event_call(struct ftrace_event_call *call)
> > +{
> > + struct trace_array *tr;
> > + struct ftrace_event_file *file;
> > +
> > +#ifdef CONFIG_PERF_EVENTS
> > + if (call->perf_refcount)
> > + return -EBUSY;
> > +#endif
> > + do_for_each_event_file(tr, file) {
> > + if (file->event_call != call)
> > + continue;
> > + /*
> > + * We can't rely on ftrace_event_enable_disable(enable => 0)
> > + * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
> > + * TRACE_REG_UNREGISTER.
> > + */
> > + if (file->flags & FTRACE_EVENT_FL_ENABLED)
> > + return -EBUSY;
> > + break;
>
> I'm going to modify the patch to include a comment here about the break
> being used to go to the next trace_array and not leaving the loop.
>
Screw it, I'm going to just add a separate patch to include it.
-- Steve
On 07/31, Steven Rostedt wrote:
>
> On Mon, 2013-07-29 at 19:50 +0200, Oleg Nesterov wrote:
> >
> > Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven
> > already has the patches.
>
> Can you refresh my memory. My INBOX is quite full. What were the
> subjects for these patches?
Please see the attached mbox. And notice who is the author ;)
I also added the ack from Masami.
But if you only need the subjects:
[RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open
[RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open
Oleg.
On Wed, 2013-07-31 at 19:38 +0200, Oleg Nesterov wrote:
> On 07/31, Steven Rostedt wrote:
> >
> > On Mon, 2013-07-29 at 19:50 +0200, Oleg Nesterov wrote:
> > >
> > > Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven
> > > already has the patches.
> >
> > Can you refresh my memory. My INBOX is quite full. What were the
> > subjects for these patches?
>
> Please see the attached mbox. And notice who is the author ;)
Heh, who is that guy?
> I also added the ack from Masami.
Thanks, but I'll take in from the email as that's how my scripts work (I
cc'd myself)
-- Steve
>
> But if you only need the subjects:
>
> [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open
> [RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open
>
> Oleg.