tracing: Fixes for 7.2

tracing fixes for 7.2:

- Fix use-after-free in eventfs_remove_rec()

The freeing of the eventfs_inode children used list_for_each_entry() where the child is freed via srcu, but there's still a chance that it gets freed. It should be using list_for_each_entry_safe().

- Fix eventfs_inode SRCU use of list in freeing

The iterator uses an SRCU protected list walk on the eventfs inodes. The eventfs inode uses its "list" field in a union with the RCU list head. When the inode gets added to the SRCU list it immediately corrupts the list pointer and can cause an issue with the iterator. Move the RCU list head to be shared with the children list head which allows the iterator to check the parent inode if is freed before referencing the child. Have the iterator check the parent "is_freed" field and break out if it is set. Also add memory barriers to make sure the ordering is correct.

- Fix various RCU synchronization issues with direct_functions

Updates to direct_functions have some missing RCU protection and synchronization. Restructure the code a bit to make sure updates to the direct_functions are protected.

- Remove an unneeded comma from a scope_guard()

There's a spurious comma in a scope_guard(). Remove it.

- Fix race in per CPU buffer swap in the ring buffer

When a per CPU buffer swap happens, it must make sure that it doesn't occur while a writer is active. Instead it returns an -EBUSY. But there's a small race window when a writer moves from one sub-buffer to the next that it resets the "committing" counter. If a swap happens at that moment, the buffer used for the commit of an event will not match the buffer the event is actually on. Instead of using the "committing" counter, use the recursive detection counter that does not get reset when the writer crosses sub-buffers.

- Fix off-by-one in ftrace_free_mem()

The function ftrace_free_mem() gets an "end_ptr" as a parameter that is exclusive to the rang to be freed. But its value is used to search for the records that expects an inclusive value. Subtract one from the parameter to convert it to an inclusive range.

- Disable resizing of the ring buffer for persistent buffers

Resizing the persistent buffer has undefined behavior. Prevent it from being resized.

- Disable changing ring buffer subbuf order when resizing is disabled

The ring buffer subbuffer order can not be changed during resizing. Use that instead of just checking if the buffer is mapped as mapped buffers also have resizing disabled.

- Initialize subbuf_order of reader pages when they are created

In rb_allocate_cpu_buffer() the bpage->order is not updated to the current subbuf_order leaving it as zero. This value is used when the page is freed.

- Fix test_ringbuffer() to test for ERR_PTR before calling kthread_stop()

The rb_threads[] array is assigned the output of kthread_run_on_cpu() which could return an ERR_PTR. At the end of the test, all threads in the array are cleaned up by kthread_stop() passing in the value in the array if it isn't zero. But if the array contains an ERR_PTR, kthread_stop() will not be able to handle it properly.

Please pull the latest trace/fixes tree, which can be found at:

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

Head SHA1: 91542863abade2fd4f2b361991f5386ad9d19c8c

Hui Su (1):
ring-buffer: Fix crash passing ERR_PTR to kthread_stop()

Josh Poimboeuf (1):
ftrace: Fix off-by-one fentry site disable in ftrace_free_mem()

Leon Hwang (4):
ftrace: Protect direct_functions in ftrace_find_rec_direct
ftrace: Protect direct_functions in update_ftrace_direct_del
ftrace: Protect direct_functions in update_ftrace_direct_mod
ftrace: Drop extra comma in trace_buffered_event_enable

Shuangpeng Bai (1):
eventfs: Fix use-after-free in eventfs_remove_rec()

Steven Rostedt (1):
eventfs: Use children field for rcu head and add memory barriers

Tengda Wu (1):
ring-buffer: Use current_context for safe per-CPU buffer swap

Vincent Donnefort (3):
ring-buffer: Prevent resizing of persistent ring buffer
ring-buffer: Prevent subbuf order change when resizing is disabled
ring-buffer: Initialise reader page order in rb_allocate_cpu_buffer()

----

 fs/tracefs/event_inode.c   | 28 ++++++++++++++++++++++++++--
 fs/tracefs/internal.h      |  4 ++--
 kernel/trace/ftrace.c      | 33 +++++++++++++++++++++++----------
 kernel/trace/ring_buffer.c | 15 +++++++++------
 kernel/trace/trace.c       |  2 +-
 5 files changed, 61 insertions(+), 21 deletions(-)

From: Shuangpeng Bai <[email protected]>

