tracing/kprobes/uprobes: Fix race between opening probe event files and deleting probe

kprobe_events

kernel/trace/trace_events.c +11 -0

index 7d85429..90cf243 100644
@@ -391,6 +391,28 @@ static void __get_system_dir(struct ftrace_subsystem_dir *dir)
 	__get_system(dir->subsystem);
 }
 
+static int ftrace_event_call_get(struct ftrace_event_call *call)
+{
+	int ret = 0;
+
+	mutex_lock(&event_mutex);
+	if ((call->flags & TRACE_EVENT_FL_REF_MASK) == TRACE_EVENT_FL_REF_MAX - 1)
+		ret = -EBUSY;
+	else
+		call->flags++;
+	mutex_unlock(&event_mutex);
+

kernel/trace/trace_events.c +9 -1

@@ -424,7 +446,15 @@ static int tracing_open_generic_file(struct inode *inode, struct file *filp)
 
 	ret = tracing_open_generic(inode, filp);
 	if (ret < 0)
-		trace_array_put(tr);
+		goto fail;
+
+	ret = ftrace_event_call_get(file->event_call);
+	if (ret < 0)
+		goto fail;
+
+	return 0;
+ fail:
+	trace_array_put(tr);
 	return ret;
 }
 

Currently ftrace_open_generic_file gets an event_file from inode->i_private, and then locks event_mutex and gets refcount. However, this can cause a race as below scenario;

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)

    lock event_mutex             get event_file from inode->i_private
      event_remove()             wait for unlock event_mutex
        ...
        free event_file
    unlock event_mutex
                                 lock event_mutex
                                 add refcount of event_file->call (*)

So, at (*) point, the event_file is already freed and we may access the corrupted object.
The same thing could happen on trace_array because it is also directly accessed from event_file.

To avoid this, when opening events/*/*/enable, we must atomically do; ensure the ftrace_event_file object still exists on a trace_array, and get refcounts of event_file->call and the trace_array.

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)
lock event_mutex get event_file from inode->i_private
event_remove() wait for unlock event_mutex
...
free event_file
unlock event_mutex
lock event_mutex search the event_file and failed unlock event_mutex return -ENODEV

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace_events.c |   58 +++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 13 deletions(-)

kernel/trace/trace_events.c +45 -13

index 1a5547e..db6b107 100644
@@ -391,15 +391,24 @@ static void __get_system_dir(struct ftrace_subsystem_dir *dir)
 	__get_system(dir->subsystem);
 }
 
-static int ftrace_event_call_get(struct ftrace_event_call *call)
+static int __ftrace_event_call_get(struct ftrace_event_call *call)
 {
 	int ret = 0;
 
-	mutex_lock(&event_mutex);
 	if ((call->flags & TRACE_EVENT_FL_REF_MASK) == TRACE_EVENT_FL_REF_MAX - 1)
 		ret = -EBUSY;
 	else
 		call->flags++;
+
+	return ret;
+}
+
+static int ftrace_event_call_get(struct ftrace_event_call *call)
+{
+	int ret = 0;
+
+	mutex_lock(&event_mutex);
+	ret = __ftrace_event_call_get(call);
 	mutex_unlock(&event_mutex);
 
 	return ret;

@@ -413,6 +422,35 @@ static void ftrace_event_call_put(struct ftrace_event_call *call)
 	mutex_unlock(&event_mutex);
 }
 
+static int ftrace_event_file_get(struct ftrace_event_file *this_file)
+{
+	struct ftrace_event_file *file;
+	struct trace_array *tr;
+	int ret = -ENODEV;
+
+	mutex_lock(&event_mutex);
+	do_for_each_event_file(tr, file) {
+		if (file == this_file) {
+			ret = __ftrace_event_call_get(file->event_call);
+			if (!ret)
+				tr->ref++;
+			goto out_unlock;
+		}
+	} while_for_each_event_file();
+ out_unlock:
+	mutex_unlock(&event_mutex);
+
+	return ret;
+}
+
+static void ftrace_event_file_put(struct ftrace_event_file *file)
+{
+	struct trace_array *tr = file->tr;
+
+	ftrace_event_call_put(file->event_call);
+	trace_array_put(tr);
+}
+
 static void __put_system_dir(struct ftrace_subsystem_dir *dir)
 {
 	WARN_ON_ONCE(dir->ref_count == 0);

@@ -438,33 +476,27 @@ static void put_system(struct ftrace_subsystem_dir *dir)
 static int tracing_open_generic_file(struct inode *inode, struct file *filp)
 {
 	struct ftrace_event_file *file = inode->i_private;
-	struct trace_array *tr = file->tr;
 	int ret;
 
-	if (trace_array_get(tr) < 0)
-		return -ENODEV;
-
-	ret = tracing_open_generic(inode, filp);
+	ret = ftrace_event_file_get(file);
 	if (ret < 0)
-		goto fail;
+		return ret;
 
-	ret = ftrace_event_call_get(file->event_call);
+	ret = tracing_open_generic(inode, filp);
 	if (ret < 0)
 		goto fail;
 
 	return 0;
  fail:
-	trace_array_put(tr);
+	ftrace_event_file_put(file);
 	return ret;
 }
 
 static int tracing_release_generic_file(struct inode *inode, struct file *filp)
 {
 	struct ftrace_event_file *file = inode->i_private;
-	struct trace_array *tr = file->tr;
 
-	ftrace_event_call_put(file->event_call);
-	trace_array_put(tr);
+	ftrace_event_file_put(file);
 
 	return 0;
 }

Steven, Oleg,

I think your patches are OK, but not enough.
Here is an additional patch to fix the unsafe case which I found. Could you review this too?

Quoted 15 lines · Masami Hiramatsu

Currently ftrace_open_generic_file gets an event_file from inode->i_private, and then locks event_mutex and gets refcount. However, this can cause a race as below scenario;

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)

    lock event_mutex             get event_file from inode->i_private
      event_remove()             wait for unlock event_mutex
        ...
        free event_file
    unlock event_mutex
                                 lock event_mutex
                                 add refcount of event_file->call (*)

Quoted 7 lines

So, at (*) point, the event_file is already freed and we may access the corrupted object.
The same thing could happen on trace_array because it is also directly accessed from event_file.

To avoid this, when opening events/*/*/enable, we must atomically do; ensure the ftrace_event_file object still exists on a

trace_array,

Quoted 16 lines

and get refcounts of event_file->call and the trace_array.

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)
lock event_mutex get event_file from inode->i_private
event_remove() wait for unlock event_mutex
...
free event_file
unlock event_mutex
lock event_mutex search the event_file and failed unlock event_mutex return -ENODEV

Quoted 3 lines

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace_events.c |   58

+++++++++++++++++++++++++++++++++----------

Review

Quoted 3 lines

 1 file changed, 45 insertions(+), 13 deletions(-)

(quoted diff) +0 -0

b/kernel/trace/trace_events.c

kernel/trace/trace_events.c static void __get_system_dir(struct

Quoted 4 lines

index 1a5547e..db6b107 100644

kernel/trace/trace_events.c +0 -0

@@ -391,15 +391,24 @@ static void __get_system_dir(struct

ftrace_subsystem_dir *dir)

kernel/trace/trace_events.c static void __get_system_dir(struct ftrace_subsystem_dir *dir)

Quoted 10 lines

kernel/trace/trace_events.c +1 -2

 	__get_system(dir->subsystem);
 }
 
