tracing: Fixes for 7.2

tracing fixes for 7.2:

- Free field in error path of synthetic event parse

In __create_synth_event() the field was allocated but was not freed in the error path.

- Fix ring_buffer_event_length() on 8 byte aligned architectures

On architectures with CONFIG_HAVE_64BIT_ALIGNED_ACCESS set to y, the ring_buffer_event_length() may return the wrong size. This is because archs with that config set will always use the "big event meta header" as that is 8 bytes keeping the payload 8 bytes aligned, even when a 4 byte header could hold the size of the event.

But ring_buffer_event_length() doesn't take this into account and only subtracts 4 bytes for the meta header in the length when it should have subtracted 8 bytes.

- Have osnoise wait for a full rcu synchronization on unregister

osnoise_unregister_instance() used to call synchronize_rcu() before freeing its copy of the instance but was switched to kfree_rcu(). The osniose tracer has code that traverses the instances that it uses, and inst is just a pointer to that instance. By using kfree_rcu() instead of synchronize_rcu(), the instance that the inst pointer is pointing to can be freed while the osnoise code is still referencing it.

That is, a rmdir on an instance first unregisters the tracer. When the unregister finishes, the rmdir expects that the tracer is finished with the instance that it is using. By putting back the synchronize_rcu() in osnoise_unregister_instance() the unregistering of osnoise will now return when all the users of the instance have finished.

- Remove an unused setting of "ret" in tracing_set_tracer()

- Fix a race between ring buffer setting subbuf_order and reading it

When ring_buffer_subbuf_order_set() is called at the same time as ring_buffer_read_page(), there is a race that the subbuf_size will change and the read_page will allocate one size and then initialize it as another size.

Fix this by moving the reading of the subbuf_size into the locking and use a local variable to use it for all other transactions when the lock is released. It is OK if it changes, as long as the allocated page is still considered the same throughout the read function.

- Fix ring_buffer_read_page() copying events

The commit that changed ring_buffer_read_page() to show dropped events from the buffer itself, split the "commit" variable between the commit value (with flags) and "size" that holds the size of the sub-buffer. A cut and paste error changed the test of the reading from checking the size of the buffer to the size of the event causing reads to only read one event at a time.

- Make tracepoint_printk a static variable

When the tracing sysctl knobs were move from sysctl.c to trace.c, the variable tracepoint_printk no longer needed to be global. Make it static.

- Fix user_event use-after-free of enabler descriptor

When the enabler is removed from the link list, it is freed immediately. But it is protected via RCU and needs to be freed via kfree_rcu().

- Fix some typos

- Fix NULL pointer dereference in func_set_flag()

The flags update of the function tracer first checks if the value of the flag is the same and exits if they are, and then it checks if the current tracer is the function tracer and exits if it isn't. The problem is that these checks need to be in a reversed order, as if the tracer isn't the function tracer, then the flag being checked may not exist. Reverse the order of these checks.

- Fix ufs core trace events to not dereference a pointer in TP_printk()

The TP_printk() part of the TRACE_EVENT() macro is called when the user reads the "trace" file. This can be seconds, minutes, hours, days, weeks, and even months after the data was recorded into the ring buffer. Thus, saving a pointer to an object into the ring buffer and then dereferencing it from TP_printk() can cause harm as the object the pointer is pointing to may no longer exist.

Fix all the trace events in ufs core to save the device name in the ring buffer instead of dereferencing the device descriptor from TP_printk().

- Prevent out-of-bound reads in glob matching of trace events

The filter logic of events allows simple glob logic to add wild cards to filter on strings. But some events have fields that may not have a terminating 'nul' character. This may cause the glob matching to go beyond the string. Change the logic to always pass in the length of the field that is being matched.

- Add no-rcu-check version of trace_##event##_enabled()

The trace_##event##_enabled() usually wraps trace events to do extra work that is only needed when the trace event is enabled. But this can hide events that are placed in locations where RCU is not watching, and can make lockdep not see these bugs when the event is not enabled.

The trace_##event##_enabled() was updated to always test to make sure RCU is watching to catch locations that may call events without RCU being active.

This caused a false positive for the irq_disabled() and related events. As that use trace_irq_disabled_enabled() to force RCU to be watching when the event is enabled via the ct_irq_enter() function, calls the event, and then calls ct_irq_exit() to put RCU back to its original state.

The trace_irq_disabled_enabled() should not trigger a warning when RCU is not watching because the code within its block handles the case properly. Make a __trace_##event##_enabled() version for this event to use that doesn't check RCU is watching as it handles the case when it isn't.

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

