Skip to contentSkip to conversation

tracing: Hold trace_types_lock when updating modules in persistent instances

PATCH New David Carlier 1 participant 2 messages Oct 16

Subsystems (2)
Stable branch Tracing
lore
Share & tools
lore

David Carlier

author
#0 lore raw

PATCH v2 1/2 tracing: Hold trace_types_lock when updating modules in persistent instances

trace_module_record() walks ftrace_trace_arrays from the module notifier without trace_types_lock. A concurrent rmdir of an instance removes it from the list and frees it under that lock, so a module load or unload can walk into a freed trace_array.

Fixes 5f3719f697c3 ("tracing: Update modules to persistent instances when loaded")
Cc [email protected]
Signed-off-by David Carlier <[email protected]>
---
 kernel/trace/trace.c | 2 ++
 1 file changed, 2 insertions(+)
kernel/trace/trace.c +2 -0
index e4a490d3d08c..d3ffbed9c44b 100644
@@ -9247,6 +9247,8 @@ static void trace_module_record(struct module *mod, bool add)
 	struct trace_array *tr;
 	unsigned long flags;
 
+	guard(mutex)(&trace_types_lock);
+
 	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
 		flags = tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT);
 		/* Update any persistent trace array that has already been started */
Signature · 1 line
2.55.0
author
#1 lore raw

PATCH v2 2/2 tracing: Hold trace_types_lock when resetting last boot data from readers

update_last_data_if_empty() is called from tracing_open() with O_TRUNC, tracing_read_pipe() and tracing_buffers_read() without trace_types_lock. Two readers can both pass the LAST_BOOT test and free module_delta twice, and a module notifier can update module_delta while it is being freed.

Take trace_types_lock in those paths and keep the callers that already hold it on an unlocked variant.

Fixes 32dc0042528d ("tracing: Reset last-boot buffers when reading out all cpu buffers")
Cc [email protected]
Signed-off-by David Carlier <[email protected]>
---
 kernel/trace/trace.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
kernel/trace/trace.c +16 -3
index d3ffbed9c44b..cf8663c27ae6 100644
@@ -3603,6 +3603,7 @@ static int tracing_single_release_tr(struct inode *inode, struct file *file)
 	return single_release(inode, file);
 }
 
+static bool __update_last_data_if_empty(struct trace_array *tr);
 static bool update_last_data_if_empty(struct trace_array *tr);
 
 static int tracing_open(struct inode *inode, struct file *file)

@@ -4697,7 +4698,7 @@ tracing_set_trace_read(struct file *filp, char __user *ubuf,
 int tracer_init(struct tracer *t, struct trace_array *tr)
 {
 	tracing_reset_online_cpus(&tr->array_buffer);
-	update_last_data_if_empty(tr);
+	__update_last_data_if_empty(tr);
 	return t->init(tr);
 }
 

@@ -5371,8 +5372,10 @@ static int tracing_wait_pipe(struct file *filp)
 	return 1;
 }
 
-static bool update_last_data_if_empty(struct trace_array *tr)
+static bool __update_last_data_if_empty(struct trace_array *tr)
 {
+	lockdep_assert_held(&trace_types_lock);
+
 	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
 		return false;
 

@@ -5387,6 +5390,16 @@ static bool update_last_data_if_empty(struct trace_array *tr)
 	return true;
 }
 
+static bool update_last_data_if_empty(struct trace_array *tr)
+{
+	/* Avoid the lock on the read path unless this is a last boot buffer */
+	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
+		return false;
+
+	guard(mutex)(&trace_types_lock);
+	return __update_last_data_if_empty(tr);
+}
+
 /*
  * Consumer reader.
  */

@@ -6479,7 +6492,7 @@ int tracing_set_clock(struct trace_array *tr, const char *clockstr)
 		ring_buffer_set_clock(tr->snapshot_buffer.buffer, trace_clocks[i].func);
 	tracing_reset_online_cpus(&tr->snapshot_buffer);
 #endif
-	update_last_data_if_empty(tr);
+	__update_last_data_if_empty(tr);
 
 	if (tr->scratch && !(tr->flags & TRACE_ARRAY_FL_LAST_BOOT)) {
 		struct trace_scratch *tscratch = tr->scratch;
Signature · 1 line
2.55.0