-static int ftrace_event_call_get(struct ftrace_event_call *call)
+static int __ftrace_event_call_get(struct ftrace_event_call *call)
 {
 	int ret = 0;
 
-	mutex_lock(&event_mutex);
 	if ((call->flags & TRACE_EVENT_FL_REF_MASK) ==

TRACE_EVENT_FL_REF_MAX - 1)

kernel/trace/trace_events.c static void ftrace_event_call_put(struct

Quoted 17 lines

kernel/trace/trace_events.c +10 -0

 		ret = -EBUSY;
 	else
 		call->flags++;
+
+	return ret;
+}
+
+static int ftrace_event_call_get(struct ftrace_event_call *call)
+{
+	int ret = 0;
+
+	mutex_lock(&event_mutex);
+	ret = __ftrace_event_call_get(call);
 	mutex_unlock(&event_mutex);
 
 	return ret;

@@ -413,6 +422,35 @@ static void ftrace_event_call_put(struct

ftrace_event_call *call)

kernel/trace/trace_events.c static void ftrace_event_call_put(struct ftrace_event_call *call)

Quoted 4 lines

kernel/trace/trace_events.c +1 -0

 	mutex_unlock(&event_mutex);
 }
 
+static int ftrace_event_file_get(struct ftrace_event_file

*this_file)

kernel/trace/trace_events.c static void put_system(struct

Quoted 32 lines

kernel/trace/trace_events.c +28 -0

+{
+	struct ftrace_event_file *file;
+	struct trace_array *tr;
+	int ret = -ENODEV;
+
+	mutex_lock(&event_mutex);
+	do_for_each_event_file(tr, file) {
+		if (file == this_file) {
+			ret = __ftrace_event_call_get(file->event_call);
+			if (!ret)
+				tr->ref++;
+			goto out_unlock;
+		}
+	} while_for_each_event_file();
+ out_unlock:
+	mutex_unlock(&event_mutex);
+
+	return ret;
+}
+
+static void ftrace_event_file_put(struct ftrace_event_file *file)
+{
+	struct trace_array *tr = file->tr;
+
+	ftrace_event_call_put(file->event_call);
+	trace_array_put(tr);
+}
+
 static void __put_system_dir(struct ftrace_subsystem_dir *dir)
 {
 	WARN_ON_ONCE(dir->ref_count == 0);

@@ -438,33 +476,27 @@ static void put_system(struct

ftrace_subsystem_dir *dir)

kernel/trace/trace_events.c static void put_system(struct · line 476

Quoted 1 line

kernel/trace/trace_events.c +0 -0

 static int tracing_open_generic_file(struct inode *inode, struct

file *filp)

Quoted 17 lines

kernel/trace/trace_events.c +3 -7

 {
 	struct ftrace_event_file *file = inode->i_private;
-	struct trace_array *tr = file->tr;
 	int ret;
 
-	if (trace_array_get(tr) < 0)
-		return -ENODEV;
-
-	ret = tracing_open_generic(inode, filp);
+	ret = ftrace_event_file_get(file);
 	if (ret < 0)
-		goto fail;
+		return ret;
 
-	ret = ftrace_event_call_get(file->event_call);
+	ret = tracing_open_generic(inode, filp);
 	if (ret < 0)

kernel/trace/trace_events.c static void put_system(struct ftrace_subsystem_dir *dir)

Quoted 10 lines

kernel/trace/trace_events.c +1 -1

 		goto fail;
 
 	return 0;
  fail:
-	trace_array_put(tr);
+	ftrace_event_file_put(file);
 	return ret;
 }
 
 static int tracing_release_generic_file(struct inode *inode, struct

file *filp)

Quoted 10 lines

kernel/trace/trace_events.c +1 -3

 {
 	struct ftrace_event_file *file = inode->i_private;
-	struct trace_array *tr = file->tr;
 
-	ftrace_event_call_put(file->event_call);
-	trace_array_put(tr);
+	ftrace_event_file_put(file);
 
 	return 0;
 }

Currently ftrace_open_generic_file gets an event_file from inode->i_private, and then locks event_mutex and gets refcount. However, this can cause a race as below scenario;

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)

    lock event_mutex             get event_file from inode->i_private
      event_remove()             wait for unlock event_mutex
        ...
        free event_file
    unlock event_mutex
                                 lock event_mutex
                                 add refcount of event_file->call (*)

So, at (*) point, the event_file is already freed and we may access the corrupted object.

Quoted 16 lines · Masami Hiramatsu

Currently ftrace_open_generic_file gets an event_file from inode->i_private, and then locks event_mutex and gets refcount. However, this can cause a race as below scenario;

CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)

    lock event_mutex             get event_file from inode->i_private
      event_remove()             wait for unlock event_mutex
        ...
        free event_file
    unlock event_mutex
                                 lock event_mutex
                                 add refcount of event_file->call (*)

Quoted 2 lines

So, at (*) point, the event_file is already freed and we may access the corrupted object.

Yes, but the same can happen with event_call, so it seems that this patch is not enough too.

Say, open(id) can take event_mutex when the caller of trace_remove_event_call() has already freed ftrace_event_call.

Or I missed something...

Perhaps we can rely on d_unlinked() ? IOW, the caller of __ftrace_event_call_get() should take event_mutex, check d_unhashed(f_dentry) and only then do _get().

Nasty, I agree.

Oleg.

To avoid this, when opening events/*/*/enable, we have to ensure the dentry of the file is not unlinked yet, under event_mutex is locked.

Quoted 4 lines · Masami Hiramatsu

To avoid this, when opening events/*/*/enable, we have to ensure the dentry of the file is not unlinked yet, under event_mutex is locked.

Probably this can work, but I am starting to think that this ref count becomes toooooo complex....

Could you please look at the draft patch I sent in another email?

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Masami Hiramatsu

To avoid this, when opening events/*/*/enable, we have to ensure the dentry of the file is not unlinked yet, under event_mutex is locked.

Quoted 2 lines

Probably this can work, but I am starting to think that this ref count becomes toooooo complex....

Oleg, I agree that is a bit complex way to fix but it can fix.

However, I think we should fix bugs first, because there are bugs. I don't like to leave a bug in the kernel as it is, while we re- start discussing new approach...

So, I'd like to suggest you getting these fixes into ftrace as first-aid treatment, and discuss your new series.

Quoted 11 lines · Masami Hiramatsu

Quoted 8 lines · Oleg Nesterov

Quoted 4 lines · Masami Hiramatsu

To avoid this, when opening events/*/*/enable, we have to ensure the dentry of the file is not unlinked yet, under event_mutex is locked.

Probably this can work, but I am starting to think that this ref count becomes toooooo complex....

Quoted 5 lines

Oleg, I agree that is a bit complex way to fix but it can fix.

However, I think we should fix bugs first, because there are bugs. I don't like to leave a bug in the kernel as it is, while we re- start discussing new approach...

And I am trying to fix the same problems, just _I think_ there is another and simpler approach.

Quoted 2 lines

So, I'd like to suggest you getting these fixes into ftrace as first-aid treatment, and discuss your new series.

Well, perhaps you are right... But this TRACE_EVENT_FL_REF_MASK code is new too, it is not that we only need a small fixlets to finish it.

So I think that it makes sense to discuss the alternatives before we decide what exactly we should do.

Quoted 2 lines · Oleg Nesterov

Well, perhaps you are right... But this TRACE_EVENT_FL_REF_MASK code is new too, it is not that we only need a small fixlets to finish it.

Would you mean that TRACE_EVENT_FL_REF_MASK may also have some problems?

Quoted 2 lines

So I think that it makes sense to discuss the alternatives before we decide what exactly we should do.

Your approach is also interesting for me, indeed. However, it is so different from current one. I think you should clarify what bug you would like to solve and how.

Quoted 5 lines · Masami Hiramatsu

Quoted 2 lines · Oleg Nesterov

Well, perhaps you are right... But this TRACE_EVENT_FL_REF_MASK code is new too, it is not that we only need a small fixlets to finish it.

Quoted 1 line

Would you mean that TRACE_EVENT_FL_REF_MASK may also have some problems?

It was you who initially pointed that it does have problems ;)

And, _afaics_ your patch which tries to fix this problem is not exactly correct.

It removes trace_array_get/put from tracing_open_generic_file() and tracing_release_generic_file(). This assumes that "call->flags++" is enough, but it is not.

Yes, the next patch adds the "flags & TRACE_EVENT_FL_REF_MASK" check into trace_remove_event_call() path. But this is still racy wrt instance_delete() unless I missed something.

IOW, I believe that either .open() should do trace_array_get(), or __trace_remove_event_dirs() needs another for-each-file loop which checks file->call->flags & TRACE_EVENT_FL_REF_MASK.

Quoted 6 lines

Quoted 2 lines

So I think that it makes sense to discuss the alternatives before we decide what exactly we should do.

Your approach is also interesting for me, indeed. However, it is so different from current one. I think you should clarify what bug you would like to solve and how.

The same bugs which Steven's 1/4 tries to solve ;)

Quoted 3 lines · Oleg Nesterov

It removes trace_array_get/put from tracing_open_generic_file() and tracing_release_generic_file(). This assumes that "call->flags++" is enough, but it is not.

No, it replaces trace_array_get/put with ftrace_event_file_get/put which calls trace_array_get/put inside.

(Just one point, previous ftrace_event_file_get has a racy point when it does tr->ref++, it should be fixed.)

Quoted 3 lines

IOW, I believe that either .open() should do trace_array_get(), or __trace_remove_event_dirs() needs another for-each-file loop which checks file->call->flags & TRACE_EVENT_FL_REF_MASK.

Agreed :)

