tracing: Fixes for 7.2

tracing fix for 7.2:

- Move rb_desc->nr_page_va before updating dynamic array

The rb_descr->page_va is a dynamic array counted by nr_page_va. But the updating of the page_va[] is done before the nr_page_va is incremented causing a build with CONFIG_UBSAN_BOUNDS to flag it as an overflow.

Move the increment of the counted by value before the array element is updated.

- Propagate errors from remote event bulk updates

The return value of trace_remote_enable_event() was not being checked by remote_events_dir_enable_write() where it would silently fail. Have it check the return value and propagate that back up to user space.

- Fix resource leak on mmiotrace trace_pipe close

The mmiotrace tracer was created in 2008 before the trace_pipe had a close callback to allow tracers to do clean up from trace_pipe open. The trace_pipe close cleanup callback was added in 2009 but the mmiotrace tracer was not updated. It had a hack to do the cleanup in the read call, where it may leak if user space did not read the entire buffer.

Add a callback to mmiotrace trace_pipe close do to the cleanup properly.

- Fix a possible NULL pointer dereference in the mmiotrace tracer

If the mmio_pipe_open() fails to find a PCI device, it will set the hiter->dev pointer to NULL. The read function will blindly dereference that pointer. Fix the read call to check to see if that pointer is populated before dereferencing it.

- Fix union collision of module and refcnt for dynamic events

In 'struct trace_event_call', the 'module' pointer and the 'refcnt' atomic variable share the same memory space in a union. The filter on module logic only checked if the 'module' was set to determine if the event belonged to the module. As dynamic events are always builtin, it doesn't need the 'module' field of the structure and used a refcount. But the module filtering logic would then mistaken these dynamic events as a module and call module_name(event->module) on it.

Add a check to see if the event is a dynamic event and if so, do not check it for being part of the given module.

- Reset the top level buffer in selftests before running instances

The ftracetest selftest initializes each instance before executing the tests. But it does not reset the top level buffer. Dynamic events are only added and removed by the top level so any left over dynamic events will not be removed by the reset in the instances.

Left over dynamic events can cause the tests to incorrectly fail. Reset the top level buffer before running the instances.

- Make the context_switch counter 64 bit

The code to read user space for a system call trace event or for a trace_marker will disable migration, enable preemption, read user space into a per CPU buffer, disable preemption and enable migration again. It checks if the per CPU context switch counter to see if it changed, and if it did not, it would know that the per CPU buffer was not touched by another task.

But the save counter was 32 bit and it would compare it to the 64 bit context_switch variable. A long running system could have the context_switch variable greater that 1<<32 in which case the compare will always fail. The compare will promote the 32 bit int saved value to 64 bit and compare it to the full 64 bit counter. Since the top 32 bits of the saved value was zero, it would never match.

- Fix a use-after-free of the event_enable trigger

The event_enable trigger allows for enabling one event when another event is triggered. When the trigger is removed, it must go through a synchronization phase to make sure it is not triggered again. The trigger itself is delayed by the "bulk delay" logic that was recently added. But the code that frees the event_enable data used to rely on the trigger code to do the synchronization. Now that the code uses the call RCU functions (and a workqueue), that delay no longer is there.

Add a callback private_data_free() function that allows triggers to clean up data after the synchronization phase has completed.

- Move the module_ref counter into the delay callback

Since an event of the event_enable trigger can enable an event for a module, it ups the module ref count for that event's module. This prevents the event from trying to enable an event that no longer exists and cause a use-after-free bug.

The ref counter was set back down when the trigger was removed but not after thy synchronization phase. This could lead to the module data being accessed after module was unloaded.

Move the module ref decrement into the private_data_free() callback of the event_enable trigger.

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace/fixes

Head SHA1: e091351b38818ef620d27f44f4bfd625f13afbff

David Carlier (1):
tracing: Fix use-after-free freeing trigger private data

Fuad Tabba (1):

      tracing/remotes: Fix page_va[] access before counter update in trace_remote_alloc_buffer()

Jackie Liu (1):
tracing: Propagate errors from remote event bulk updates