Head SHA1: 9cc30885fbd3a59d81f70414bc7a13cbc148f60e

Ben Dooks (1):
tracing: make tracepoint_printk static as not exported

Crystal Wood (1):
tracing/osnoise: Call synchronize_rcu() when unregistering

David Carlier (1):
ring-buffer: Fix ring_buffer_read_page() copying only one event per page

Hui Wang (1):
ring-buffer: Fix event length with forced 8-byte alignment

Huihui Huang (1):
tracing: Prevent out-of-bounds read in glob matching

Michael Bommarito (1):
tracing/user_events: Fix use-after-free of enabler in user_event_mm_dup()

Steven Rostedt (2):
ufs: core: tracing: Do not dereference pointers in TP_printk()
tracing: Add a no-rcu-check version of trace_##event##_enabled()

Wayen.Yan (1):
tracing: Remove unused ret assignment in tracing_set_tracer()

Yash Suthar (1):
ring_buffer: Check page order under reader_lock

Yu Peng (1):
tracing/synthetic: Free pending field on error path

Yuanhe Shu (1):
tracing: Fix NULL pointer dereference in func_set_flag()

Yudistira Putra (1):
samples: ftrace: Fix typos in benchmark comment

----

 drivers/ufs/core/ufs_trace.h       | 36 +++++++++++++++++++++++++++---------
 include/linux/glob.h               |  1 +
 include/linux/tracepoint.h         | 12 +++++++++++-
 kernel/trace/ring_buffer.c         | 25 +++++++++++++++++--------
 kernel/trace/trace.c               |  3 +--
 kernel/trace/trace_events_filter.c |  6 ++----
 kernel/trace/trace_events_synth.c  |  6 ++++--
 kernel/trace/trace_events_user.c   | 10 +++++++++-
 kernel/trace/trace_functions.c     |  8 ++++----
 kernel/trace/trace_osnoise.c       |  4 +++-
 kernel/trace/trace_preemptirq.c    |  2 +-
 lib/glob.c                         | 31 +++++++++++++++++++++++++++++--
 samples/ftrace/ftrace-ops.c        |  4 ++--
 13 files changed, 111 insertions(+), 37 deletions(-)

From: Yu Peng <[email protected]>

Some __create_synth_event() error paths run after parse_synth_field() succeeds but before the field is stored in fields[]. The common cleanup then misses the field. Free it before freeing argv.

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

index e6871230bde9..cdd5b9332835 100644
@@ -1446,13 +1446,13 @@ static int __create_synth_event(const char *name, const char *raw_fields)
 			if (cmd_version > 1 && n_fields_this_loop >= 1) {
 				synth_err(SYNTH_ERR_INVALID_CMD, errpos(field_str));
 				ret = -EINVAL;
-				goto err_free_arg;
+				goto err_free_field;
 			}
 
 			if (n_fields == SYNTH_FIELDS_MAX) {
 				synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
 				ret = -EINVAL;
-				goto err_free_arg;
+				goto err_free_field;
 			}
 			fields[n_fields++] = field;
 

@@ -1491,6 +1491,8 @@ static int __create_synth_event(const char *name, const char *raw_fields)
 	kfree(saved_fields);
 
 	return ret;
+ err_free_field:
+	free_synth_field(field);
  err_free_arg:
 	argv_free(argv);
  err:

2.53.0

From: Hui Wang <[email protected]>

When RB_FORCE_8BYTE_ALIGNMENT is true, rb_calculate_event_length() reserves the space of event->array[0] for placing the data length and rb_update_event() stores the data length in event->array[0] accordingly. As a result the whole event length will add extra 4 bytes for sizeof(event.array[0]) unconditionally.

But ring_buffer_event_length() only subtracts the sizeof(event->array[0]) for events larger than RB_MAX_SMALL_DATA + sizeof(event->array[0]). As a result, small events on architectures with RB_FORCE_8BYTE_ALIGNMENT=true report a data length that is 4 bytes larger than expected.

To fix it, add the RB_FORCE_8BYTE_ALIGNMENT as a condition to subtract the size of that length field whenever RB_FORCE_8BYTE_ALIGNMENT is true.