Quoted 1 line

The same bugs which Steven's 1/4 tries to solve ;)

OK, let me confirm that, would you mean we still need 2/4 - 4/4?

Quoted 8 lines · Masami Hiramatsu

Quoted 3 lines · Oleg Nesterov

It removes trace_array_get/put from tracing_open_generic_file() and tracing_release_generic_file(). This assumes that "call->flags++" is enough, but it is not.

No, it replaces trace_array_get/put with ftrace_event_file_get/put which calls trace_array_get/put inside.

Ah, I didn't notice your patch adds "file->tr->ref++" into ftrace_event_file_get...

So I was wrong in any case, thanks for correcting me.

But,

Quoted 2 lines

(Just one point, previous ftrace_event_file_get has a racy point when it does tr->ref++, it should be fixed.)

Not sure what do you mean, but unless I missed something again this "tr->ref++" above still looks racy. instance_delete() checks tr->ref first, then it takes event_mutex and removes/kfrees event_files.

But this doesn't really matter even if I am right, surely this can be fixed. My only point, imho this is more complex than necessary.

In particular,

Quoted 5 lines

Quoted 3 lines

IOW, I believe that either .open() should do trace_array_get(), or __trace_remove_event_dirs() needs another for-each-file loop which checks file->call->flags & TRACE_EVENT_FL_REF_MASK.

Agreed :)

Yes ;) and this makes the ref-counting even more complex, we use different methods to avoid the races with rmdir and event_remove().

Quoted 3 lines

Quoted 1 line

The same bugs which Steven's 1/4 tries to solve ;)

OK, let me confirm that, would you mean we still need 2/4 - 4/4?

Yes, yes.

Quoted 1 line · Oleg Nesterov

My only point, imho this is more complex than necessary.

I see, so I'd like to see the fix. However, I'm not sure we have enough time to fix that cleanly.

Note that except
for the timing bug, we still leave a kernel bug which can easily be reproduced as Jovi reported.

Quoted 3 lines

Quoted 1 line

OK, let me confirm that, would you mean we still need 2/4 - 4/4?

Yes, yes.

And those are depends on 1/4...

Quoted 5 lines · Masami Hiramatsu

Quoted 2 lines · Oleg Nesterov

Quoted 1 line

OK, let me confirm that, would you mean we still need 2/4 - 4/4?

Quoted 3 lines

Quoted 1 line

Yes, yes.

And those are depends on 1/4...

Not at all or I missed something (quite possible). Just 2/4 should not check ->flags, of course. 3/4 looks "obviously fine", 4/4 was already merged.

From: Oleg Nesterov <[email protected]>

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.

We also check TRACE_EVENT_FL_REF_MASK to ensure that nobody opened the files we are going to remove, these means that nobody can access the soon-to-be-freed ftrace_event_file/call via filp->private_data.

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/ftrace_event.h |    2 +-
 kernel/trace/trace_events.c  |   33 +++++++++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 3 deletions(-)

include/linux/ftrace_event.h +1 -1

index 72ff2c6..bdf6bdd 100644
@@ -338,7 +338,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)
 

kernel/trace/trace_events.c +31 -2

index 90cf243..1a5547e 100644
@@ -1766,16 +1766,45 @@ 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;
+
+	if (call->flags & TRACE_EVENT_FL_REF_MASK)
+		return -EBUSY;
+
+#ifdef CONFIG_PERF_EVENTS
+	if (call->perf_refcount)
+		return -EBUSY;
+#endif
+	do_for_each_event_file(tr, file) {
+		if (file->event_call != call)
+			continue;
+		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)			\

kprobe_events

kernel/trace/trace_kprobe.c +1 -1

index 7ed6976..ffcaf42 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -340,9 +340,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 

Quoted 8 lines · Steven Rostedt

kernel/trace/trace_kprobe.c +4 -1

+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);

Quoted 2 lines

return 0;
}

This may cause an unexpected access violation at kprobe handler because unregister_probe_event frees event_call/event_files and it will be accessed until kprobe is *completely* disabled.

Actually disable_kprobe() doesn't ensure to finish the current running kprobe handlers.

Thus, even if trace_probe_is_enabled() returns false,
we must do synchronize_sched() for waiting, before unregister_probe_event().

OTOH, unregister_kprobe() waits for that.

Quoted 10 lines · Masami Hiramatsu

Quoted 8 lines · Steven Rostedt

kernel/trace/trace_kprobe.c +4 -1

+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);

Quoted 6 lines

Quoted 2 lines

return 0;
}

This may cause an unexpected access violation at kprobe handler because unregister_probe_event frees event_call/event_files and it will be accessed until kprobe is *completely* disabled.

I don't think so... Please correct me.

(but yes I think the patch needs a small update, see below).

Quoted 2 lines

Actually disable_kprobe() doesn't ensure to finish the current running kprobe handlers.

Yes. in fact disable_trace_probe(file != NULL) does, but perf doesn't.

Quoted 3 lines

Thus, even if trace_probe_is_enabled() returns false,
we must do synchronize_sched() for waiting, before unregister_probe_event().