eventfs_remove_rec() recursively removes the child at the current loop position. After the recursive call returns, list_for_each_entry() advances by reading list.next from the removed child.

If free_ei() drops the final reference, release_ei() reuses the list/rcu union to queue an SRCU callback. The child may be freed before that read. The eventfs_mutex serializes list updates, but it does not keep the removed child alive or prevent the SRCU callback from running.

Use list_for_each_entry_safe() to save the next sibling before recursively removing the current child.

---
 fs/tracefs/event_inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 39c7a34531e8..93bc4f83b73e 100644
@@ -822,7 +822,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
  */
 static void eventfs_remove_rec(struct eventfs_inode *ei, int level)
 {
-	struct eventfs_inode *ei_child;
+	struct eventfs_inode *ei_child, *tmp;
 
 	/*
 	 * Check recursion depth. It should never be greater than 3:

@@ -835,7 +835,7 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, int level)
 		return;
 
 	/* search for nested folders or files */
-	list_for_each_entry(ei_child, &ei->children, list)
+	list_for_each_entry_safe(ei_child, tmp, &ei->children, list)
 		eventfs_remove_rec(ei_child, level + 1);
 
 	list_del_rcu(&ei->list);

2.53.0

From: Steven Rostedt <[email protected]>

When an eventfs inode is freed, it sets ei->is_freed and then uses its ei->list to add it to the srcu link list as the list field is a union with the rcu list head. As the ei->list is used to iterate over an SRCU protected list without taking the eventfs_mutex, there's nothing stopping the iteration over that list to see the ei->rcu instead of the ei->list and it will read a corrupt target.

To fix this, change the union of the rcu list head with the children list. On freeing the eventfs inode, set the is_free and execute a smp_wmb() before adding the eventfs inode to the SRCU list.

On iteration of the ei->children list, at the start, execute a smp_rmb() and then read the is_freed of the ei to see if the children list is still valid. If is_freed is set, then the ei_child read is not valid and the loop should exit immediately.

---
 fs/tracefs/event_inode.c | 24 ++++++++++++++++++++++++
 fs/tracefs/internal.h    |  4 ++--
 2 files changed, 26 insertions(+), 2 deletions(-)

index 93bc4f83b73e..a52458435327 100644
@@ -124,7 +124,17 @@ static inline void put_ei(struct eventfs_inode *ei)
 static inline void free_ei(struct eventfs_inode *ei)
 {
 	if (ei) {
+		/* The ei should have no children if it is being freed. */
+		WARN_ON_ONCE(!list_empty(&ei->children));
 		ei->is_freed = 1;
+		/*
+		 * The SRCU iteration has a smp_rmb() to make sure it
+		 * sees a child (that may have already been freed)
+		 * before it reads is_free. If is_free is set, it must
+		 * not use the child it acquired from ei->children, as
+		 * the list may be used for SRCU.
+		 */
+		smp_wmb();
 		put_ei(ei);
 	}
 }

@@ -627,6 +637,20 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
 	list_for_each_entry_srcu(ei_child, &ei->children, list,
 				 srcu_read_lock_held(&eventfs_srcu)) {
 
+		/*
+		 * If the ei is being freed, then the ei->children may be
+		 * being used as the rcu list, which means the next element
+		 * may be garbage. The ei->is_free is set before switching
+		 * the ei->children over to ei->rcu. The read memory barrier
+		 * here makes sure the ei_child is read before is_free is
+		 * updated.
+		 *
+		 * Matches the smp_wmb() in free_ei()
+		 */
+		smp_rmb();
+		if (ei->is_freed)
+			return -EINVAL;
+
 		if (c > 0) {
 			c--;
 			continue;

index a4a7f8431aff..c61481d04c8e 100644
@@ -46,11 +46,11 @@ struct eventfs_attr {
  * @ino:	The saved inode number
  */
 struct eventfs_inode {
+	struct list_head	list;
 	union {
-		struct list_head	list;
+		struct list_head	children;
 		struct rcu_head		rcu;
 	};
-	struct list_head		children;
 	const struct eventfs_entry	*entries;
 	const char			*name;
 	struct eventfs_attr		*entry_attrs;

2.53.0

From: Leon Hwang <[email protected]>

Fix accessing the __rcu pointer direct_functions with RCU protection.

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

index 6c47a94f5924..c5d1d0d42ccc 100644
@@ -2645,7 +2645,8 @@ unsigned long ftrace_find_rec_direct(unsigned long ip)
 {
 	struct ftrace_func_entry *entry;
 
-	entry = __ftrace_lookup_ip(direct_functions, ip);
+	guard(preempt_notrace)();
+	entry = __ftrace_lookup_ip(rcu_dereference_sched(direct_functions), ip);
 	if (!entry)
 		return 0;
 

2.53.0

From: Leon Hwang <[email protected]>

Fix accessing the __rcu pointer direct_functions with RCU protection.

---
 kernel/trace/ftrace.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

index c5d1d0d42ccc..9ea39110927f 100644
@@ -6512,6 +6512,7 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 	struct ftrace_hash *new_direct_functions;
 	struct ftrace_hash *new_filter_hash = NULL;
 	struct ftrace_hash *old_filter_hash;
+	struct ftrace_hash *direct_hash;
 	struct ftrace_func_entry *entry;
 	struct ftrace_func_entry *del;
 	unsigned long size;

@@ -6523,11 +6524,13 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 		return -EINVAL;
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return -EINVAL;
-	if (direct_functions == EMPTY_HASH)
-		return -EINVAL;
 
 	mutex_lock(&direct_mutex);
 
+	direct_hash = rcu_dereference_protected(direct_functions, lockdep_is_held(&direct_mutex));
+	if (direct_hash == EMPTY_HASH)
+		goto out_unlock;
+
 	old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
 
 	if (!hash_count(old_filter_hash))

@@ -6537,7 +6540,7 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 	size = 1 << hash->size_bits;
 	for (int i = 0; i < size; i++) {
 		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
-			del = __ftrace_lookup_ip(direct_functions, entry->ip);
+			del = __ftrace_lookup_ip(direct_hash, entry->ip);
 			if (!del || del->direct != entry->direct)
 				goto out_unlock;
 		}

@@ -6548,7 +6551,7 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 	if (!new_filter_hash)
 		goto out_unlock;
 
-	new_direct_functions = hash_sub(direct_functions, hash);
+	new_direct_functions = hash_sub(direct_hash, hash);
 	if (!new_direct_functions)
 		goto out_unlock;
 

@@ -6575,7 +6578,7 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 		/* free the new_direct_functions */
 		old_direct_functions = new_direct_functions;
 	} else {
-		old_direct_functions = direct_functions;
+		old_direct_functions = direct_hash;
 		rcu_assign_pointer(direct_functions, new_direct_functions);
 	}
 

2.53.0

From: Leon Hwang <[email protected]>

Fix accessing the __rcu pointer direct_functions with RCU protection.

---
 kernel/trace/ftrace.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index 9ea39110927f..414e425c2d80 100644
@@ -6617,6 +6617,7 @@ int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, b
 		.func		= ftrace_stub,
 		.flags		= FTRACE_OPS_FL_STUB,
 	};
+	struct ftrace_hash *direct_hash;
 	struct ftrace_hash *orig_hash;
 	unsigned long size, i;
 	int err = -EINVAL;

@@ -6627,8 +6628,6 @@ int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, b
 		return -EINVAL;
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return -EINVAL;
-	if (direct_functions == EMPTY_HASH)
-		return -EINVAL;
 
 	/*
 	 * We can be called from within ops_func callback with direct_mutex

@@ -6636,6 +6635,12 @@ int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, b
 	 */
 	if (do_direct_lock)
 		mutex_lock(&direct_mutex);
+	else
+		lockdep_assert_held_once(&direct_mutex);
+
+	direct_hash = rcu_dereference_protected(direct_functions, lockdep_is_held(&direct_mutex));
+	if (direct_hash == EMPTY_HASH)
+		goto unlock;
 
 	orig_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
 	if (!orig_hash)

@@ -6667,7 +6672,7 @@ int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, b
 	size = 1 << hash->size_bits;
 	for (i = 0; i < size; i++) {
 		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
-			tmp = __ftrace_lookup_ip(direct_functions, entry->ip);
+			tmp = __ftrace_lookup_ip(direct_hash, entry->ip);
 			if (!tmp)
 				continue;
 			tmp->direct = entry->direct;

2.53.0

From: Leon Hwang <[email protected]>

Drop the extra comma in "scoped_guard()" to cleanup the code.

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

index 01a5e87af299..395238b2b715 100644
@@ -1788,7 +1788,7 @@ void trace_buffered_event_enable(void)
 
 		per_cpu(trace_buffered_event, cpu) = event;
 
-		scoped_guard(preempt,) {
+		scoped_guard(preempt) {
 			if (cpu == smp_processor_id() &&
 			    __this_cpu_read(trace_buffered_event) !=
 			    per_cpu(trace_buffered_event, cpu))

2.53.0

From: Tengda Wu <[email protected]>

The ring_buffer_swap_cpu() function currently checks the per-CPU committing counter to determine if a buffer is actively being written to before performing the swap. However, there exists a race window where this check can be bypassed:

    ring_buffer_lock_reserve
        cpu_buffer = buffer->buffers[cpu];       // cpu_buffer_a
        rb_reserve_next_event
            rb_start_commit // inc committing
            if (unlikely(READ_ONCE(cpu_buffer->buffer) != buffer)) {...}
            __rb_reserve_next
                rb_move_tail
                    rb_end_commit(cpu_buffer);   // dec committing => 0
                    /* interrupt hits here, successfully swaps! */
                    local_inc(&cpu_buffer->committing);

    ring_buffer_unlock_commit
        cpu_buffer = buffer->buffers[cpu];      // cpu_buffer_b
        rb_commit
            rb_end_commit
            RB_WARN_ON(cpu_buffer, !local_read(&cpu_buffer->committing))
                                                // triggers warning

The committing counter can temporarily drop to 0 during a single write operation (within rb_move_tail), creating a window where swap can succeed even though the write is still in progress. This leads to inconsistent buffer state and triggers the RB_WARN_ON in rb_commit().

Replace the committing counter check with current_context checks, which are set at the entry of ring_buffer_lock_reserve() and remain valid throughout the entire write operation, providing a reliable indicator of buffer busy state during swap.

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

index 8e2485bb3aa8..58dc8995a88d 100644
@@ -6852,7 +6852,7 @@ int ring_buffer_swap_cpu(struct trace_buffer *buffer_a,
 {
 	struct ring_buffer_per_cpu *cpu_buffer_a;
 	struct ring_buffer_per_cpu *cpu_buffer_b;
-	int ret = -EINVAL;
+	int ret = -EBUSY;
 
 	if (!cpumask_test_cpu(cpu, buffer_a->cpumask) ||
 	    !cpumask_test_cpu(cpu, buffer_b->cpumask))

@@ -6893,10 +6893,10 @@ int ring_buffer_swap_cpu(struct trace_buffer *buffer_a,
 	atomic_inc(&cpu_buffer_a->record_disabled);
 	atomic_inc(&cpu_buffer_b->record_disabled);
 
-	ret = -EBUSY;
-	if (local_read(&cpu_buffer_a->committing))
+	/* Do not swap if either buffer is in the process of writing */
+	if (cpu_buffer_a->current_context)
 		goto out_dec;
-	if (local_read(&cpu_buffer_b->committing))
+	if (cpu_buffer_b->current_context)
 		goto out_dec;
 
 	/*

2.53.0

From: Josh Poimboeuf <[email protected]>

When a module's init text is freed, do_init_module() calls ftrace_free_mem() with a half-open [start, end) range. However the ftrace_cmp_recs() comparator treats the upper bound as inclusive, as all its other users do, passing 'ip + size - 1'. So ftrace_free_mem() can delete a record sitting exactly at 'end', which is outside the freed range.

For a kernel without CFI or IBT, the first record of a function is at the function start, which for the first function in a module is also the base of its text allocation. As the module allocator packs its regions, that address is often the 'end' passed by a neighboring module's do_init_module(), causing the first function's ftrace location to get disabled, preventing an attempt to livepatch it:

livepatch: failed to find location for function 'pcspkr_probe'

Convert the exclusive end to the inclusive 'end - 1' the comparator expects, and return early for an empty range to avoid the subtraction from underflowing when the init text size is zero.

---
 kernel/trace/ftrace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 414e425c2d80..7c50f8ae5a0c 100644
@@ -8305,7 +8305,8 @@ static void add_to_clear_hash_list(struct list_head *clear_list,
 void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 {
 	unsigned long start = (unsigned long)(start_ptr);
-	unsigned long end = (unsigned long)(end_ptr);
+	/* end is inclusive and end_ptr is exclusive */
+	unsigned long end = (unsigned long)(end_ptr) - 1;
 	struct ftrace_page **last_pg = &ftrace_pages_start;
 	struct ftrace_page *tmp_page = NULL;
 	struct ftrace_page *pg;

@@ -8315,6 +8316,9 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 	struct ftrace_init_func *func, *func_next;
 	LIST_HEAD(clear_hash);
 
+	if (start_ptr >= end_ptr)
+		return;
+
 	key.ip = start;
 	key.flags = end;	/* overload flags, as it is unsigned long */
 

2.53.0

From: Vincent Donnefort <[email protected]>

Dynamically resizing a persistent ring buffer is not possible. Disable the feature.

---
 kernel/trace/ring_buffer.c | 2 ++
 1 file changed, 2 insertions(+)

index 58dc8995a88d..09d502ef4c55 100644
@@ -2528,6 +2528,8 @@ rb_allocate_cpu_buffer(struct trace_buffer *buffer, long nr_pages, int cpu)
 		if (cpu_buffer->ring_meta->head_buffer)
 			rb_meta_buffer_update(cpu_buffer, bpage);
 		bpage->range = 1;
+
+		atomic_inc(&cpu_buffer->resize_disabled);
 	} else if (buffer->remote) {
 		struct ring_buffer_desc *desc = ring_buffer_desc(buffer->remote->desc, cpu);
 

2.53.0

From: Vincent Donnefort <[email protected]>

Because ring_buffer_subbuf_order_set() frees buffer pages, we can't allow it when resizing is disabled. A non-consuming reader is at risk of use-after-free (rb_advance_iter()).

Return -EBUSY on resize_disabled, matching ring_buffer_resize() behaviour.

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

index 09d502ef4c55..6cbd80ccef37 100644
@@ -7360,7 +7360,7 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 
 		cpu_buffer = buffer->buffers[cpu];
 
-		if (cpu_buffer->mapped) {
+		if (atomic_read(&cpu_buffer->resize_disabled)) {
 			err = -EBUSY;
 			goto error;
 		}

2.53.0

From: Vincent Donnefort <[email protected]>

In rb_allocate_cpu_buffer(), bpage->order was omitted, leaving it as 0. This is an issue for a ring-buffer with subbufs bigger than PAGE_SIZE if when freed: free_buffer_page() relies on this value. Align the value with the actual allocation size (buffer::subbuf_order).

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

index 6cbd80ccef37..760a00e8505c 100644
@@ -2510,6 +2510,7 @@ rb_allocate_cpu_buffer(struct trace_buffer *buffer, long nr_pages, int cpu)
 	bpage = alloc_cpu_page(cpu);
 	if (!bpage)
 		return NULL;
+	bpage->order = cpu_buffer->buffer->subbuf_order;
 
 	rb_check_bpage(cpu_buffer, bpage);
 

2.53.0

From: Hui Su <[email protected]>

In test_ringbuffer()'s out_free cleanup loop, the check !rb_threads[cpu] only catches NULL entries and misses entries that hold an ERR_PTR.

rb_threads[] is static, so unassigned slots are NULL. But when kthread_run_on_cpu() fails for a cpu, it stores ERR_PTR(-ENOMEM) (or -EINTR) in rb_threads[cpu] before the creation loop jumps to out_free. That entry is non-NULL, so the old !ptr check does not break, and the cleanup proceeds to call kthread_stop() on the ERR_PTR. kthread_stop() then dereferences the bogus pointer, crashing the kernel during the late_initcall self-test.

crash logs:

  BUG: kernel NULL pointer dereference, address: 000000000000001c
  Oops: 0002 [#1] SMP NOPTI
  CPU: 1 PID: 1 Comm: swapper/0 Not tainted 7.2.0-rc6-dirty #7 PREEMPT(lazy)
  RIP: 0010:kthread_stop+0x2e/0x220
  RBX: fffffffffffffff4
  CR2: 000000000000001c
  Call Trace:
   <TASK>
   test_ringbuffer+0x1ec/0x650
   do_one_initcall+0x6c/0x2c0
   kernel_init_freeable+0x21d/0x420
   kernel_init+0x15/0x1c0
   ret_from_fork+0x21b/0x320
   </TASK>
  Kernel panic - not syncing: Fatal exception

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

index 760a00e8505c..2667992f0aa2 100644
@@ -8217,7 +8217,7 @@ static __init int test_ringbuffer(void)
 
  out_free:
 	for_each_online_cpu(cpu) {
-		if (!rb_threads[cpu])
+		if (IS_ERR_OR_NULL(rb_threads[cpu]))
 			break;
 		kthread_stop(rb_threads[cpu]);
 	}

2.53.0