Masami Hiramatsu (Google) (2):
tracing: Fix union collision of module and refcnt for dynamic events selftests/ftrace: Reset triggers at top level before instance loop

Steven Rostedt (2):
tracing: Fix mmiotrace possible NULL dereferencing of hiter->dev
tracing: Delay module ref count for "enable_event" trigger

Usama Arif (1):
tracing: Fix context switch counter truncation

deepakraog (1):
tracing: Fix resource leak on mmiotrace trace_pipe close

----

 kernel/trace/trace.c                      |  2 +-
 kernel/trace/trace.h                      |  1 +
 kernel/trace/trace_events.c               |  4 +++-
 kernel/trace/trace_events_hist.c          |  2 ++
 kernel/trace/trace_events_trigger.c       | 22 ++++++++++++++++++----
 kernel/trace/trace_mmiotrace.c            |  4 ++--
 kernel/trace/trace_remote.c               | 16 +++++++++++++---
 tools/testing/selftests/ftrace/ftracetest |  1 +
 8 files changed, 41 insertions(+), 11 deletions(-)

From: Fuad Tabba <[email protected]>

page_va[] is annotated __counted_by(nr_page_va), so nr_page_va must cover an index before that element is accessed. The allocation loop writes page_va[id] while nr_page_va is still id and increments it only afterwards, so every write is one element past the declared count.

The store is out of bounds with respect to the annotation: a build with CONFIG_UBSAN_BOUNDS on a toolchain that honours __counted_by (clang >= 20.1, gcc >= 15.1) flags it as an array-index overflow.

Increment nr_page_va before writing the element it now covers. A failed allocation then leaves the slot counted but NULL; the error path frees it with free_page(0), which is a no-op.

---
 kernel/trace/trace_remote.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 0f6ef5c36d84..ef42d9c38b37 100644
@@ -1004,11 +1004,10 @@ int trace_remote_alloc_buffer(struct trace_buffer_desc *desc, size_t desc_size,
 		desc->nr_cpus++;
 
 		for (id = 0; id < nr_pages; id++) {
+			rb_desc->nr_page_va++;
 			rb_desc->page_va[id] = (unsigned long)__get_free_page(GFP_KERNEL);
 			if (!rb_desc->page_va[id])
 				goto err;
-
-			rb_desc->nr_page_va++;
 		}
 		rb_desc = __next_ring_buffer_desc(rb_desc);
 	}

2.53.0

Uggg...
Shouldn't the __counted_by() field be the size of the array itself, not the number of valid items?

David

Not always. It's simply a way to tell the compiler which field holds the number of elements for a dynamic array. Sometimes that field is used to create the array itself, but like in this case, that field is used to compare with another value that holds the size that happens not to be part of the structure.

-- Steve

From: Jackie Liu <[email protected]>

remote_events_dir_enable_write() ignores the return value from trace_remote_enable_event(). If a remote rejects an event state change, the write therefore reports success even though the affected event remains in its previous state.

Keep trying all events, but retain and return the first error. This matches __ftrace_set_clr_event_nolock(), which permits partial updates while notifying userspace when an operation fails.

---
 kernel/trace/trace_remote.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

index ef42d9c38b37..e6724f947170 100644
@@ -1149,10 +1149,21 @@ static ssize_t remote_events_dir_enable_write(struct file *filp, const char __us
 
 	for (i = 0; i < remote->nr_events; i++) {
 		struct remote_event *evt = &remote->events[i];
+		int eret;
 
-		trace_remote_enable_event(remote, evt, enable);
+		eret = trace_remote_enable_event(remote, evt, enable);
+		/*
+		 * Save the first error and return that. Some events
+		 * may still have been enabled, but let the user
+		 * know that something went wrong.
+		 */
+		if (!ret && eret)
+			ret = eret;
 	}
 
+	if (ret)
+		return ret;
+
 	return count;
 }
 

2.53.0

From: deepakraog <[email protected]>