No, I think we should simply kill trace_probe_is_enabled() here. And synchronize_sched() _before_ unregister_probe_event() can't help, exactly because trace_probe_is_enabled() is racy.

Quoted 1 line

OTOH, unregister_kprobe() waits for that.

Yes.

So I think we only need to move kfree(tp->call.print_fmt). In fact I already wrote the patch assuming that trace_remove_event_call() will be changed as we discussed.

So the sequence should be:

	if (trace_remove_event_call(...))
		return;

	/* does synchronize_sched */
	unregister_kprobe();

	kfree(everything);

Agreed?

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Masami Hiramatsu

Actually disable_kprobe() doesn't ensure to finish the current running kprobe handlers.

Yes. in fact disable_trace_probe(file != NULL) does, but perf doesn't.

Ah, right. we did that.

Quoted 5 lines

Quoted 1 line

OTOH, unregister_kprobe() waits for that.

Yes.

So I think we only need to move kfree(tp->call.print_fmt).

Quoted 8 lines

So the sequence should be:

	if (trace_remove_event_call(...))
		return;

	/* does synchronize_sched */
	unregister_kprobe();

Quoted 3 lines

	kfree(everything);

Agreed?

If we can free everything after all, I'd like to do so. Hmm, but AFAICS, trace_remove_event_call() supposes that all event is disabled completely.

A safe way is to wait rcu always right after disable_*probe in disable_trace_probe. If we have an unused link, we can free it after that.

Quoted 10 lines · Masami Hiramatsu

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines · Masami Hiramatsu

Actually disable_kprobe() doesn't ensure to finish the current running kprobe handlers.

Yes. in fact disable_trace_probe(file != NULL) does, but perf doesn't.

Ah, right. we did that.

And thus we only need to synchronize kprobe_dispatcher()->kprobe_perf_func() path. And afaics kprobe_perf_func() doesn't use anything which can be freed by trace_remove_event_call?

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

OTOH, unregister_kprobe() waits for that.

Yes.

So I think we only need to move kfree(tp->call.print_fmt).

OOPS. I was wrong. It seems that ->print_fmt is only for event/format ? Then it is fine to kfree it right after trace_remove_event_call().

Quoted 10 lines

Quoted 10 lines

So the sequence should be:

	if (trace_remove_event_call(...))
		return;

	/* does synchronize_sched */
	unregister_kprobe();

	kfree(everything);

Quoted 5 lines

Quoted 1 line

Agreed?

If we can free everything after all, I'd like to do so. Hmm, but AFAICS, trace_remove_event_call() supposes that all event is disabled completely.

Yes, but kprobe_trace_func() is really disabled?

Quoted 3 lines

A safe way is to wait rcu always right after disable_*probe in disable_trace_probe. If we have an unused link, we can free it after that.

Aaaah... I am starting to understand... Even if kprobe_perf_func() is fine, synchronize_sched() is calles _before_ disable_kprobe() and thus it can't synchronize with the handlers which hit this probe after we start synchronize_sched().

Did you mean this or I misssed something else?

(2013/07/06 2:26), Oleg Nesterov wrote:
(Perhaps, uprobes too.)

Quoted 4 lines

Quoted 3 lines

A safe way is to wait rcu always right after disable_*probe in disable_trace_probe. If we have an unused link, we can free it after that.

Quoted 6 lines

Aaaah... I am starting to understand... Even if kprobe_perf_func() is fine, synchronize_sched() is calles _before_ disable_kprobe() and thus it can't synchronize with the handlers which hit this probe after we start synchronize_sched().

Did you mean this or I misssed something else?

Right, thus perf path also need to be synchronized.

kernel/trace/trace_kprobe.c disable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file) · line 281

Quoted 8 lines · Masami Hiramatsu

kernel/trace/trace_kprobe.c +8 -0

+	if (wait) {
+		/*
+		 * synchronize with kprobe_trace_func/kretprobe_trace_func
+		 * to ensure disabled (all running handlers are finished)
+		 */
+		synchronize_sched();
+		kfree(link);	/* Ignored if link == NULL */
+	}

What's not clear to me from this comment is if we're only waiting for kfree()? In that case shouldn't we use call_rcu() to free the thing? Or do we need the sync_sched() for other purposes as well?

Quoted 13 lines · Peter Zijlstra

kernel/trace/trace_kprobe.c disable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file) · line 281

Quoted 8 lines · Masami Hiramatsu

kernel/trace/trace_kprobe.c +8 -0

+	if (wait) {
+		/*
+		 * synchronize with kprobe_trace_func/kretprobe_trace_func
+		 * to ensure disabled (all running handlers are finished)
+		 */
+		synchronize_sched();
+		kfree(link);	/* Ignored if link == NULL */
+	}

What's not clear to me from this comment is if we're only waiting for kfree()? In that case shouldn't we use call_rcu() to free the thing? Or do we need the sync_sched() for other purposes as well?

No, this is not only for kfree, but also to ensure completing disabling process, because trace_remove_event_call() supposes that for releasing event_call related objects (Those objects will be accessed in the handlers).

Quoted 15 lines · Masami Hiramatsu

Quoted 13 lines · Peter Zijlstra

kernel/trace/trace_kprobe.c disable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file) · line 281

Quoted 8 lines · Masami Hiramatsu

kernel/trace/trace_kprobe.c +8 -0

+	if (wait) {
+		/*
+		 * synchronize with kprobe_trace_func/kretprobe_trace_func
+		 * to ensure disabled (all running handlers are finished)
+		 */
+		synchronize_sched();
+		kfree(link);	/* Ignored if link == NULL */
+	}

What's not clear to me from this comment is if we're only waiting for kfree()? In that case shouldn't we use call_rcu() to free the thing? Or do we need the sync_sched() for other purposes as well?

Quoted 4 lines

No, this is not only for kfree, but also to ensure completing disabling process, because trace_remove_event_call() supposes that for releasing event_call related objects (Those objects will be accessed in the handlers).

Then may I kindly suggest you clarify this in the comment? :-)

Wait for disabling all running kprobe handlers when a kprobe event is disabled, since the caller, trace_remove_event_call() supposes that a removing event is disabled completely by disabling the event.
With this change, ftrace can ensure that there is no running event handlers after disabling it.

Changes in V2:
- Comment (in code) for clarify why we need to wait there.

Signed-off-by Masami Hiramatsu <[email protected]>

Quoted 11 lines · Masami Hiramatsu

Wait for disabling all running kprobe handlers when a kprobe event is disabled, since the caller, trace_remove_event_call() supposes that a removing event is disabled completely by disabling the event.
With this change, ftrace can ensure that there is no running event handlers after disabling it.

Changes in V2:
- Comment (in code) for clarify why we need to wait there.

Quoted 1 line

Signed-off-by Masami Hiramatsu <[email protected]>

Agreed.

We need this change in any case, whatever we do in trace/trace_events.c and in unregister_trace_probe().

Quoted 14 lines · Oleg Nesterov

Quoted 12 lines · Masami Hiramatsu

Wait for disabling all running kprobe handlers when a kprobe event is disabled, since the caller, trace_remove_event_call() supposes that a removing event is disabled completely by disabling the event.
With this change, ftrace can ensure that there is no running event handlers after disabling it.

Changes in V2:
- Comment (in code) for clarify why we need to wait there.