This issue is observed in a riscv64 kernel with CONFIG_HAVE_64BIT_ALIGNED_ACCESS set to y, when we run ftrace selftest trace_marker_raw.tc, we get the weird log: for cases where the id is 1..100, the number of data field is 8*N, but once id exceeds 100, the number of data field becomes 8*N+4:
# 1 buf: 58 00 00 00 80 5e d1 63 (number of data field is 8*1)
...

 # a buf: 58 ...                  (number of data field is 8*2)
 ...
 # 64 buf: 58 ...                 (number of data field is 8*13)
 # 65 buf: 58 ...                 (number of data field is 8*13+4)

After applying this change, the number of data field keeps being 8*N+4 consistently.

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

index 56a328e94395..d9af2bbaf9c0 100644
@@ -270,7 +270,8 @@ unsigned ring_buffer_event_length(struct ring_buffer_event *event)
 	if (event->type_len > RINGBUF_TYPE_DATA_TYPE_LEN_MAX)
 		return length;
 	length -= RB_EVNT_HDR_SIZE;
-	if (length > RB_MAX_SMALL_DATA + sizeof(event->array[0]))
+	if (length > RB_MAX_SMALL_DATA + sizeof(event->array[0]) ||
+	    RB_FORCE_8BYTE_ALIGNMENT)
                 length -= sizeof(event->array[0]);
 	return length;
 }

2.53.0

From: Crystal Wood <[email protected]>

This ensures that any RCU readers traversing the instance list have finished, before releasing the reference on the tracer that the instance points to.

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

index 5e83c4f6f2b4..0e1265acd1cc 100644
@@ -179,7 +179,9 @@ static void osnoise_unregister_instance(struct trace_array *tr)
 	if (!found)
 		return;
 
-	kvfree_rcu_mightsleep(inst);
+	/* Do a full sync to ensure that tr remains valid, not just inst */
+	synchronize_rcu();
+	kvfree(inst);
 }
 
 /*

2.53.0

From: "Wayen.Yan" <[email protected]>

In tracing_set_tracer(), the assignment 'ret = 0' following the __tracing_resize_ring_buffer() error check is a dead store. After this point, all subsequent code paths either return with a constant value (-EINVAL, 0, -EBUSY) or reassign ret before reading it (tracing_arm_snapshot_locked, tracer_init).

Remove the unnecessary assignment.

No functional change.

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

index 1146b83b711a..299f5ab630b9 100644
@@ -5015,7 +5015,6 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
 						RING_BUFFER_ALL_CPUS);
 		if (ret < 0)
 			return ret;
-		ret = 0;
 	}
 
 	list_for_each_entry(t, &tr->tracers, list) {

2.53.0

From: Yash Suthar <[email protected]>

When the ring_buffer_subbuf_order_set() is called the same time as the ring_buffer_read_page(), the wrong buffer->subbuf_size can be used as there is nothing keeping that in sync. This can cause an incorrect allocation and initialization that can cause a crash.

Move the saving of the subbuf_size into a variable within the cpu_buffer->reader_lock, and use that throughout the function. The size only needs to be consistent throughout the allocation and initialization. If the buffer->subbuf_size changes when used, the reader data page will be found to be invalid and the read function will return an error (this is as expected).

syzbot did not provide a reproducer for this crash, the race condition is logically sound and found via code inspection of the trace.

---
 kernel/trace/ring_buffer.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

index d9af2bbaf9c0..3ec173e22eeb 100644
@@ -7070,6 +7070,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	struct ring_buffer_event *event;
 	struct buffer_data_page *dpage;
 	struct buffer_page *reader;
+	unsigned int subbuf_size;
 	long missed_events;
 	unsigned int commit;
 	unsigned int size;

@@ -7092,15 +7093,22 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	if (!data_page || !data_page->data)
 		return -1;
 
-	if (data_page->order != buffer->subbuf_order)
-		return -1;
-
 	dpage = data_page->data;
 	if (!dpage)
 		return -1;
 
 	guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
 
+	/*
+	 * Check data_page order under lock to prevent a race with a
+	 * concurrent ring_buffer_subbuf_order_set() swap, which can
+	 * cause an outofbounds memset() if the subbuf_size changes.
+	 */
+	if (data_page->order != buffer->subbuf_order)
+		return -1;
+
+	subbuf_size = (PAGE_SIZE << data_page->order) - BUF_PAGE_HDR_SIZE;
+
 	reader = rb_get_reader_page(cpu_buffer);
 	if (!reader)
 		return -1;