The mmiotrace tracer was added May 12th 2008. At that time, resources created in pipe_open() could not be freed because there was not pipe_close function pointer of the tracer. The pipe_close function pointer was added in December 7th, 2009, but the mmiotrace tracer was not updated.

mmio_pipe_open() allocates a header_iter and takes a pci_dev reference when trace_pipe is opened. mmio_close() frees them, but it was only wired to the tracer's .close callback.

tracing_release_pipe() invokes .pipe_close, not .close, when the trace_pipe file is released. As a result, closing trace_pipe with the mmiotrace tracer active leaked the header_iter allocation and left a stale pci_dev reference.

Set .pipe_close to mmio_close, matching how function_graph wires both callbacks to the same handler.

Note, if the trace_pipe is read to completion, it will clean up the resources, but if one were to run:

  # head -n 1 /sys/kernel/tracing/trace_pipe
 VERSION 20070824

Over and over again, it would trigger a massive leak.

---
 kernel/trace/trace_mmiotrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 226cf66e0d68..20812e7f911f 100644
@@ -109,7 +109,6 @@ static void mmio_pipe_open(struct trace_iterator *iter)
 	iter->private = hiter;
 }
 
-/* XXX: This is not called when the pipe is closed! */
 static void mmio_close(struct trace_iterator *iter)
 {
 	struct header_iter *hiter = iter->private;

@@ -279,6 +278,7 @@ static struct tracer mmio_tracer __read_mostly =
 	.start		= mmio_trace_start,
 	.pipe_open	= mmio_pipe_open,
 	.close		= mmio_close,
+	.pipe_close	= mmio_close,
 	.read		= mmio_read,
 	.print_line	= mmio_print_line,
 	.noboot		= true,

2.53.0

From: Steven Rostedt <[email protected]>

If the mmio_pipe_open() fails to find a PCI device, the hiter->dev will be assigned to NULL. The mmiotrace read() function dereferences the hiter->dev if hiter exists.

Change the test of the read to not only check hiter being NULL, but also the hiter->dev before dereferencing it.

---
 kernel/trace/trace_mmiotrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 20812e7f911f..b88b8d9923ad 100644
@@ -145,7 +145,7 @@ static ssize_t mmio_read(struct trace_iterator *iter, struct file *filp,
 		goto print_out;
 	}
 
-	if (!hiter)
+	if (!hiter || !hiter->dev)
 		return 0;
 
 	mmio_print_pcidev(s, hiter->dev);

2.53.0

From: "Masami Hiramatsu (Google)" <[email protected]>

In 'struct trace_event_call', the 'module' pointer and the 'refcnt' atomic variable share the same memory space in a union. For dynamic events, the union member is 'refcnt', which acts as an active reference counter.

When a dynamic event (such as kprobe, uprobe, fprobe, eprobe, or wprobe) has a non-zero reference count (e.g. due to active event triggers or perf attachments), its 'call->module' evaluates to a small non-zero integer instead of NULL.

When filtering or setting events for a specific module (e.g., writing ':mod:<module>' to 'set_event'), the code in '__ftrace_set_clr_event_nolock()' and 'update_event_fields()' reads 'call->module' directly without checking whether the event is dynamic. This causes the kernel to treat the small integer (refcnt) as a 'struct module' pointer, leading to a NULL/invalid pointer dereference (Oops) when dereferencing the module name.

Fix this by ensuring that the 'TRACE_EVENT_FL_DYNAMIC' flag is checked before treating 'call->module' as a valid pointer in these code paths.

---
 kernel/trace/trace_events.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index c46e623e7e0d..956692856fa8 100644
@@ -1350,7 +1350,9 @@ __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
 		call = file->event_call;
 
 		/* If a module is specified, skip events that are not that module */
-		if (module && (!call->module || strcmp(module_name(call->module), module)))
+		if (module &&
+		    ((call->flags & TRACE_EVENT_FL_DYNAMIC) ||
+		     !call->module || strcmp(module_name(call->module), module)))
 			continue;
 
 		name = trace_event_name(call);

2.53.0

From: "Masami Hiramatsu (Google)" <[email protected]>

When running instance tests, 'ftracetest' creates a new ftrace instance and runs the tests inside it. Before starting each test, it executes 'initialize_system()' to reset the ftrace state to initial-state.

However, since 'initialize_system()' is executed in the context of the instance directory, it only cleans up triggers and filters of that instance.
Any triggers or dynamic events left behind in the top-level instance by previous failed top-level tests, are left completely untouched. These top-level leftovers can cause subsequent instance-based tests to fail or even crash the kernel.

Fix this by executing 'initialize_system()' in the top-level tracing directory once before entering the instance loop.

---
 tools/testing/selftests/ftrace/ftracetest | 1 +
 1 file changed, 1 insertion(+)

index 0a56bf209f6c..8ad2c385407e 100755
@@ -503,6 +503,7 @@ for t in $TEST_CASES; do
 done
 
 # Test on instance loop
+(cd $TRACING_DIR; initialize_system)
 INSTANCE=" (instance) "
 for t in $TEST_CASES; do
   test_on_instance $t || continue

2.53.0

From: Usama Arif <[email protected]>

trace_user_fault_read() samples nr_context_switches_cpu() before enabling preemption and retries the user copy if the counter changes. The helper returns unsigned long long because rq->nr_switches is u64, but the saved value is unsigned int.

Once a CPU has performed 2^32 context switches, assigning the counter to cnt discards its upper bits. The comparison after the copy promotes cnt back to unsigned long long, but the lost bits remain zero, so it reports a change even when the task was never scheduled out. Every retry then fails the same way until the 100-try guard warns and the user copy is abandoned.

This affects long-running systems and workloads with high context-switch rates. A CPU switching 1,000 times per second takes about 50 days.

Store the sampled count in unsigned long long so the full value is preserved.

---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 18710c190c92..01a5e87af299 100644
@@ -6187,7 +6187,7 @@ char *trace_user_fault_read(struct trace_user_buf_info *tinfo,
 {
 	int cpu = smp_processor_id();
 	char *buffer = per_cpu_ptr(tinfo->tbuf, cpu)->buf;
-	unsigned int cnt;
+	unsigned long long cnt;
 	int trys = 0;
 	int ret;
 

2.53.0

From: David Carlier <[email protected]>

Commit 61d445af0a7c ("tracing: Add bulk garbage collection of freeing event_trigger_data") moved the kfree() of event_trigger_data to a kthread that runs tracepoint_synchronize_unregister() before freeing. That removed the synchronization the trigger .free callbacks used to get implicitly and inline from trigger_data_free().

event_hist_trigger_free(), event_hist_trigger_named_free() and event_enable_trigger_free() free their satellite data (hist_data, cmd_ops, enable_data) right after trigger_data_free() returns. With the synchronization now deferred to the kthread, a concurrent tracepoint handler can still reach that data through the list_del_rcu()'d trigger, causing a use-after-free.

The histogram teardown must stay synchronous: remove_hist_vars() and unregister_field_var_hists() have to detach a synthetic event from the histogram before the trigger-removal write returns, otherwise a following command races in and the synthetic-event removal fails with -EBUSY, as the trigger-synthetic-eprobe.tc selftest catches. Make those callbacks wait with the correct barrier - tracepoint_synchronize_unregister(), matching the free kthread - before freeing.

The enable trigger has no such synchronous requirement, and a blocking synchronize there would re-serialize the path that commit deliberately deferred. Give it an optional private_data_free() callback that the free kthread runs after its grace period, and free enable_data from there.

---
 kernel/trace/trace.h                |  1 +
 kernel/trace/trace_events_hist.c    |  2 ++
 kernel/trace/trace_events_trigger.c | 18 +++++++++++++++---
 3 files changed, 18 insertions(+), 3 deletions(-)

index 80fe152af1dd..bf77331f56a4 100644
@@ -1941,6 +1941,7 @@ struct event_trigger_data {
 	struct list_head		named_list;
 	struct event_trigger_data	*named_data;
 	struct llist_node		llist;
+	void				(*private_data_free)(struct event_trigger_data *data);
 };
 
 /* Avoid typos */

index 82ce492ab268..58d28cd1afa3 100644
@@ -6349,6 +6349,7 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
 
 		trigger_data_free(data);
 
+		tracepoint_synchronize_unregister();
 		remove_hist_vars(hist_data);
 
 		unregister_field_var_hists(hist_data);

@@ -6388,6 +6389,7 @@ static void event_hist_trigger_named_free(struct event_trigger_data *data)
 
 		del_named_trigger(data);
 		trigger_data_free(data);
+		tracepoint_synchronize_unregister();
 		kfree(cmd_ops);
 	}
 }

index 655db2e82513..46e60b70a4bb 100644
@@ -38,6 +38,13 @@ static void trigger_create_kthread_locked(void)
 	}
 }
 