Signed-off-by Masami Hiramatsu <[email protected]>

Quoted 4 lines

Agreed.

We need this change in any case, whatever we do in trace/trace_events.c and in unregister_trace_probe().

Steven, I think this patch is still needed anyway even if we are discussing file handling bugs, because this fixes another timing bug.

From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

 # cd /sys/kernel/debug/tracing
 # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
 # enable_probe() {
    sleep 10
    echo 1
 }
 # file=events/kprobes/sigprocmask/enable
 # enable_probe > $file &

Quoted 1 line

kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

 # echo 'p:sigprocmask sigprocmask' > kprobe_events
 # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
 # ls events/kprobes/
enable  filter

Have the unregister probe fail when the event files are open, in use are used by perf.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_kprobe.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

kernel/trace/trace_kprobe.c +15 -6

index 7ed6976..ffcaf42 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -340,9 +340,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 
+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);
 
 	return 0;
 }

@@ -621,7 +624,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}
 

@@ -1242,11 +1247,15 @@ static int register_probe_event(struct trace_probe *tp)
 	return ret;
 }
 
-static void unregister_probe_event(struct trace_probe *tp)
+static int unregister_probe_event(struct trace_probe *tp)
 {
+	int ret;
+
 	/* tp->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tp->call);
-	kfree(tp->call.print_fmt);
+	ret = trace_remove_event_call(&tp->call);
+	if (!ret)
+		kfree(tp->call.print_fmt);
+	return ret;
 }
 
 /* Make a debugfs interface for controlling probe points */

plain text document attachment (0003-tracing-kprobes-Fail-to-unregister-if-probe-event-fi.patch)
From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

 # cd /sys/kernel/debug/tracing
 # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
 # enable_probe() {
    sleep 10
    echo 1
 }
 # file=events/kprobes/sigprocmask/enable
 # enable_probe > $file &

Quoted 1 line

kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

 # echo 'p:sigprocmask sigprocmask' > kprobe_events
 # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
 # ls events/kprobes/
enable  filter

Have the unregister probe fail when the event files are open, in use are used by perf.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_kprobe.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

kernel/trace/trace_kprobe.c +15 -6

index 7ed6976..ffcaf42 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -340,9 +340,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 
+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);
 
 	return 0;
 }

@@ -621,7 +624,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}
 

@@ -1242,11 +1247,15 @@ static int register_probe_event(struct trace_probe *tp)
 	return ret;
 }
 
-static void unregister_probe_event(struct trace_probe *tp)
+static int unregister_probe_event(struct trace_probe *tp)
 {
+	int ret;
+
 	/* tp->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tp->call);
-	kfree(tp->call.print_fmt);
+	ret = trace_remove_event_call(&tp->call);
+	if (!ret)
+		kfree(tp->call.print_fmt);
+	return ret;
 }
 
 /* Make a debugfs interface for controlling probe points */

Quoted 2 lines · Steven Rostedt

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Oleg,

The above no longer triggers the bug due to your changes. The race is much tighter now

and requires a process with the enable file opened and races with a write to enable it where the removal of the trace file checks the trace disabled, sees that it is, continues, but then the write enables it just as it gets deleted.

Do you know of a way to trigger this bug?

Hmm, what happens without this patch now? If it is active, and we delete it? It will call back into the kprobes and access a tracepoint that does not exist? Would this cause a crash?

Quoted 9 lines · Steven Rostedt

Quoted 2 lines · Steven Rostedt

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Oleg,

The above no longer triggers the bug due to your changes. The race is much tighter now

Yes, the changelog should be updated...

Quoted 4 lines

and requires a process with the enable file opened and races with a write to enable it where the removal of the trace file checks the trace disabled, sees that it is, continues, but then the write enables it just as it gets deleted.

This should be fine. Either event_remove() path takes event_mutex first and then ->write() fails, or ftrace_event_enable_disable() actually disables this even successfully.

Quoted 4 lines

This should be fine. Either event_remove() path takes event_mutex first and then ->write() fails, or ftrace_event_enable_disable() actually disables this even successfully.

Actually I meant while in unregister_trace_probe(), it gets by the trace_probe_is_enabled() part first, then the write succeeds (as the event_mutex isn't taken till unregister_probe_event()). The the unregister_probe_event fails, but the tp was freed. The event files still reference the tp and this is where a crash can happen without this patch set.

From 40c32592668b727cbfcf7b1c0567f581bd62a5e4 Mon Sep 17 00:00:00 2001

Here's the new change log, but the same patch. Does this sound ok to you guys?

-- Steve

Quoted 1 line

From 40c32592668b727cbfcf7b1c0567f581bd62a5e4 Mon Sep 17 00:00:00 2001

From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 3 Jul 2013 23:33:50 -0400
Subject: [PATCH] tracing/kprobes: Fail to unregister if probe event files are
in use

When a probe is being removed, it cleans up the event files that correspond to the probe. But there is a race between writing to one of these files and deleting the probe. This is especially true for the "enable" file.

	CPU 0				CPU 1
	-----				-----

				  fd = open("enable",O_WRONLY);

probes_open()
release_all_trace_probes()
unregister_trace_probe()
if (trace_probe_is_enabled(tp))

	return -EBUSY

				   write(fd, "1", 1)
				   __ftrace_set_clr_event()
				   call->class->reg()
				    (kprobe_register)
				     enable_trace_probe(tp)

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

				   write(fd, "0", 1)
				   __ftrace_set_clr_event()
				   call->class->unreg
				    (kprobe_register)
				    disable_trace_probe(tp) <-- BOOM!

A test program was written that used two threads to simulate the above scenario adding a nanosleep() interval to change the timings and after several thousand runs, it was able to trigger this bug and crash:

BUG: unable to handle kernel paging request at 00000005000000f9
IP: [<ffffffff810dee70>] probes_open+0x3b/0xa7
PGD 7808a067 PUD 0
Oops: 0000 [#1] PREEMPT SMP
Dumping ftrace buffer:
---------------------------------
Modules linked in: ipt_MASQUERADE sunrpc ip6t_REJECT nf_conntrack_ipv6
CPU: 1 PID: 2070 Comm: test-kprobe-rem Not tainted 3.11.0-rc3-test+ #47
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS SDBLI944.86P 05/08/2007
task: ffff880077756440 ti: ffff880076e52000 task.ti: ffff880076e52000
RIP: 0010:[<ffffffff810dee70>]  [<ffffffff810dee70>] probes_open+0x3b/0xa7
RSP: 0018:ffff880076e53c38  EFLAGS: 00010203
RAX: 0000000500000001 RBX: ffff88007844f440 RCX: 0000000000000003
RDX: 0000000000000003 RSI: 0000000000000003 RDI: ffff880076e52000
RBP: ffff880076e53c58 R08: ffff880076e53bd8 R09: 0000000000000000
R10: ffff880077756440 R11: 0000000000000006 R12: ffffffff810dee35
R13: ffff880079250418 R14: 0000000000000000 R15: ffff88007844f450
FS:  00007f87a276f700(0000) GS:ffff88007d480000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000005000000f9 CR3: 0000000077262000 CR4: 00000000000007e0
Stack:
 ffff880076e53c58 ffffffff81219ea0 ffff88007844f440 ffffffff810dee35
 ffff880076e53ca8 ffffffff81130f78 ffff8800772986c0 ffff8800796f93a0
 ffffffff81d1b5d8 ffff880076e53e04 0000000000000000 ffff88007844f440
Call Trace:
 [<ffffffff81219ea0>] ? security_file_open+0x2c/0x30
 [<ffffffff810dee35>] ? unregister_trace_probe+0x4b/0x4b
 [<ffffffff81130f78>] do_dentry_open+0x162/0x226
 [<ffffffff81131186>] finish_open+0x46/0x54
 [<ffffffff8113f30b>] do_last+0x7f6/0x996
 [<ffffffff8113cc6f>] ? inode_permission+0x42/0x44
 [<ffffffff8113f6dd>] path_openat+0x232/0x496
 [<ffffffff8113fc30>] do_filp_open+0x3a/0x8a
 [<ffffffff8114ab32>] ? __alloc_fd+0x168/0x17a
 [<ffffffff81131f4e>] do_sys_open+0x70/0x102
 [<ffffffff8108f06e>] ? trace_hardirqs_on_caller+0x160/0x197
 [<ffffffff81131ffe>] SyS_open+0x1e/0x20
 [<ffffffff81522742>] system_call_fastpath+0x16/0x1b
Code: e5 41 54 53 48 89 f3 48 83 ec 10 48 23 56 78 48 39 c2 75 6c 31 f6 48 c7
RIP  [<ffffffff810dee70>] probes_open+0x3b/0xa7
 RSP <ffff880076e53c38>
CR2: 00000005000000f9
---[ end trace 35f17d68fc569897 ]---

The unregister_trace_probe() must be done first, and if it fails it must fail the removal of the kprobe.

Several changes have already been made by Oleg Nesterov and Masami Hiramatsu to allow moving the unregister_probe_event() before the removal of the probe and exit the function if it fails. This prevents the tp structure from being used after it is freed.

Link http://lkml.kernel.org/r/[email protected] open here

Acked-by Masami Hiramatsu <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_kprobe.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

kernel/trace/trace_kprobe.c +15 -6

index 3811487..243f683 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -351,9 +351,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 
+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);
 
 	return 0;
 }

@@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}
 