@@ -7226,7 +7234,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 		 * missed events, then record it there.
 		 */
 		if (missed_events > 0 &&
-		    buffer->subbuf_size - size >= sizeof(missed_events)) {
+		    subbuf_size - size >= sizeof(missed_events)) {
 			memcpy(&dpage->data[size], &missed_events,
 			       sizeof(missed_events));
 			local_add(RB_MISSED_STORED, &dpage->commit);

@@ -7246,8 +7254,8 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	/*
 	 * This page may be off to user land. Zero it out here.
 	 */
-	if (size < buffer->subbuf_size)
-		memset(&dpage->data[size], 0, buffer->subbuf_size - size);
+	if (size < subbuf_size)
+		memset(&dpage->data[size], 0, subbuf_size - size);
 
 	return read;
 }

2.53.0

I'm actually dropping this. It's not enough, and there is work to fix it properly:

https://lore.kernel.org/all/[email protected]/

-- Steve

From: David Carlier <[email protected]>

Commit 8928e4a3be34 ("ring-buffer: Show persistent buffer dropped events in trace_pipe file") split the "commit" variable in ring_buffer_read_page() into "commit" (raw) and "size" (masked page size), but the inner copy loop's terminator was changed to compare rpos against "event_size" instead of "size".

rpos is the cumulative read offset within the page; event_size is the length of the single event just copied. The loop thus breaks after the first event, so only one event is copied per call. This regresses the per-event memcpy path (partial reads, the active commit page, and mapped/remote buffers) used by splice/trace_pipe_raw and mmap consumers into a one-event-at-a-time read.

Compare rpos against the page size as the original code did.

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

index 3ec173e22eeb..4b994aea9e61 100644
@@ -7183,7 +7183,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 			rpos = reader->read;
 			pos += event_size;
 
-			if (rpos >= event_size)
+			if (rpos >= size)
 				break;
 
 			event = rb_reader_event(cpu_buffer);

2.53.0

From: Ben Dooks <[email protected]>

The tracepoint_printk symbol is not exported, so make it static to remove the following sparse warning:

kernel/trace/trace.c:90:5: warning: symbol 'tracepoint_printk' was not declared. Should it be static?

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

index 299f5ab630b9..18710c190c92 100644
@@ -87,7 +87,7 @@ void __init disable_tracing_selftest(const char *reason)
 
 /* Pipe tracepoints to printk */
 static struct trace_iterator *tracepoint_print_iter;
-int tracepoint_printk;
+static int tracepoint_printk;
 static bool tracepoint_printk_stop_on_boot __initdata;
 static bool traceoff_after_boot __initdata;
 static DEFINE_STATIC_KEY_FALSE(tracepoint_printk_key);

2.53.0

From: Michael Bommarito <[email protected]>

user_event_enabler_destroy() removes an enabler from the per-mm mm->enablers list with list_del_rcu() and then frees it immediately with kfree(). That list is walked locklessly by user_event_mm_dup() during fork(), under rcu_read_lock() only:

	rcu_read_lock();
	list_for_each_entry_rcu(enabler, &old_mm->enablers, mm_enablers_link)
		...

user_event_mm_dup() does not take event_mutex. The per-enabler destroy path user_events_ioctl_unreg() (DIAG_IOCSUNREG) takes event_mutex but nothing that excludes the dup walk. Threads that share an mm share one user_event_mm and one enabler list, so an unregister on one thread can free an enabler while another thread is forking and user_event_mm_dup() is mid-walk. The walk then dereferences the freed enabler (for example enabler->event in user_event_enabler_dup()).

This is reachable by an unprivileged task that can open user_events_data:
a single multithreaded process that registers an enabler and then concurrently unregisters it and calls fork() triggers the race. KASAN reports a slab-use-after-free read in user_event_enabler_dup() called from user_event_mm_dup() and copy_process() during clone(); with kasan.fault=panic the kernel panics.

Free the enabler after a grace period with kfree_rcu(), matching the list_del_rcu() removal and the rcu_read_lock() readers in user_event_mm_dup(). Add an rcu_head to struct user_event_enabler for this. The error path in user_event_enabler_create() keeps using kfree() because that enabler is freed before it is published to the RCU list.

---
 kernel/trace/trace_events_user.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index c4ba484f7b38..412ca1e3a40c 100644
@@ -109,6 +109,9 @@ struct user_event_enabler {
 
 	/* Track enable bit, flags, etc. Aligned for bitops. */
 	unsigned long		values;
+
+	/* Defer free so RCU list readers (user_event_mm_dup) are safe. */
+	struct rcu_head		rcu;
 };
 
 /* Bits 0-5 are for the bit to update upon enable/disable (0-63 allowed) */