+static void trigger_data_free_one(struct event_trigger_data *data)
+{
+	if (data->private_data_free)
+		data->private_data_free(data);
+	kfree(data);
+}
+
 static void trigger_data_free_queued_locked(void)
 {
 	struct event_trigger_data *data, *tmp;

@@ -52,7 +59,7 @@ static void trigger_data_free_queued_locked(void)
 	tracepoint_synchronize_unregister();
 
 	llist_for_each_entry_safe(data, tmp, llnodes, llist)
-		kfree(data);
+		trigger_data_free_one(data);
 }
 
 /* Bulk garbage collection of event_trigger_data elements */

@@ -75,7 +82,7 @@ static int trigger_kthread_fn(void *ignore)
 		tracepoint_synchronize_unregister();
 
 		llist_for_each_entry_safe(data, tmp, llnodes, llist)
-			kfree(data);
+			trigger_data_free_one(data);
 	}
 
 	return 0;

@@ -1717,6 +1724,11 @@ int event_enable_trigger_print(struct seq_file *m,
 	return 0;
 }
 
+static void enable_trigger_private_data_free(struct event_trigger_data *data)
+{
+	kfree(data->private_data);
+}
+
 void event_enable_trigger_free(struct event_trigger_data *data)
 {
 	struct enable_trigger_data *enable_data = data->private_data;

@@ -1728,9 +1740,9 @@ void event_enable_trigger_free(struct event_trigger_data *data)
 	if (!data->ref) {
 		/* Remove the SOFT_MODE flag */
 		trace_event_enable_disable(enable_data->file, 0, 1);
+		data->private_data_free = enable_trigger_private_data_free;
 		trace_event_put_ref(enable_data->file->event_call);
 		trigger_data_free(data);
-		kfree(enable_data);
 	}
 }
 