@@ -1247,11 +1252,15 @@ static int register_probe_event(struct trace_probe *tp)
 	return ret;
 }
 
-static void unregister_probe_event(struct trace_probe *tp)
+static int unregister_probe_event(struct trace_probe *tp)
 {
+	int ret;
+
 	/* tp->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tp->call);
-	kfree(tp->call.print_fmt);
+	ret = trace_remove_event_call(&tp->call);
+	if (!ret)
+		kfree(tp->call.print_fmt);
+	return ret;
 }
 
 /* Make a debugfs interface for controlling probe points */

Subject: [PATCH] tracing/kprobes: Fail to unregister if probe event files are
in use

When a probe is being removed, it cleans up the event files that correspond to the probe. But there is a race between writing to one of these files and deleting the probe. This is especially true for the "enable" file.

	CPU 0				CPU 1
	-----				-----

				  fd = open("enable",O_WRONLY);

probes_open()
release_all_trace_probes()
unregister_trace_probe()
if (trace_probe_is_enabled(tp))

	return -EBUSY

				   write(fd, "1", 1)
				   __ftrace_set_clr_event()
				   call->class->reg()
				    (kprobe_register)
				     enable_trace_probe(tp)

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

				   write(fd, "0", 1)

And just in case. I believe that the patch is fine.

Just one off-topic note,

kernel/trace/trace_kprobe.c static int release_all_trace_probes(void) · line 642

Quoted 10 lines

kernel/trace/trace_kprobe.c +3 -1

@@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}

This obviously breaks all-or-nothing semantics (I mean, this breaks the intent, the current code is buggy).

I think we can't avoid this, and I hope this is fine. But then perhaps we should simply remove the "list_for_each_entry" check above?

Quoted 25 lines · Steven Rostedt

Subject: [PATCH] tracing/kprobes: Fail to unregister if probe event files are
in use

When a probe is being removed, it cleans up the event files that correspond to the probe. But there is a race between writing to one of these files and deleting the probe. This is especially true for the "enable" file.

	CPU 0				CPU 1
	-----				-----

				  fd = open("enable",O_WRONLY);

probes_open()
release_all_trace_probes()
unregister_trace_probe()
if (trace_probe_is_enabled(tp))

	return -EBUSY

				   write(fd, "1", 1)
				   __ftrace_set_clr_event()
				   call->class->reg()
				    (kprobe_register)
				     enable_trace_probe(tp)

Quoted 4 lines

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

Yes. But again, this doesn't explain why unregister_probe_event()-> __trace_remove_event_call() can't simply proceed and
do ftrace_event_enable_disable() + remove_event_from_tracers().

IOW, if we do not apply the previous "trace_remove_event_call() should fail if call/file is in use" patch, then everything is fine:

Quoted 1 line

				   write(fd, "0", 1)

this will fail with ENODEV.

Let's consider another race:

	CPU 0					CPU 1
	-----					-----

	probes_open()
	trace_probe_is_enabled() == F;

						sys_perf_event_open(attr.config == id)


	...
	trace_remove_event_call()

Whatever we do, we should abort. We could even do TRACE_REG_PERF_UNREGISTER (although the current code doesn't do this), but we have no way to cleanup the perf event's which have ->>tp_event = call.

trace_remove_event_call() was already changed to return the error.

And. Since it can fail, this obviously means that it should be checked, we can't blindly do free_trace_probe().

IOW, the changelog could be very simple, I think. Either trace_remove_event_call() should always succeed or we should check the possible failure.

But I won't argue with this changelog. The only important thing is that we all seem to agree that we do have the races here which can be fixed by this and the previous change.

And just in case. I believe that the patch is fine.

Just one off-topic note,

kernel/trace/trace_kprobe.c static int release_all_trace_probes(void) · line 642

Quoted 10 lines

kernel/trace/trace_kprobe.c +3 -1

@@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}

This obviously breaks all-or-nothing semantics (I mean, this breaks the intent, the current code is buggy).

I think we can't avoid this, and I hope this is fine. But then perhaps we should simply remove the "list_for_each_entry" check above?

Quoted 6 lines · Oleg Nesterov

Quoted 5 lines

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

Quoted 3 lines

Yes. But again, this doesn't explain why unregister_probe_event()-> __trace_remove_event_call() can't simply proceed and
do ftrace_event_enable_disable() + remove_event_from_tracers().

The problem is with the soft disable.

so the
i_private wont work.

Quoted 6 lines

IOW, if we do not apply the previous "trace_remove_event_call() should fail if call/file is in use" patch, then everything is fine:

Quoted 1 line

				   write(fd, "0", 1)

this will fail with ENODEV.

Currently it does not, because the failure in probe_remove_event_call() due to the event being busy wont remove the event (event_remove() is never called). Thus the event is still alive and the write will still have access to it.

I can update the change log to remove some of the functions that are being called to be less confusing.

I agree, this isn't really nice, but for now we have to deal with it.

Quoted 8 lines · Steven Rostedt

Quoted 6 lines · Oleg Nesterov

Quoted 5 lines

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

Quoted 5 lines

Quoted 3 lines

Yes. But again, this doesn't explain why unregister_probe_event()-> __trace_remove_event_call() can't simply proceed and
do ftrace_event_enable_disable() + remove_event_from_tracers().

The problem is with the soft disable.

Exactly! This is another (also unlikely) race we need to prevent.