@@ -404,7 +407,12 @@ static void user_event_enabler_destroy(struct user_event_enabler *enabler,
 	/* No longer tracking the event via the enabler */
 	user_event_put(enabler->event, locked);
 
-	kfree(enabler);
+	/*
+	 * The enabler is removed from an RCU-traversed list
+	 * (user_event_mm_dup walks mm->enablers under rcu_read_lock only),
+	 * so the backing memory must outlive a grace period.
+	 */
+	kfree_rcu(enabler, rcu);
 }
 
 static int user_event_mm_fault_in(struct user_event_mm *mm, unsigned long uaddr,

2.53.0

From: Yudistira Putra <[email protected]>

Fix two typos in the ftrace operations sample benchmark comment.

---
 samples/ftrace/ftrace-ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 68d6685c80bd..152ffc1a30b6 100644
@@ -232,8 +232,8 @@ static int __init ftrace_ops_sample_init(void)
 	ops_destroy(ops_irrelevant, nr_ops_irrelevant);
 
 	/*
-	 * The benchmark completed sucessfully, but there's no reason to keep
-	 * the module around. Return an error do the user doesn't have to
+	 * The benchmark completed successfully, but there's no reason to keep
+	 * the module around. Return an error so the user doesn't have to
 	 * manually unload the module.
 	 */
 	return -EINVAL;

2.53.0

From: Yuanhe Shu <[email protected]>

func_set_flag() dereferences tr->current_trace_flags before verifying that the current tracer is actually the function tracer. When the active tracer has been switched away from "function" (e.g., to "wakeup_rt"), tr->current_trace_flags can be NULL, leading to a NULL pointer dereference and kernel crash.

The call chain that triggers this is:

trace_options_write()

    -> __set_tracer_option()
      -> trace->set_flag()          /* func_set_flag */

In func_set_flag(), the first operation is:

if (!!set == !!(tr->current_trace_flags->val & bit))

This dereferences tr->current_trace_flags unconditionally. The safety check that guards against a non-function tracer:

if (tr->current_trace != &function_trace)

      return 0;

is placed *after* the dereference, which is too late.

This was observed with the following crash dump:

  BUG: unable to handle page fault at 0000000000000000
  RIP: func_set_flag+0xd

  Call Trace:
   __set_tracer_option+0x27
   trace_options_write+0x75
   vfs_write+0x12a
   ksys_write+0x66
   do_syscall_64+0x5b

  RIP: ffffffff914c973d  RSP: ff67ec88b01dfdf0  RFLAGS: 00010202
  RAX: 0000000000000000  RBX: ff3a826e80354580  RCX: 0000000000000001
  RDX: 0000000000000001  RSI: 0000000000000000  RDI: ffffffff93918080

The disassembly confirms the fault:

  func_set_flag+0:   mov 0x1f08(%rdi), %rax  ; RAX = tr->current_trace_flags = NULL
  func_set_flag+13:  mov (%rax), %eax        ; page fault: dereference NULL

At the time of the crash:
tr->current_trace_flags = 0x0 (NULL) tr->current_trace = wakeup_rt_tracer (not function_trace)

The scenario is that a process opens a function tracer option file (such as "func_stack_trace"), then the current tracer is switched to another tracer (e.g., "wakeup_rt"), which sets current_trace_flags to NULL. When the process subsequently writes to the option file, func_set_flag() is invoked and crashes on the NULL dereference.

Fix this by moving the current_trace check before the current_trace_flags dereference, so that func_set_flag() returns early when the function tracer is not active.

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

index f283391a4dc8..cd37f2013758 100644
@@ -458,12 +458,12 @@ func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
 	ftrace_func_t func;
 	u32 new_flags;
 
-	/* Do nothing if already set. */
-	if (!!set == !!(tr->current_trace_flags->val & bit))
+	/* We can change this flag only when current tracer is function. */
+	if (tr->current_trace != &function_trace)
 		return 0;
 
-	/* We can change this flag only when not running. */
-	if (tr->current_trace != &function_trace)
+	/* Do nothing if already set. */
+	if (!!set == !!(tr->current_trace_flags->val & bit))
 		return 0;
 
 	new_flags = (tr->current_trace_flags->val & ~bit) | (set ? bit : 0);

2.53.0

From: Steven Rostedt <[email protected]>

The trace events in drivers/ufs/core/ufs_trace.h were converted to take a pointer to the hba structure as an argument for the tracepoint and then in TP_printk() the printing of the dev_name from the ring buffer was converted to using the dev dereferenced pointer from the hba saved pointer.

This is not allowed as the TP_printk() is executed at the time the trace event is read from /sys/kernel/tracing/trace file. That can happen literally, seconds, minutes, hours, weeks, days, or even months later! There is no guarantee that the hba pointer will still exist by the time it is dereferenced when the "trace" file is read.

Instead, save the device name from the hba pointer at the time the tracepoint is called and place it into the ring buffer event. Then the TP_printk() can read the name directly from the ring buffer and remove the possibility that it will read a freed pointer and crash the kernel.

This was detected when testing the trace event code that looks for TP_printk() parameters doing illegal derferences[1]

[1] https://lore.kernel.org/all/[email protected]/

---
 drivers/ufs/core/ufs_trace.h | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

index 309ae51b4906..377a3c54b9f5 100644
@@ -89,16 +89,18 @@ TRACE_EVENT(ufshcd_clk_gating,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(int, state)
 	),
 
 	TP_fast_assign(
+		__assign_str(dev_name);
 		__entry->hba = hba;
 		__entry->state = state;
 	),
 
 	TP_printk("%s: gating state changed to %s",
-		dev_name(__entry->hba->dev),
+		__get_str(dev_name),
 		__print_symbolic(__entry->state, UFSCHD_CLK_GATING_STATES))
 );
 