2.53.0

From: Steven Rostedt <[email protected]>

Triggers are now delayed from freeing, but can still be triggered until after the RCU grace period has ended. The freeing of the enable_event data is put into the private_data_free() callback, but the put of the module refcount is done immediately.

It is possible that if a module is removed that has an event that would enable (or disable) it is still active, it can read the data of the module after it is removed causing a use-after-free bug.

Move the trace_event_put_ref() that releases the module into the delayed callback so that the module can not be removed until any reference to its events are finished.

---
 kernel/trace/trace_events_trigger.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 46e60b70a4bb..ad83419cb420 100644
@@ -1726,7 +1726,10 @@ int event_enable_trigger_print(struct seq_file *m,
 
 static void enable_trigger_private_data_free(struct event_trigger_data *data)
 {
-	kfree(data->private_data);
+	struct enable_trigger_data *enable_data = data->private_data;
+
+	trace_event_put_ref(enable_data->file->event_call);
+	kfree(enable_data);
 }
 
 void event_enable_trigger_free(struct event_trigger_data *data)

@@ -1741,7 +1744,6 @@ void event_enable_trigger_free(struct event_trigger_data *data)
 		/* Remove the SOFT_MODE flag */
 		trace_event_enable_disable(enable_data->file, 0, 1);
 		data->private_data_free = enable_trigger_private_data_free;
-		trace_event_put_ref(enable_data->file->event_call);
 		trigger_data_free(data);
 	}
 }

2.53.0