Quoted 13 lines · Oleg Nesterov

Quoted 12 lines · Steven Rostedt

Quoted 9 lines · Oleg Nesterov

Quoted 5 lines

  __unregister_trace_probe(tp);
  list_del(&tp->list)
  unregister_probe_event(tp) <-- fails!
  free_trace_probe(tp)

Yes. But again, this doesn't explain why unregister_probe_event()-> __trace_remove_event_call() can't simply proceed and
do ftrace_event_enable_disable() + remove_event_from_tracers().

Quoted 4 lines

Quoted 1 line

The problem is with the soft disable.

Exactly! This is another (also unlikely) race we need to prevent.

Is there a race even with these patches?

I don't see one.

Or are you just saying that these patches fix that case too?

Just one off-topic note,

Quoted 10 lines

kernel/trace/trace_kprobe.c static int release_all_trace_probes(void) · line 642

Quoted 10 lines

kernel/trace/trace_kprobe.c +3 -1

@@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}

This obviously breaks all-or-nothing semantics (I mean, this breaks the intent, the current code is buggy).

I think we can't avoid this, and I hope this is fine. But then perhaps we should simply remove the "list_for_each_entry" check above?

Quoted 4 lines · Oleg Nesterov

This should be fine. Either event_remove() path takes event_mutex first and then ->write() fails, or ftrace_event_enable_disable() actually disables this even successfully.

Actually I meant while in unregister_trace_probe(), it gets by the trace_probe_is_enabled() part first, then the write succeeds (as the event_mutex isn't taken till unregister_probe_event()). The the unregister_probe_event fails,

plain text document attachment (0004-tracing-uprobes-Fail-to-unregister-if-probe-event-fi.patch)
From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

Have the unregister probe fail when the event files are open, in use are used by perf.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_uprobe.c |   48 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 12 deletions(-)

kernel/trace/trace_uprobe.c +36 -12

index d5d0cd3..4916da5 100644
@@ -70,7 +70,7 @@ struct trace_uprobe {
 	(sizeof(struct probe_arg) * (n)))
 
 static int register_uprobe_event(struct trace_uprobe *tu);
-static void unregister_uprobe_event(struct trace_uprobe *tu);
+static int unregister_uprobe_event(struct trace_uprobe *tu);
 
 static DEFINE_MUTEX(uprobe_lock);
 static LIST_HEAD(uprobe_list);

@@ -164,11 +164,17 @@ static struct trace_uprobe *find_probe_event(const char *event, const char *grou
 }
 
 /* Unregister a trace_uprobe and probe_event: call with locking uprobe_lock */
-static void unregister_trace_uprobe(struct trace_uprobe *tu)
+static int unregister_trace_uprobe(struct trace_uprobe *tu)
 {
+	int ret;
+
+	ret = unregister_uprobe_event(tu);
+	if (ret)
+		return ret;
+
 	list_del(&tu->list);
-	unregister_uprobe_event(tu);
 	free_trace_uprobe(tu);
+	return 0;
 }
 
 /* Register a trace_uprobe and probe_event */

@@ -181,9 +187,12 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
 
 	/* register as an event */
 	old_tp = find_probe_event(tu->call.name, tu->call.class->system);
-	if (old_tp)
+	if (old_tp) {
 		/* delete old event */
-		unregister_trace_uprobe(old_tp);
+		ret = unregister_trace_uprobe(old_tp);
+		if (ret)
+			goto end;
+	}
 
 	ret = register_uprobe_event(tu);
 	if (ret) {

@@ -256,6 +265,8 @@ static int create_trace_uprobe(int argc, char **argv)
 		group = UPROBE_EVENT_SYSTEM;
 
 	if (is_delete) {
+		int ret;
+
 		if (!event) {
 			pr_info("Delete command needs an event name.\n");
 			return -EINVAL;

@@ -269,9 +280,9 @@ static int create_trace_uprobe(int argc, char **argv)
 			return -ENOENT;
 		}
 		/* delete an event */
-		unregister_trace_uprobe(tu);
+		ret = unregister_trace_uprobe(tu);
 		mutex_unlock(&uprobe_lock);
-		return 0;
+		return ret;
 	}
 
 	if (argc < 2) {

@@ -408,16 +419,20 @@ fail_address_parse:
 	return ret;
 }
 
-static void cleanup_all_probes(void)
+static int cleanup_all_probes(void)
 {
 	struct trace_uprobe *tu;
+	int ret = 0;
 
 	mutex_lock(&uprobe_lock);
 	while (!list_empty(&uprobe_list)) {
 		tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
-		unregister_trace_uprobe(tu);
+		ret = unregister_trace_uprobe(tu);
+		if (ret)
+			break;
 	}
 	mutex_unlock(&uprobe_lock);
+	return ret;
 }
 
 /* Probes listing interfaces */

@@ -462,8 +477,12 @@ static const struct seq_operations probes_seq_op = {
 
 static int probes_open(struct inode *inode, struct file *file)
 {
+	int ret = 0;
+
 	if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC))
-		cleanup_all_probes();
+		ret = cleanup_all_probes();
+	if (ret)
+		return ret;
 
 	return seq_open(file, &probes_seq_op);
 }

@@ -970,12 +989,17 @@ static int register_uprobe_event(struct trace_uprobe *tu)
 	return ret;
 }
 
