tracing: Simplify trace_array_get()

trace_array_get() scans the global ftrace_trace_arrays list to ensure that "this_tr" was not removed by instance_delete().

This looks a bit confusing, we can simply use list_empty() with the same result. list_empty() == F can not be false positive, new_instance_create() does everything under the same lock and tracer_alloc_buffers() which plays with global_trace is __init.

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

---
 kernel/trace/trace.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

kernel/trace/trace.c +0 -1

index 0cd500b..ee471fb 100644
@@ -206,16 +206,12 @@ LIST_HEAD(ftrace_trace_arrays);
 
 int trace_array_get(struct trace_array *this_tr)
 {
-	struct trace_array *tr;

kernel/trace/trace.c +3 -1

+		this_tr->ref++;
+		ret = 0;
 	}
 	mutex_unlock(&trace_types_lock);
 

@@ -6019,7 +6015,7 @@ static int instance_delete(const char *name)
 	if (tr->ref)
 		goto out_unlock;
 
-	list_del(&tr->list);
+	list_del_init(&tr->list); /* trace_array_get() checks list_empty() */
 
 	event_trace_del_tracer(tr);
 	debugfs_remove_recursive(tr->dir);

kernel/trace/trace.c LIST_HEAD(ftrace_trace_arrays); · line 212

Quoted 8 lines · Oleg Nesterov

kernel/trace/trace.c +1 -6

 	mutex_lock(&trace_types_lock);
-	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
-		if (tr == this_tr) {
-			tr->ref++;
-			ret = 0;
-			break;
-		}
+	if (!list_empty(&this_tr->list)) {

Because this_tr can be freed outside the lock. Accessing this_tr->list can cause a crash.

Quoted 13 lines · Steven Rostedt

kernel/trace/trace.c LIST_HEAD(ftrace_trace_arrays); · line 212

Quoted 8 lines · Oleg Nesterov

kernel/trace/trace.c +1 -6

 	mutex_lock(&trace_types_lock);
-	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
-		if (tr == this_tr) {
-			tr->ref++;
-			ret = 0;
-			break;
-		}
+	if (!list_empty(&this_tr->list)) {

Because this_tr can be freed outside the lock. Accessing this_tr->list can cause a crash.

Aaah, indeed.

Thanks Steven!

Yes. But unless I missed something again this logic doesn't look exactly correct. Because it seems that trace_array_get() can succeed when it shoudn't.

trace_array_get() can race with instance_delete() + new_instance_create(), and _create()->kzalloc() can return the same memory which was freed by _delete().

No?

What would happen in that case, is that an event might be enabled or disabled in another buffer instance. As that can only happen by the root user, it would be the root user doing multiple things at the same time to cause it. They might get a strange result at worse, but that would also mean they were trying to add and delete instances while reading those same instances. Bad root, bad!

Quoted 4 lines · Oleg Nesterov

Yes. But unless I missed something again this logic doesn't look exactly correct. Because it seems that trace_array_get() can succeed when it shoudn't.

Quoted 5 lines

trace_array_get() can race with instance_delete() + new_instance_create(), and _create()->kzalloc() can return the same memory which was freed by _delete().

No?

Correct, but I don't see that as a major problem, do you?

Quoted 2 lines · Steven Rostedt

Correct, but I don't see that as a major problem, do you?

Neither me. I should have mentioned this.

Except "might get a strange result" as you said.

But I have to admit, I am still trying to find something really bad ;)