@@ -111,6 +113,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(state, state)
 		__string(clk, clk)
 		__field(u32, prev_state)

@@ -119,6 +122,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(state);
 		__assign_str(clk);
 		__entry->prev_state = prev_state;

@@ -126,7 +130,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 	),
 
 	TP_printk("%s: %s %s from %u to %u Hz",
-		dev_name(__entry->hba->dev), __get_str(state), __get_str(clk),
+		__get_str(dev_name), __get_str(state), __get_str(clk),
 		__entry->prev_state, __entry->curr_state)
 );
 

@@ -138,16 +142,18 @@ TRACE_EVENT(ufshcd_auto_bkops_state,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(state, state)
 	),
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(state);
 	),
 
 	TP_printk("%s: auto bkops - %s",
-		dev_name(__entry->hba->dev), __get_str(state))
+		__get_str(dev_name), __get_str(state))
 );
 
 DECLARE_EVENT_CLASS(ufshcd_profiling_template,

@@ -158,6 +164,7 @@ DECLARE_EVENT_CLASS(ufshcd_profiling_template,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(profile_info, profile_info)
 		__field(s64, time_us)
 		__field(int, err)

@@ -165,13 +172,14 @@ DECLARE_EVENT_CLASS(ufshcd_profiling_template,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(profile_info);
 		__entry->time_us = time_us;
 		__entry->err = err;
 	),
 
 	TP_printk("%s: %s: took %lld usecs, err %d",
-		dev_name(__entry->hba->dev), __get_str(profile_info),
+		__get_str(dev_name), __get_str(profile_info),
 		__entry->time_us, __entry->err)
 );
 

@@ -200,6 +208,7 @@ DECLARE_EVENT_CLASS(ufshcd_template,
 		__field(s64, usecs)
 		__field(int, err)
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(int, dev_state)
 		__field(int, link_state)
 	),

@@ -208,13 +217,14 @@ DECLARE_EVENT_CLASS(ufshcd_template,
 		__entry->usecs = usecs;
 		__entry->err = err;
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->dev_state = dev_state;
 		__entry->link_state = link_state;
 	),
 
 	TP_printk(
 		"%s: took %lld usecs, dev_state: %s, link_state: %s, err %d",
-		dev_name(__entry->hba->dev),
+		__get_str(dev_name),
 		__entry->usecs,
 		__print_symbolic(__entry->dev_state, UFS_PWR_MODES),
 		__print_symbolic(__entry->link_state, UFS_LINK_STATES),

@@ -279,6 +289,7 @@ TRACE_EVENT(ufshcd_command,
 	TP_STRUCT__entry(
 		__field(struct scsi_device *, sdev)
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(&sdev->sdev_dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__field(unsigned int, tag)
 		__field(u32, doorbell)

@@ -291,6 +302,7 @@ TRACE_EVENT(ufshcd_command,
 	),
 
 	TP_fast_assign(
+		__assign_str(dev_name);
 		__entry->sdev = sdev;
 		__entry->hba = hba;
 		__entry->str_t = str_t;

@@ -307,7 +319,7 @@ TRACE_EVENT(ufshcd_command,
 	TP_printk(
 		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, hwq_id: %d",
 		show_ufs_cmd_trace_str(__entry->str_t),
-		dev_name(&__entry->sdev->sdev_dev), __entry->tag,
+		__get_str(dev_name), __entry->tag,
 		__entry->doorbell, __entry->transfer_len, __entry->intr,
 		__entry->lba, (u32)__entry->opcode, str_opcode(__entry->opcode),
 		(u32)__entry->group_id, __entry->hwq_id

@@ -322,6 +334,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__field(u32, cmd)
 		__field(u32, arg1)

@@ -331,6 +344,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->str_t = str_t;
 		__entry->cmd = cmd;
 		__entry->arg1 = arg1;

@@ -340,7 +354,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_printk(
 		"%s: %s: cmd: 0x%x, arg1: 0x%x, arg2: 0x%x, arg3: 0x%x",
-		show_ufs_cmd_trace_str(__entry->str_t), dev_name(__entry->hba->dev),
+		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
 		__entry->cmd, __entry->arg1, __entry->arg2, __entry->arg3
 	)
 );

@@ -353,6 +367,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__array(unsigned char, hdr, 12)
 		__array(unsigned char, tsf, 16)

@@ -361,6 +376,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->str_t = str_t;
 		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
 		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));

@@ -369,7 +385,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_printk(
 		"%s: %s: HDR:%s, %s:%s",
-		show_ufs_cmd_trace_str(__entry->str_t), dev_name(__entry->hba->dev),
+		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
 		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
 		show_ufs_cmd_trace_tsf(__entry->tsf_t),
 		__print_hex(__entry->tsf, sizeof(__entry->tsf))

@@ -384,16 +400,18 @@ TRACE_EVENT(ufshcd_exception_event,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(u16, status)
 	),
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->status = status;
 	),
 
 	TP_printk("%s: status 0x%x",
-		dev_name(__entry->hba->dev), __entry->status
+		__get_str(dev_name), __entry->status
 	)
 );
 

2.53.0

From: Huihui Huang <[email protected]>

String event fields are not necessarily NUL-terminated, so the filter predicate functions (filter_pred_string(), filter_pred_strloc() and filter_pred_strrelloc()) pass the field length to the regex match callbacks, and the length-aware matchers honour it.

regex_match_glob() was the exception: it ignored the length and called glob_match(), which scans the string until it hits a NUL byte. Some string fields are not NUL-terminated. One example is the dynamic char array of the xfs_* namespace tracepoints, which is copied without a trailing NUL. For such a field, glob matching reads past the end of the event field, causing a KASAN slab-out-of-bounds read in glob_match(), reached via regex_match_glob() and filter_match_preds() from the xfs_lookup tracepoint.

Add a length-bounded glob_match_len() and use it from regex_match_glob() so glob matching always stops at the field boundary. The matching loop is factored into a shared helper so glob_match() keeps its behaviour.

---
 include/linux/glob.h               |  1 +
 kernel/trace/trace_events_filter.c |  6 ++----
 lib/glob.c                         | 31 ++++++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 6 deletions(-)

index 861327b33e41..91595e750936 100644
@@ -6,5 +6,6 @@
 #include <linux/compiler.h>	/* For __pure */
 
 bool __pure glob_match(char const *pat, char const *str);
+bool __pure glob_match_len(char const *pat, char const *str, size_t len);
 
 #endif	/* _LINUX_GLOB_H */

index 609325f57942..6385cd662d8d 100644
@@ -1056,11 +1056,9 @@ static int regex_match_end(char *str, struct regex *r, int len)
 	return 0;
 }
 
-static int regex_match_glob(char *str, struct regex *r, int len __maybe_unused)
+static int regex_match_glob(char *str, struct regex *r, int len)
 {
-	if (glob_match(r->pattern, str))
-		return 1;
-	return 0;
+	return glob_match_len(r->pattern, str, len) ? 1 : 0;
 }
 
 /**

index 7aca76c25bcb..c80d9dd736b4 100644
@@ -11,6 +11,9 @@
 MODULE_DESCRIPTION("glob(7) matching");
 MODULE_LICENSE("Dual MIT/GPL");
 
+static bool __pure glob_match_str(char const *pat, char const *str,
+				  char const *str_end);
+
 /**
  * glob_match - Shell-style pattern matching, like !fnmatch(pat, str, 0)
  * @pat: Shell-style pattern to match, e.g. "*.[ch]".

@@ -40,6 +43,29 @@ MODULE_LICENSE("Dual MIT/GPL");
  * An opening bracket without a matching close is matched literally.
  */
 bool __pure glob_match(char const *pat, char const *str)
+{
+	return glob_match_str(pat, str, NULL);
+}
+EXPORT_SYMBOL(glob_match);
+
+/**
+ * glob_match_len - glob match against a length-bounded string
+ * @pat: Shell-style pattern to match.
+ * @str: String to match.  Need not be NUL-terminated.
+ * @len: Number of bytes of @str that may be read.
+ *
+ * Like glob_match(), but @str is only read up to @len bytes, so it can be
+ * used on buffers that are not NUL-terminated (e.g. trace event fields).
+ * A NUL byte within @len still terminates the string.
+ */
+bool __pure glob_match_len(char const *pat, char const *str, size_t len)
+{
+	return glob_match_str(pat, str, str + len);
+}
+EXPORT_SYMBOL(glob_match_len);
+
+static bool __pure glob_match_str(char const *pat, char const *str,
+				  char const *str_end)
 {
 	/*
 	 * Backtrack to previous * on mismatch and retry starting one

@@ -55,9 +81,11 @@ bool __pure glob_match(char const *pat, char const *str)
 	 * on mismatch, or true after matching the trailing nul bytes.
 	 */
 	for (;;) {
-		unsigned char c = *str++;
+		unsigned char c = (str_end && str >= str_end) ? '\0' : *str;
 		unsigned char d = *pat++;
 
+		str++;
+
 		switch (d) {
 		case '?':	/* Wildcard: anything but nul */
 			if (c == '\0')

@@ -125,4 +153,3 @@ bool __pure glob_match(char const *pat, char const *str)
 		}
 	}
 }
-EXPORT_SYMBOL(glob_match);

2.53.0

From: Steven Rostedt <[email protected]>

Tracepoints require that RCU is watching. To prevent them from being used in places that RCU is not watching, the trace_##event() macro always calls rcu_is_watching() even when the event is not enabled and warns if RCU is not watching. This is to make sure a warning is triggered even if the tracepoint is never enabled (as it is only a bug when it is).

It was noticed that tracepoints could be hidden within trace_#event#_enabled() calls, which are used to do extra work for the tracepoint only if the tracepoint is enabled. But this also can hide the fact that a tracepoint is placed in a location that can be called when RCU is not watching.

Commit 9764e731ef6ab ("tracepoint: Add lockdep rcu_is_watching() check to trace_##name##_enabled()") added a check to the trace_##event##_enabled() macro to make sure RCU is watching when it is called to make sure not to hide the bug of a tracepoint being called when RCU is not watching.

There is one case in the irq_disable tracepoint where it is within a trace_irq_disable_enabled() block, but it checks if RCU is watching, and if it isn't, it makes a call to ct_irq_enter() that makes RCU watch again. But because trace_irq_disable_enabled() now checks if RCU is watching and will trigger if it isn't. This is a false warning as the code within the block handles this case.

Add a new internal macro __trace_##event##_enabled() that doesn't check if RCU is watching, and convert the irq_enable/disable tracepoints over to it.

---
 include/linux/tracepoint.h      | 12 +++++++++++-
 kernel/trace/trace_preemptirq.c |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

index 4a0c36f40fe2..e0d838c9ce93 100644
@@ -292,13 +292,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	{								\
 	}								\
 	static inline bool						\
+	__trace_##name##_enabled(void)					\
+	{								\
+		return static_branch_unlikely(&__tracepoint_##name.key);\
+	}								\
+	static inline bool						\
 	trace_##name##_enabled(void)					\
 	{								\
 		if (IS_ENABLED(CONFIG_LOCKDEP)) {			\
 			WARN_ONCE(!rcu_is_watching(),			\
 				  "RCU not watching for tracepoint");	\
 		}							\
-		return static_branch_unlikely(&__tracepoint_##name.key);\
+		return __trace_##name##_enabled();			\
 	}
 
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto)			\

@@ -457,6 +462,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	{								\
 	}								\
 	static inline bool						\
+	__trace_##name##_enabled(void)					\
+	{								\
+		return false;						\
+	}								\
+	static inline bool						\
 	trace_##name##_enabled(void)					\
 	{								\
 		return false;						\

index 0c42b15c3800..b63e3558948f 100644
@@ -30,7 +30,7 @@
 #else
 #define trace(point, args)					\
 	do {							\
-		if (trace_##point##_enabled()) {		\
+		if (__trace_##point##_enabled()) {		\
 			bool exit_rcu = false;			\
 			if (in_nmi())				\
 				break;				\

2.53.0