-static void unregister_uprobe_event(struct trace_uprobe *tu)
+static int unregister_uprobe_event(struct trace_uprobe *tu)
 {
+	int ret;
+
 	/* tu->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tu->call);
+	ret = trace_remove_event_call(&tu->call);
+	if (ret)
+		return ret;
 	kfree(tu->call.print_fmt);
 	tu->call.print_fmt = NULL;
+	return 0;
 }
 
 /* Make a trace interface for controling probe points */

From: "Steven Rostedt (Red Hat)" <[email protected]>

Modules do with with the module owner set (automated from the VFS layer).

The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened

kernel/trace/trace_uprobe.c +2 -1

-static void cleanup_all_probes(void)
+static int cleanup_all_probes(void)
 {
 	struct trace_uprobe *tu;
+	int ret = 0;
 
 	mutex_lock(&uprobe_lock);

Quoted 2 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

Acked-by Oleg Nesterov <[email protected]>

Just a couple of nits in the case you are going to redo this change,

Quoted 2 lines

Modules do with with the module owner set (automated from the VFS layer).

This logic is dead, I think.

Quoted 2 lines

The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened

This is too.

Quoted 15 lines

kernel/trace/trace_uprobe.c +5 -2

-static void cleanup_all_probes(void)
+static int cleanup_all_probes(void)
 {
 	struct trace_uprobe *tu;
+	int ret = 0;
 
 	mutex_lock(&uprobe_lock);
 	while (!list_empty(&uprobe_list)) {
 		tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
-		unregister_trace_uprobe(tu);
+		ret = unregister_trace_uprobe(tu);
+		if (ret)
+			break;
 	}
 	mutex_unlock(&uprobe_lock);

Review

Quoted 2 lines

(quoted diff) +1 -0

+	return ret;
 }

Again, it is not clear what exactly we should do and I won't argue either way. But note that (with or without this patch) this doesn't match kprobe's release_all_trace_probes() which checks (tries to, actually) trace_probe_is_enabled() for every probe first. Perhaps we should cleanup this later.

kernel/trace/trace_uprobe.c static const struct seq_operations probes_seq_op = { · line 485

Quoted 10 lines

static int probes_open(struct inode *inode, struct file *file) {

kernel/trace/trace_uprobe.c +5 -1

+	int ret = 0;
+
 	if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC))
-		cleanup_all_probes();
+		ret = cleanup_all_probes();
+	if (ret)
+		return ret;

Cosmetic, but perhaps it would be a bit more clean to move this check (with "int ret") under if (WRITE && TRUNC) block.

Currently there exists a race with deleting a kprobe or uprobe and a user opening the probe event file or using perf events.

The problem stems from not being able to take the probe_lock from the unregister code because we may have the event_mutex at the time, and the event mutex may be taken with the probe_lock held.

To solve this, the events get a ref count (using the flags field), where when an event file is opened, the ftrace_event_call ref count increments. Then this is checked under event_mutex and if set, the unregistering of the probe will fail.

Here's a test that shows how things break:

     # cd /sys/kernel/debug/tracing
     # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
     # enable_probe() {
        sleep 10
        echo 1
     }
     # file=events/kprobes/sigprocmask/enable
     # enable_probe > $file &
     > kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

# echo 'p:sigprocmask sigprocmask' > kprobe_events # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
# ls events/kprobes/
enable filter

After applying these patches, the "> kprobe_events" fails due to the event being busy.

Masami, please review these patches and give your ack.

Oleg Nesterov (1):
tracing: trace_remove_event_call() should fail if call/file is in use

Steven Rostedt (Red Hat) (3):
tracing: Add ref count to ftrace_event_call tracing/kprobes: Fail to unregister if probe event files are open tracing/uprobes: Fail to unregister if probe event files are open

Quoted 4 lines

Steven Rostedt (Red Hat) (3):
tracing: Add ref count to ftrace_event_call tracing/kprobes: Fail to unregister if probe event files are open tracing/uprobes: Fail to unregister if probe event files are open

I just started to look into the series, but the 3/4 and 4/4 seems same... Which one is good to go?

Currently there exists a race with deleting a kprobe or uprobe and a user opening the probe event file or using perf events.

The problem stems from not being able to take the probe_lock from the unregister code because we may have the event_mutex at the time, and the event mutex may be taken with the probe_lock held.

To solve this, the events get a ref count (using the flags field), where when an event file is opened, the ftrace_event_call ref count increments. Then this is checked under event_mutex and if set, the unregistering of the probe will fail.

Here's a test that shows how things break:

     # cd /sys/kernel/debug/tracing
     # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
     # enable_probe() {
        sleep 10
        echo 1
     }
     # file=events/kprobes/sigprocmask/enable
     # enable_probe > $file &
     > kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

# echo 'p:sigprocmask sigprocmask' > kprobe_events # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
# ls events/kprobes/
enable filter

After applying these patches, the "> kprobe_events" fails due to the event being busy.

Masami, please review these patches and give your ack.

Steve, Oleg, could you also take a look on my additional 2 patches too?

Currently there exists a race with deleting a kprobe or uprobe and a user opening the probe event file or using perf events.

The problem stems from not being able to take the probe_lock from the unregister code because we may have the event_mutex at the time, and the event mutex may be taken with the probe_lock held.

To solve this, the events get a ref count (using the flags field), where when an event file is opened, the ftrace_event_call ref count increments. Then this is checked under event_mutex and if set, the unregistering of the probe will fail.

So. As Masami pointed out, this is not enough. Probably we can add more hacks, but I'd like to discuss the alternative approach.

Note also that this ref count has the unfortunate property, if someone keeps the file opened we can't remove an event.

What this patch does:

- add the new "topmost" rw_semaphore, file_sem.

- trace_remove_event_call() takes this sem for writing and cleares enable/id/format/filter->i_private

- removes tracing_open_generic_file/tracing_release_generic_file, we do not use file->f_private any longer

- changes event_enable_read/event_enable_write to read ftrace_event_file = i_private under read_lock(file_sem) and abort if it is null.

Question: why event_enable_write() needs trace_array_get() ?

Steven, Masami, do you think this can make any sense?

Quoted 6 lines · Oleg Nesterov

So. As Masami pointed out, this is not enough. Probably we can add more hacks, but I'd like to discuss the alternative approach.

Note also that this ref count has the unfortunate property, if someone keeps the file opened we can't remove an event.

And please correct me, but afaics there are similar problems with rmdir instances/xxx.

Quoted 4 lines

What this patch does:

- add the new "topmost" rw_semaphore, file_sem.

probably unneeded...

Quoted 6 lines

	- trace_remove_event_call() takes this sem for writing and
	  cleares enable/id/format/filter->i_private

	- removes tracing_open_generic_file/tracing_release_generic_file,
	  we do not use file->f_private any longer

Quoted 5 lines

- changes event_enable_read/event_enable_write to read ftrace_event_file = i_private under read_lock(file_sem) and abort if it is null.

Question: why event_enable_write() needs trace_array_get() ?

probably it doesn't...

Quoted 2 lines

Steven, Masami, do you think this can make any sense?

Quoted 5 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

So. As Masami pointed out, this is not enough. Probably we can add more hacks, but I'd like to discuss the alternative approach.

Quoted 5 lines

Quoted 2 lines

Note also that this ref count has the unfortunate property, if someone keeps the file opened we can't remove an event.

And please correct me, but afaics there are similar problems with rmdir instances/xxx.

The instances have their own refcount. And if a file is open, it wont remove the directory. Just try it...

Quoted 3 lines

Quoted 1 line

Question: why event_enable_write() needs trace_array_get() ?

probably it doesn't...

I'm confused. Which code has event_enable_write doing a trace_array_get()?

I need to sort these patches out. They are all over the place. What exactly do we have for a solution here. Did we figure out which patches are needed?

From Jovi, from me. I see nothing new in linux-trace.git#for-next...

But tracing_open_generic_file()->trace_array_get() is racy, no? Somehow we need to ensure it is safe to use file / file->tr.

Btw, Steven, what about other pending (and orthogonal changes) ?

Quoted 1 line

From Jovi, from me. I see nothing new in linux-trace.git#for-next...

10,000 emails, and it's just getting worse.

Quoted 3 lines

Btw, Steven, what about other pending (and orthogonal changes) ?

Quoted 1 line

From Jovi, from me. I see nothing new in linux-trace.git#for-next...

My INBOX is full with too much crap. I thought I grabbed the patches from you and Jovi, but they may have been something else.

Can you just tell me what the subjects were. My INBOX currently has

Quoted 1 line

10,000 emails, and it's just getting worse.

Quoted 8 lines · Steven Rostedt

Quoted 2 lines

Btw, Steven, what about other pending (and orthogonal changes) ?

Quoted 1 line

From Jovi, from me. I see nothing new in linux-trace.git#for-next...

My INBOX is full with too much crap. I thought I grabbed the patches from you and Jovi, but they may have been something else.

Can you just tell me what the subjects were. My INBOX currently has

Quoted 1 line

10,000 emails, and it's just getting worse.

From aa40385bba7692f4b53572da24119c3b293c9aa1 Mon Sep 17 00:00:00 2001

From 803ce880302ef7d21bdad2196cddc76b4c1a49a1 Mon Sep 17 00:00:00 2001

From 6037b2b9af2e6122380e967406f89ae9410fc5d6 Mon Sep 17 00:00:00 2001