tracing: Fixes for v7.3

tracing fixes for 7.3:

- Don't destroy user event fields when removal fails

User event fields are destroyed before the event is removed from visibility. But that can fail leaving the still visible event with no fields. Move the destroying of the fields to after the event is successfully removed from visibility.

- Initialize function graph state is fork before calling copy_exec_state()

For non-CLONE_VM forks, copy_exec_state() allocates a new task_exec_state. If that allocation fails, ftrace_graph_exit_task() will free the tasks ret_stack pointer. Since that pointer is still using the parent's ret_stack, it mistakenly frees the parent's pointer too.

Call ftrace_graph_init() on the task first which will NULL out the new tasks's ret_stack and if the copy fails, it will not free anything.

- Remove FGRAPH_MAX_INDEX

The macro FGRAPH_MAX_INDEX was added but never used. Remove it.

- Save ent_size in function graph printing of nested functions

The function graph tracer needs to look at the next event to see if the next event is the return of the current function entry. If it is, it prints a single line:

    ktime_get();

Otherwise it prints it like a nested function:

    tick_nohz_irq_exit() {
      ktime_get();
      kcpustat_irq_exit();
    }

In order to look at the next event, it must save the current event so that it has the information to print from it. It saves the event in the iterator descriptor called "ent". What it doesn't save is the ent_size of the event which is now used to know if the function graph arguments are to be printed. The peek doesn't save the size so the size used happens to be that of the size of the last event that was seen.

Save the entry event size in the iterator descriptor so that the correct size is used.

- Fix several errors with freeing data in the histogram code

The histogram code had a lot of leaked or or incorrect accounting when failures happen. Correct them.

- Fix histogram regression of .percent and .graph modifiers

Up until 6.3 histogram values could have "percent" or "graph" modifiers that changed how they were printed. But a change that added restricting histograms values from being strings, stack traces and other modifiers inadvertently prevented them from using the percent and graph modifiers, which were legal use cases for values.

Put back the percent and graph modifiers.

- Fix various typos in the comments

- Set the trace_clock before initializing a histogram with clock argument

The histogram API allows the user to specific which trace clock to use via a "clock=" string. The histogram is set up first before the clock is checked. If the passed in clock is not valid, it exits without fully fixing up the histogram leaving it on the list and a use-after-free can trigger.

Update the clock argument first and if it fails then exit gracefully before the histogram trigger is placed on any lists.

- Restore :mod: trailer after parsing in ftrace_set_clr_event

The function ftrace_set_clr_event() modifies the parse string and needs to put it back to what was passed in. It searches for ":mod:" via a strsep() but fails to put back the first ':' in the string.

Add back the ':' in the passed in string.

- Take trace_array reference when opening a tracer options file

The options files are dynamically created and some tracers add their own options. When a tracer adds their own list of options, the trace_array holding them has an array to hold the list of options for each tracer. This array increases in size via a krealloc(), and the new entry gets a newly allocated array to hold the options of the new tracer being added.

The element in each entry of the tracer's option array holds a pointer back to the trace_array, a pointer to the tracer it is associated to, a pointer to the flags of the option.

The issue is that these arrays are freed when the trace_array is freed when its instance it represents is removed from the instances directory. There's a race that an open of one of these options files can happen when the instance is being removed.

Add a new helper function to be called by the open function of the options file to iterate all existing trace_arrays under a lock and find the one that has the given option element in one of it's tracer arrays. If found, then update the associated trace_array's reference counter to keep it from being freed. If not found, have the open call return -ENODEV.

- Disable interrupts when acquiring the lock in rb_wake_up_waiters()

The function rb_wake_up_waiters() assumes it will be called in interrupt context and does not disable irqs when taking cpu_buffer->reader_lock, which can be called in hard interrupt context. The issue is in PREEMPT_RT, this function is called in thread context leaving this lock open to a deadlock.

Take the lock with interrupts disabled.

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

Head SHA1: 815e07c8fe885a87751c2496a30ae0dcd4118210

Donggeun Yoo (12):
fgraph: Remove unused FGRAPH_MAX_INDEX
function_graph: Use the saved entry's size when reprinting it
tracing: Free histogram var refs regardless of how often they are referenced
tracing: Free histogram the var ref when its initialization fails
tracing: Free histogram the field rejected for a bad modifier
tracing: Keep the entry count when the histogram stats allocation fails
tracing: Let histogram values keep the percent and graph modifiers
tracing: Set the trace clock before registering the histogram trigger
tracing: Take the reference before publishing the named histogram trigger
tracing: Undo the registration when enabling the histogram trigger fails
tracing: Fix memory corruption from the histogram stacktrace modifier
tracing: Fix memory corruption from a "STACKTRACE" histogram key

Hemanth Selam (2):
tracing: Fix typo "availabe" in comment
tracing: Fix typo "preceeded" in comment

Henry Martin (1):
tracing/user_events: Don't destroy fields when event removal fails

Jérémy Jean (1):
ftrace: fork: Initialize function graph state before copy_exec_state()

Karl Mehltretter (1):
tracing: Fix ring_buffer_read_page_size() kernel-doc

Sebastian Andrzej Siewior (1):
ring-buffer: Acquire the lock with irqsave in rb_wake_up_waiters()

Steven Rostedt (1):
tracing: Take trace_array reference when opening a tracer options file

Thomas Weißschuh (1):
tracing: Restore :mod: trailer after parsing in ftrace_set_clr_event()

----

 include/trace/events/timer.h         |  2 +-
 kernel/fork.c                        |  7 ++-
 kernel/trace/fgraph.c                |  3 --
 kernel/trace/rethook.c               |  2 +-
 kernel/trace/ring_buffer.c           | 15 +++----
 kernel/trace/trace.c                 | 46 ++++++++++++++++++-
 kernel/trace/trace.h                 |  1 +
 kernel/trace/trace_events.c          |  2 +
 kernel/trace/trace_events_hist.c     | 86 +++++++++++++++++++-----------------
 kernel/trace/trace_events_user.c     | 26 ++++++++---
 kernel/trace/trace_functions_graph.c |  3 ++
 11 files changed, 130 insertions(+), 63 deletions(-)

From: Henry Martin <[email protected]>

destroy_user_event() destroys the event's fields before attempting to remove the trace event call. If user_event_set_call_visible() fails, e.g. because the event is still enabled and trace_remove_event_call() returns -EBUSY, the event is left registered with an irreversibly destroyed field list. Any subsequent interaction with the event then operates on an empty field list while it is still fully visible in tracefs.

Move the field destruction after the call removal, and splice the field list back onto the event when the removal fails so the event remains in a consistent state.

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

index 93cda2f6f269..f658c3a77aa7 100644
@@ -1122,10 +1122,9 @@ static void user_event_destroy_validators(struct user_event *user)
 	}
 }
 
-static void user_event_destroy_fields(struct user_event *user)
+static void user_event_destroy_fields(struct list_head *head)
 {
 	struct ftrace_event_field *field, *next;
-	struct list_head *head = &user->fields;
 
 	list_for_each_entry_safe(field, next, head, link) {
 		list_del(&field->link);

@@ -1502,17 +1501,32 @@ static int user_event_set_call_visible(struct user_event *user, bool visible)
 
 static int destroy_user_event(struct user_event *user)
 {
+	LIST_HEAD(fields);
 	int ret = 0;
 
 	lockdep_assert_held(&event_mutex);
 
-	/* Must destroy fields before call removal */
-	user_event_destroy_fields(user);
+	/*
+	 * Detach the fields before removing the call. Removing the event
+	 * frees the field list memory (trace_destroy_fields() is run on
+	 * successful removal and kmem_cache_free()s the fields), but the
+	 * fields here are allocated and owned by user_events. Destroy
+	 * them separately once removal has succeeded.
+	 */
+	list_splice_init(&user->fields, &fields);
 
 	ret = user_event_set_call_visible(user, false);
 
-	if (ret)
+	if (ret) {
+		/*
+		 * Removal failed and the event stays registered, recover
+		 * the fields so it is left in a consistent state.
+		 */
+		list_splice(&fields, &user->fields);
 		return ret;
+	}
+
+	user_event_destroy_fields(&fields);
 
 	dyn_event_remove(&user->devent);
 	hash_del(&user->node);

@@ -2212,7 +2226,7 @@ static int user_event_parse(struct user_event_group *group, char *name,
 put_user_lock:
 	mutex_unlock(&event_mutex);
 put_user:
-	user_event_destroy_fields(user);
+	user_event_destroy_fields(&user->fields);
 	user_event_destroy_validators(user);
 	kfree(user->call.print_fmt);
 

2.53.0

From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Jean?= <[email protected]>

dup_task_struct() copies the parent's task_struct, including ret_stack. ftrace_graph_init_task() clears the copied function graph state, but it currently runs after copy_exec_state().

For non-CLONE_VM forks, copy_exec_state() allocates a new task_exec_state. If that allocation fails, copy_process() reaches bad_fork_free and free_task() calls ftrace_graph_exit_task(). Since the child still carries the parent's ret_stack pointer, the unwind frees the parent's active function graph return stack. The parent subsequently accesses freed memory from function_graph_enter_regs().

KASAN reports:

[   22.190920] ==================================================================
[   22.195899] BUG: KASAN: slab-use-after-free in function_graph_enter_regs+0xa76/0xb90
[   22.200747] Write of size 8 at addr ff110000054dc0a8 by task repro/1
[   22.205134]
[   22.210770] CPU: 0 UID: 0 PID: 1 Comm: repro Not tainted 7.2.0-07732-g9328b3b03bdc-dirty #3 PREEMPT(lazy)
[   22.212576] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   22.213750] Call Trace:
[   22.215271]  <TASK>
[   22.216242]  ? ftrace_stub_direct_tramp+0x10/0x10
[   22.217774]  dump_stack_lvl+0x4e/0x70
[   22.220531]  print_report+0x157/0x4b4
[   22.223202]  ? fixup_red_left+0x9/0x30
[   22.224407]  ? complete_report_info+0x83/0x110
[   22.226679]  ? function_graph_enter_regs+0xa76/0xb90
[   22.228084]  kasan_report+0xce/0x100
[   22.230109]  ? function_graph_enter_regs+0xa76/0xb90
[   22.232860]  ? stack_trace_save+0x4/0xd0
[   22.234156]  function_graph_enter_regs+0xa76/0xb90
[   22.236090]  ? kasan_save_stack+0x30/0x50
[   22.237752]  ? __pfx_function_graph_enter_regs+0x10/0x10
[   22.238694]  ? ring_buffer_lock_reserve+0x345/0xf80
[   22.239628]  ? stack_trace_save+0x4/0xd0
[   22.242121]  ? stack_trace_save+0x4/0xd0
[   22.243588]  ftrace_graph_func+0xda/0x160
[   22.245362]  ? ftrace_stub_direct_tramp+0x10/0x10
[   22.246520]  0xffffffffa0000095
[   22.250528]  ? stack_trace_save+0x9/0xd0
[   22.251757]  ? ring_buffer_unlock_commit+0x11d/0x5c0
[   22.253152]  stack_trace_save+0x9/0xd0
[   22.254264]  kasan_save_stack+0x30/0x50
[   22.273631]  kasan_save_track+0x14/0x30
[   22.276763]  kasan_save_free_info+0x3b/0x70
[   22.278296]  __kasan_slab_free+0x43/0x70
[   22.280157]  kmem_cache_free+0xbf/0x3b0
[   22.282963]  ? ftrace_stub_direct_tramp+0x10/0x10
[   22.284001]  free_task+0xa2/0x160
[   22.285699]  ? ftrace_stub_direct_tramp+0x10/0x10
[   22.286752]  copy_process+0x2aae/0x7bc0

Initialize the child function graph state immediately after dup_task_struct(), before the first fallible operation.

---
 kernel/fork.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 416758c8a3d4..a5934a317634 100644
@@ -2133,6 +2133,11 @@ __latent_entropy struct task_struct *copy_process(
 	p = dup_task_struct(current, node);
 	if (!p)
 		goto fork_out;
+	/*
+	 * Must run before the first fallible op, so error paths never
+	 * free the parent's ret_stack.
+	 */
+	ftrace_graph_init_task(p);
 	retval = copy_exec_state(clone_flags, p);
 	if (retval)
 		goto bad_fork_free;

@@ -2159,8 +2164,6 @@ __latent_entropy struct task_struct *copy_process(
 	 */
 	p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? args->child_tid : NULL;
 
-	ftrace_graph_init_task(p);
-
 	rt_mutex_init_task(p);
 	raw_spin_lock_init(&p->blocked_lock);
 

2.53.0

From: Donggeun Yoo <[email protected]>

FGRAPH_MAX_INDEX has no user, and it expands to FGRAPH_INDEX_SIZE and FGRAPH_RET_INDEX, neither of which is defined anywhere in the tree. It was added in that form by commit 91c46b0aa917 ("function_graph:
Implement fgraph_reserve_data() and fgraph_retrieve_data()"), which introduced the current data word layout under new names, so anything referencing it would have failed to build ever since.

Remove it.

---
 kernel/trace/fgraph.c | 3 ---
 1 file changed, 3 deletions(-)

index 40d373d65f9b..ed455b53513b 100644
@@ -143,9 +143,6 @@ enum {
 #define FGRAPH_DATA_INDEX_MASK	GENMASK(FGRAPH_DATA_INDEX_BITS - 1, 0)
 #define FGRAPH_DATA_INDEX_SHIFT	(FGRAPH_DATA_SHIFT + FGRAPH_DATA_BITS)
 
-#define FGRAPH_MAX_INDEX	\
-	((FGRAPH_INDEX_SIZE << FGRAPH_DATA_BITS) + FGRAPH_RET_INDEX)
-
 #define FGRAPH_ARRAY_SIZE	FGRAPH_INDEX_BITS
 
 /*

2.53.0

From: Donggeun Yoo <[email protected]>

When a graph entry does not fit in the trace_seq, print_graph_entry() saves it in the iterator's fgraph_data and reprints it on the next read. The entry has already been consumed from the ring buffer by then, so the copy is all that is left of it.

The copy is sized with iter->ent_size, which no longer describes the saved entry but whatever entry the iterator has moved on to. The argument count is derived from the same field, so a 72 byte entry saved and then reprinted ahead of a 48 byte return entry loses its arguments.

Record the size next to the failure flag, so that the two are always set together, and restore it before reprinting.

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

index ff7cb1a76b95..c5befd0c5b9a 100644
@@ -52,6 +52,7 @@ struct fgraph_data {
 	};
 	struct ftrace_graph_ret_entry	ret;
 	int				failed;
+	int				ent_size;
 	int				cpu;
 };
 

@@ -1274,6 +1275,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
 		if (s->full) {
 			data->failed = 1;
 			data->cpu = cpu;
+			data->ent_size = iter->ent_size;
 		} else
 			data->failed = 0;
 	}

@@ -1457,6 +1459,7 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
 	if (data && data->failed) {
 		field = &data->ent.ent;
 		iter->cpu = data->cpu;
+		iter->ent_size = data->ent_size;
 		ret = print_graph_entry(field, s, iter, flags);
 		if (ret == TRACE_TYPE_HANDLED && iter->cpu != cpu) {
 			per_cpu_ptr(data->cpu_data, iter->cpu)->ignore = 1;

2.53.0

From: Donggeun Yoo <[email protected]>

Using the same variable three or more times in one hist trigger leaks the variable reference and its strings when the trigger is removed.

commit 656fe2ba85e8 ("tracing: Use hist trigger's var_ref array to destroy var_refs") made a trigger's var_refs[] array the only owner of a var ref:
destroy_hist_field() returns early for HIST_FIELD_FL_VAR_REF, so the field expressions never destroy one. One entry, freed once, no count needed.

commit 8bcebc77e85f ("tracing: Fix histogram code when expression has same var as value") then made repeated references share one object and added a count of them. Only the increment side exists, since those expressions still return early and never drop a reference, so __destroy_hist_field() sees how many references were created rather than how many are left. It frees when the decremented count is 0 or 1, so two references work and three or more leak.

Sharing kept one array entry per object, and create_var_ref() searches and appends within a single trigger, so nothing outside it holds the object. Removing a trigger whose variables are still referenced is already refused by check_var_refs() with -EBUSY. Drop the count and free unconditionally.

---
 kernel/trace/trace_events_hist.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

index 963e0d6b61fd..f90680b33a37 100644
@@ -169,7 +169,6 @@ struct hist_field {
 	struct hist_field		*operands[HIST_FIELD_OPERANDS_MAX];
 	struct hist_trigger_data	*hist_data;
 	enum hist_field_fn		fn_num;
-	unsigned int			ref;
 	unsigned int			size;
 	unsigned int			offset;
 	unsigned int                    is_signed;

@@ -1913,16 +1912,8 @@ static int contains_operator(char *str, char **sep)
 	return field_op;
 }
 
-static void get_hist_field(struct hist_field *hist_field)
-{
-	hist_field->ref++;
-}
-
 static void __destroy_hist_field(struct hist_field *hist_field)
 {
-	if (--hist_field->ref > 1)
-		return;
-
 	kfree(hist_field->var.name);
 	kfree(hist_field->name);
 

@@ -1969,8 +1960,6 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
 	if (!hist_field)
 		return NULL;
 
-	hist_field->ref = 1;
-
 	hist_field->hist_data = hist_data;
 
 	if (flags & HIST_FIELD_FL_EXPR || flags & HIST_FIELD_FL_ALIAS)

@@ -2223,10 +2212,8 @@ static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
 	for (i = 0; i < hist_data->n_var_refs; i++) {
 		ref_field = hist_data->var_refs[i];
 		if (ref_field->var.idx == var_field->var.idx &&
-		    ref_field->var.hist_data == var_field->hist_data) {
-			get_hist_field(ref_field);
+		    ref_field->var.hist_data == var_field->hist_data)
 			return ref_field;
-		}
 	}
 	/* Sanity check to avoid out-of-bound write on 'hist_data->var_refs' */
 	if (hist_data->n_var_refs >= TRACING_MAP_VARS_MAX)

@@ -3276,7 +3263,6 @@ static struct hist_field *create_var(struct hist_trigger_data *hist_data,
 		goto out;
 	}
 
-	var->ref = 1;
 	var->flags = HIST_FIELD_FL_VAR;
 	var->var.idx = idx;
 	var->var.hist_data = var->hist_data = hist_data;

2.53.0

From: Donggeun Yoo <[email protected]>

create_var_ref() allocates a VAR_REF hist_field and then calls init_var_ref() to fill it in. When that fails the field is leaked.

commit 656fe2ba85e8 ("tracing: Use hist trigger's var_ref array to destroy var_refs") made destroy_hist_field() return early for HIST_FIELD_FL_VAR_REF, since var refs are freed by walking the trigger's var_refs[] array instead. create_var_ref() adds the field to that array only after init_var_ref() has succeeded, so on this path the field is in neither place and nothing frees it. The call was correct when it was written, before var refs were taken out of destroy_hist_field().

init_var_ref() cannot free it either. The caller owns the field, so init_var_ref() undoes only its own string allocations and leaves the field alone. Freeing it there would leave create_var_ref() passing freed memory to destroy_hist_field(), which reads its flags.

Call __destroy_hist_field(), which frees the field without consulting the flag.

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

index f90680b33a37..bbdd56208eff 100644
@@ -2221,7 +2221,7 @@ static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
 	ref_field = create_hist_field(var_field->hist_data, NULL, flags, NULL);
 	if (ref_field) {
 		if (init_var_ref(ref_field, var_field, system, event_name)) {
-			destroy_hist_field(ref_field, 0);
+			__destroy_hist_field(ref_field);
 			return NULL;
 		}
 

2.53.0

From: Donggeun Yoo <[email protected]>

Writing a hist trigger whose value or variable carries a modifier that is not allowed there leaks the fields that were built for it.

__create_val_field() takes the field from parse_expr() and stores it in hist_data->fields[] only after the modifier checks have run:

	hist_field = parse_expr(hist_data, file, field_str, flags, var_name,
				&n_subexprs);
	...
	if (hist_field->flags & HIST_FIELD_FL_VAR) {
		if (hist_field->flags & (...))
			goto err;
	} else {
		if (hist_field->flags & (...))
			goto err;
	}

	hist_data->fields[val_idx] = hist_field;

Both checks jump past that store, and the err label returns without freeing anything. The error unwinds to create_hist_data(), which calls destroy_hist_data() -> destroy_hist_fields(), and that reaches a field only by walking fields[]. A field that never got there is unreachable.

commit e0213434fe3e ("tracing: Do not let histogram values have some modifiers") set ret to -EINVAL and fell through to the store, which left the field owned by fields[] and freed along with the rest of hist_data. Splitting the check into a value case and a variable case replaced that fall-through with a goto that skips it.

With CONFIG_DEBUG_KMEMLEAK, 200 writes of

  # echo 'hist:keys=prev_pid:vals=next_pid.log2' > \
	 events/sched/sched_switch/trigger

each correctly rejected with -EINVAL, leave 332 unreferenced objects (63744 bytes) reported at create_hist_field(); 200 install and remove cycles of a valid trigger leave none. A '.log2' field is two allocations, since create_hist_field() puts the plain field in operands[0] of the log2 field, and both are reported.

Use destroy_hist_field() rather than __destroy_hist_field() so that operands[0] is freed as well. It returns early for HIST_FIELD_FL_VAR_REF, which is what an operand owned by hist_data->var_refs[] needs; the rejected field itself is never a var ref, because a var ref never carries a modifier flag.

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

index bbdd56208eff..8cad99a8d01e 100644
@@ -4317,6 +4317,7 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
 	return ret;
  err:
 	hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
+	destroy_hist_field(hist_field, 0);
 	return -EINVAL;
 }
 

2.53.0

From: Donggeun Yoo <[email protected]>

print_entries() uses n_entries both as the number of sort entries and as its own return value, so the -ENOMEM it stores when the stats allocation fails overwrites the count that the cleanup still needs:

	n_entries = tracing_map_sort_entries(map, ...);
	if (n_entries < 0)
		return n_entries;
	...
			if (!stats) {
				n_entries = -ENOMEM;
				goto out;
			}
	...
 out:
	tracing_map_destroy_sort_entries(sort_entries, n_entries);

tracing_map_destroy_sort_entries() takes an unsigned int and loops up to it, so -ENOMEM arrives as 4294967284. It walks an array of at most map->max_elts pointers and calls destroy_sort_entry(), which dereferences and frees, on whatever lies past the end.

Reading the hist file of a trigger with a .percent value, with that allocation forced to fail:

  BUG: KASAN: vmalloc-out-of-bounds in tracing_map_destroy_sort_entries+0xa0/0xb0
  Read of size 8 at addr ffffc90000045000 by task init/1
   tracing_map_destroy_sort_entries+0xa0/0xb0
   hist_show+0x6f7/0x1df0
   seq_read_iter+0x2b8/0x1190
   vfs_read+0x176/0xa40
  The buggy address belongs to a 4-page vmalloc region starting at
  ffffc90000041000 allocated at tracing_map_sort_entries+0x5c/0xd50

A few pages further the fault is fatal. The registers at the oops confirm the bound: the loop's end pointer less the array start, over the pointer size, is 4294967284.

Return the error in a separate variable and leave n_entries holding the count, the way tracing_map_sort_entries() does on its own error path.

The stats block is only entered for a value carrying .percent or .graph, which __create_val_field() has rejected since v6.3, so this cannot be reached in mainline as it stands. It becomes reachable again with "tracing: hist: let values keep the percent and graph modifiers", so it should be applied first.

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

index 8cad99a8d01e..8d80562fb502 100644
@@ -5677,7 +5677,7 @@ static int print_entries(struct seq_file *m,
 {
 	struct tracing_map_sort_entry **sort_entries = NULL;
 	struct tracing_map *map = hist_data->map;
-	int i, j, n_entries;
+	int i, j, n_entries, ret;
 	struct hist_val_stat *stats = NULL;
 	u64 val;
 

@@ -5687,6 +5687,8 @@ static int print_entries(struct seq_file *m,
 	if (n_entries < 0)
 		return n_entries;
 
+	ret = n_entries;
+
 	/* Calculate the max and the total for each field if needed. */
 	for (j = 0; j < hist_data->n_vals; j++) {
 		if (!(hist_data->fields[j]->flags &

@@ -5695,7 +5697,7 @@ static int print_entries(struct seq_file *m,
 		if (!stats) {
 			stats = kzalloc_objs(*stats, hist_data->n_vals);
 			if (!stats) {
-				n_entries = -ENOMEM;
+				ret = -ENOMEM;
 				goto out;
 			}
 		}

@@ -5716,7 +5718,7 @@ static int print_entries(struct seq_file *m,
 out:
 	tracing_map_destroy_sort_entries(sort_entries, n_entries);
 
-	return n_entries;
+	return ret;
 }
 
 static void hist_trigger_show(struct seq_file *m,

2.53.0

From: Donggeun Yoo <[email protected]>

The .percent and .graph modifiers exist only for histogram values, but a value carrying either of them has been rejected since v6.3. The example in Documentation/trace/histogram.rst,

  # echo 'hist:keys=prev_comm:vals=hitcount.percent:nohitcount' > \
	 events/sched/sched_switch/trigger

returns -EINVAL.

parse_field() sets the two flags only when the field is neither a key nor a variable, that is, only on a value:

	} else if (strncmp(modifier, "percent", 7) == 0) {
		if (*flags & (HIST_FIELD_FL_VAR | HIST_FIELD_FL_KEY))
			goto error;
		*flags |= HIST_FIELD_FL_PERCENT;

__create_val_field() then rejects a value for carrying them, so no field can reach hist_trigger_print_val(), where both are implemented.

commit e0213434fe3e ("tracing: Do not let histogram values have some modifiers") added the check after a value with .buckets oopsed in hist_field_name(). That happens because .buckets and .log2 make create_hist_field() build a nested field in operands[0] which hist_field_name() then walks into. The percent and graph flags do not create an operand and are not read by hist_field_name(); they are only used when printing a value.

Stop rejecting the two flags on a value. The check for variables is left alone, where they are unreachable anyway because parse_field() rejects a variable carrying them first.

With the two flags removed, the trigger above installs and prints as documented:

  { prev_comm: rcu_preempt  }  hitcount (%):   0.00
  { prev_comm: init         }  hitcount (%):  99.98
  Totals:
      Hits: 237896

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

index 8d80562fb502..5e00da2d5b1a 100644
@@ -4299,8 +4299,7 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
 			goto err;
 	} else {
 		/* Value */
-		if (hist_field->flags & (HIST_FIELD_FL_GRAPH | HIST_FIELD_FL_PERCENT |
-					 HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
+		if (hist_field->flags & (HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
 					 HIST_FIELD_FL_SYM | HIST_FIELD_FL_SYM_OFFSET |
 					 HIST_FIELD_FL_SYSCALL | HIST_FIELD_FL_STACKTRACE))
 			goto err;

2.53.0

From: Hemanth Selam <[email protected]>

Correct "availabe" to "available", reported by scripts/checkpatch.pl using the misspelling list in scripts/spelling.txt. Only touches comments, no code changes.

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

index 5a8bdf88999a..87a27f3aa4a6 100644
@@ -171,7 +171,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
 	 * This expects the caller will set up a rethook on a function entry.
 	 * When the function returns, the rethook will eventually be reclaimed
 	 * or released in the rethook_recycle() with call_rcu().
-	 * This means the caller must be run in the RCU-availabe context.
+	 * This means the caller must be run in the RCU-available context.
 	 */
 	if (unlikely(!rcu_is_watching()))
 		return NULL;

2.53.0

From: Hemanth Selam <[email protected]>

Correct "preceeded" to "Preceded", reported by scripts/checkpatch.pl using the misspelling list in scripts/spelling.txt. Only touches comments, no code changes.

---
 include/trace/events/timer.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ca82fd62dc30..3aa0608c6361 100644
@@ -302,7 +302,7 @@ DECLARE_EVENT_CLASS(hrtimer_class,
  * hrtimer_start_expired - Invoked when a expired timer was started
  * @hrtimer:	pointer to struct hrtimer
  *
- * Preceeded by a hrtimer_start tracepoint.
+ * Preceded by a hrtimer_start tracepoint.
  */
 DEFINE_EVENT(hrtimer_class, hrtimer_start_expired,
 

2.53.0

From: Donggeun Yoo <[email protected]>

hist_register_trigger() puts the trigger on the global named_triggers list in cmd_ops->init(), and only then sets the trace clock:

	if (data->cmd_ops->init) {
		ret = data->cmd_ops->init(data);
		if (ret < 0)
			goto out;
	}

	if (hist_data->enable_timestamps) {
		ret = tracing_set_clock(file->tr, hist_data->attrs->clock);
		if (ret) {
			hist_err(tr, HIST_ERR_SET_CLOCK_FAIL, errpos(clock));
			goto out;
		}

The clock string is not checked anywhere before that call, so a named trigger using common_timestamp with an unknown clock fails after it has already become findable. event_hist_trigger_parse() then frees it without taking it off the list, and the next lookup by name reads the freed object:

~# cd /sys/kernel/tracing/events/sched/sched_switch
~# echo 'hist:name=foo:keys=common_pid:ts=common_timestamp:clock=bogus' > trigger
bash: echo: write error: Invalid argument
~# echo 'hist:name=foo:keys=common_pid' > trigger

  BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
  Read of size 8 at addr ffff88800915d760 by task init/1
   find_named_trigger+0xac/0xc0
   hist_register_trigger+0xc1/0x900
   event_hist_trigger_parse+0x3146/0x6af0
   event_trigger_write+0xce/0x160
  Freed by task 63:
   kfree+0x154/0x420
   trigger_kthread_fn+0xfd/0x160

Set the clock before the trigger is registered, so that nothing which can fail runs after it is published, the way commit 6f86bdeab633 ("tracing: Fix bad hist from corrupting named_triggers list") moved the registration below the rest of the setup.

tracing_set_filter_buffering() is reference counted, so the init failure path has to drop the reference that the clock block now takes first.

---
 kernel/trace/trace_events_hist.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index 5e00da2d5b1a..1889e310b73c 100644
@@ -6631,12 +6631,6 @@ static int hist_register_trigger(char *glob,
 		data->cmd_ops = cmd_ops;
 	}
 
-	if (data->cmd_ops->init) {
-		ret = data->cmd_ops->init(data);
-		if (ret < 0)
-			goto out;
-	}
-
 	if (hist_data->enable_timestamps) {
 		char *clock = hist_data->attrs->clock;
 

@@ -6649,6 +6643,15 @@ static int hist_register_trigger(char *glob,
 		tracing_set_filter_buffering(file->tr, true);
 	}
 
+	if (data->cmd_ops->init) {
+		ret = data->cmd_ops->init(data);
+		if (ret < 0) {
+			if (hist_data->enable_timestamps)
+				tracing_set_filter_buffering(file->tr, false);
+			goto out;
+		}
+	}
+
 	if (named_data) {
 		remove_hist_vars(hist_data);
 		destroy_hist_data(hist_data);

2.53.0

From: Donggeun Yoo <[email protected]>

event_hist_trigger_named_init() puts the trigger on the global named_triggers list and only then takes the reference on the trigger it shares its histogram with:

	data->ref++;

	save_named_trigger(data->named_data->name, data);

	ret = event_hist_trigger_init(data->named_data);
	if (ret < 0) {
		kfree(data->cmd_ops);
		data->cmd_ops = &trigger_hist_cmd;
	}

	return ret;

event_hist_trigger_init() fails when alloc_hist_pad() cannot allocate, and nothing takes the trigger back off the list on the way out. event_hist_trigger_parse() frees it, and the next lookup by name reads the freed object:

 BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
 Read of size 8 at addr ffff888009346860 by task init/1
  find_named_trigger+0xac/0xc0
  hist_register_trigger+0xc1/0xa00
  event_hist_trigger_parse+0x3146/0x6af0
  event_trigger_write+0xce/0x160
 Freed by task 67:
  kfree+0x154/0x420
  trigger_kthread_fn+0xfd/0x160

Do the reference first and publish once it has succeeded, so that nothing which can fail runs after the trigger becomes findable.

---
 kernel/trace/trace_events_hist.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

index 1889e310b73c..54c95f9bd0a3 100644
@@ -6371,17 +6371,18 @@ static int event_hist_trigger_named_init(struct event_trigger_data *data)
 {
 	int ret;
 
-	data->ref++;
-
-	save_named_trigger(data->named_data->name, data);
-
 	ret = event_hist_trigger_init(data->named_data);
 	if (ret < 0) {
 		kfree(data->cmd_ops);
 		data->cmd_ops = &trigger_hist_cmd;
+		return ret;
 	}
 
-	return ret;
+	data->ref++;
+
+	save_named_trigger(data->named_data->name, data);
+
+	return 0;
 }
 
 static void event_hist_trigger_named_free(struct event_trigger_data *data)

2.53.0

From: Donggeun Yoo <[email protected]>

Commit 6f86bdeab633 ("tracing: Fix bad hist from corrupting named_triggers list") described how a trigger that is registered but not on file->triggers ends up freed while still on the global named_triggers list, and moved the registration down so that hist_trigger_enable() follows it immediately. One path still gets there. hist_trigger_enable() adds the trigger and takes it straight back out when the event cannot be enabled:

	list_add_tail_rcu(&data->list, &file->triggers);

	update_cond_flag(file);

	if (trace_event_trigger_enable_disable(file, 1) < 0) {
		list_del_rcu(&data->list);
		update_cond_flag(file);
		ret--;
	}

so the list walk in hist_unregister_trigger() matches nothing, test stays NULL, and the ->free() that would call del_named_trigger() is skipped. out_unreg falls through to out_free, which frees the trigger anyway:

 BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
 Read of size 8 at addr ffff8880091d3160 by task init/1
  find_named_trigger+0xac/0xc0
  hist_register_trigger+0xc1/0xa00
  event_hist_trigger_parse+0x3146/0x6af0
  event_trigger_write+0xce/0x160
 Freed by task 69:
  kfree+0x154/0x420
  trigger_kthread_fn+0xfd/0x160

Leave the trigger where hist_unregister_trigger() can find it and let that undo the registration, which is the only code that knows all of what cmd_ops->init() took: the named list entry, the hist_pad reference, the reference on the trigger a named histogram is shared with, and the copied cmd_ops. It also pairs the failed trace_event_trigger_enable_disable(), whose sm_ref and buffered event reference are otherwise left behind.

Since ->free() releases trigger_data and, for a trigger that does not share its histogram, hist_data with it, out_unreg can no longer fall through to out_free. For a trigger that does share, hist_register_trigger() has already destroyed the caller's hist_data, so the fall-through was reading freed memory there as well.

Move the enable_timestamps check in hist_unregister_trigger() above the ->free() call for the same reason: hist_data does not outlive it once the trigger being removed is the one that owns it.

---
 kernel/trace/trace_events_hist.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

index 54c95f9bd0a3..53100287466f 100644
@@ -6670,11 +6670,12 @@ static int hist_trigger_enable(struct event_trigger_data *data,
 
 	update_cond_flag(file);
 
-	if (trace_event_trigger_enable_disable(file, 1) < 0) {
-		list_del_rcu(&data->list);
-		update_cond_flag(file);
+	/*
+	 * On failure the caller undoes the registration, and
+	 * hist_unregister_trigger() can only find the trigger here.
+	 */
+	if (trace_event_trigger_enable_disable(file, 1) < 0)
 		ret--;
-	}
 
 	return ret;
 }

@@ -6752,13 +6753,13 @@ static void hist_unregister_trigger(char *glob,
 		}
 	}
 
-	if (test && test->cmd_ops->free)
-		test->cmd_ops->free(test);
-
 	if (hist_data->enable_timestamps) {
 		if (!hist_data->remove || test)
 			tracing_set_filter_buffering(file->tr, false);
 	}
+
+	if (test && test->cmd_ops->free)
+		test->cmd_ops->free(test);
 }
 
 static bool hist_file_check_refs(struct trace_event_file *file)

@@ -6963,6 +6964,8 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
 	return ret;
  out_unreg:
 	event_trigger_unregister(cmd_ops, file, glob+1, trigger_data);
+	/* The unregister frees trigger_data, skip out_free */
+	goto out;
  out_free:
 	remove_hist_vars(hist_data);
 

2.53.0

From: Donggeun Yoo <[email protected]>

parse_field() sets HIST_FIELD_FL_STACKTRACE from the ".stacktrace" modifier before it looks the field name up, and nothing afterwards checks that the name resolved to a field which holds a stacktrace. create_hist_field() picks HIST_FIELD_FN_STACK on the strength of the field pointer alone, which reads a __data_loc word from the record and follows its low 16 bits as an offset into the same record. event_hist_trigger() takes the first word there as an entry count and copies that many longs into a 31 entry array:

	n_entries = *stack;
	memcpy(entries, ++stack, n_entries * sizeof(unsigned long));

Neither end of that copy is bounded, and the count is whatever the event holds at the offset, so any field will do:

  # cd /sys/kernel/tracing/events/sched/sched_process_fork
  # echo 'hist:keys=parent_pid.stacktrace' > trigger
  # (true)

  BUG: kernel NULL pointer dereference, address: 0000000000000008
  RIP: 0010:rb_insert_color+0x18/0x130
   timerqueue_linked_add+0x7e/0xd0
   enqueue_hrtimer+0x39/0xb0
   __hrtimer_run_queues+0x10f/0x1f0
   </IRQ>
  RIP: 0010:memcpy+0xc/0x30
   event_hist_trigger+0x165/0x690

The timer interrupt landed on the rbtree the copy had already run over. No debug options are needed for this; KASAN reports the same write as an out-of-bounds read of 13835058055416381440 bytes.

Documentation/trace/histogram.rst already states the rule, "must be a long[] type", so enforce it once the name has been resolved. Names which resolve to no field at all, "hitcount.stacktrace" and the common_* pseudo-fields, are refused for the same reason: they hold no stacktrace to read.

---
 kernel/trace/trace_events_hist.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index 53100287466f..9bc829c1e876 100644
@@ -2317,6 +2317,7 @@ parse_field(struct hist_trigger_data *hist_data, struct trace_event_file *file,
 	struct ftrace_event_field *field = NULL;
 	char *field_name, *modifier, *str;
 	struct trace_array *tr = file->tr;
+	bool stack_modifier = false;
 
 	modifier = str = kstrdup(field_str, GFP_KERNEL);
 	if (!modifier)

@@ -2339,9 +2340,10 @@ parse_field(struct hist_trigger_data *hist_data, struct trace_event_file *file,
 			*flags |= HIST_FIELD_FL_EXECNAME;
 		else if (strcmp(modifier, "syscall") == 0)
 			*flags |= HIST_FIELD_FL_SYSCALL;
-		else if (strcmp(modifier, "stacktrace") == 0)
+		else if (strcmp(modifier, "stacktrace") == 0) {
 			*flags |= HIST_FIELD_FL_STACKTRACE;
-		else if (strcmp(modifier, "log2") == 0)
+			stack_modifier = true;
+		} else if (strcmp(modifier, "log2") == 0)
 			*flags |= HIST_FIELD_FL_LOG2;
 		else if (strcmp(modifier, "usecs") == 0)
 			*flags |= HIST_FIELD_FL_TIMESTAMP_USECS;

@@ -2412,6 +2414,12 @@ parse_field(struct hist_trigger_data *hist_data, struct trace_event_file *file,
 			}
 		}
 	}
+
+	if (stack_modifier &&
+	    (!field || field->filter_type != FILTER_STACKTRACE)) {
+		hist_err(tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
+		field = ERR_PTR(-EINVAL);
+	}
  out:
 	kfree(str);
 

2.53.0

From: Donggeun Yoo <[email protected]>

"cpu", "CPU", "stacktrace" and "STACKTRACE" are generic fields, defined with an offset and a size of zero so that the filter code can match them by name. parse_field() maps them onto their common_* equivalents for backward compatibility, but unlike the common_* names it hands the placeholder back to the caller instead of NULL.

create_hist_field() takes a non-NULL field as a promise that the record carries a stacktrace and picks HIST_FIELD_FN_STACK, so the __data_loc word is read from offset 0, that is from common_type, and its low 16 bits are followed as an offset into the record. What is found there becomes the length of an unbounded memcpy. Pick an event whose id is small enough that the offset stays inside its own record and the length is a kernel text address:

  # cd /sys/kernel/tracing
  # echo 'hist:keys=STACKTRACE' > events/ftrace/print/trigger
  # echo hello > trace_marker

  Oops: general protection fault, probably for non-canonical address
  RIP: 0010:rb_next+0x23/0x60
   </IRQ>
  RIP: 0010:memcpy+0xc/0x30
   event_hist_trigger+0x2e7/0x12c0
  Kernel panic - not syncing: Fatal exception in interrupt

Leave the field NULL, which is what the comment above the branch says the code does and what common_stacktrace already does. FILTER_CPU and FILTER_COMM are left alone, their create_hist_field() branches never look at the field.

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

index 9bc829c1e876..8af97fd4ee2d 100644
@@ -2404,6 +2404,7 @@ parse_field(struct hist_trigger_data *hist_data, struct trace_event_file *file,
 				*flags |= HIST_FIELD_FL_CPU;
 			} else if (field && field->filter_type == FILTER_STACKTRACE) {
 				*flags |= HIST_FIELD_FL_STACKTRACE;
+				field = NULL;
 			} else if (field && field->filter_type == FILTER_COMM) {
 				*flags |= HIST_FIELD_FL_COMM | HIST_FIELD_FL_STRING;
 			} else {

2.53.0

From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <[email protected]>

While ftrace_set_clr_event() modifies its input buffer during parsing, before returning to the caller the buffer is supposed to be restored to its original state.

This works correctly for the colon between the subsystem and event but not the colon at the beginning of :mod:.

Restore the colon, so the :mod: trailer is not stripped after ftrace_set_clr_event().

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

index 9dbc2441763b..30c0ddf90887 100644
@@ -1489,6 +1489,8 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 	/* Put back the colon to allow this to be called again */
 	if (buf)
 		*(buf - 1) = ':';
+	if (mod)
+		*(mod - 5) = ':';
 
 	return ret;
 }

2.53.0

From: Karl Mehltretter <[email protected]>

ring_buffer_read_page_size() takes a parameter named rpage, but its kernel-doc describes page. As a result, kernel-doc reports rpage as undescribed and page as an excess parameter description.

Rename the documentation entry to match the function.

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

index 9c03a555a6ba..0e30c7bd6045 100644
@@ -7384,7 +7384,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_read_page_data);
 
 /**
  * ring_buffer_read_page_size - get size of the read page.
- * @page:  the page to get the size from
+ * @rpage:  the page to get the size from
  *
  * Returns size of the page in bytes.
  */

2.53.0

From: Steven Rostedt <[email protected]>

When a tracer option file is opened, it is passed a descriptor that points to an element on the trace_array's topts array. This element has information to find the trace array and other information. It uses this element to take a reference of the trace_array so that the trace_array does not get removed while this file is opened.

Unfortunately, there's a race condition where the element itself could be freed by the removal of the instance the trace_array represents causing a use-after-free as this element that is used to find the trace_array to increment its reference counter is also freed when the instance is removed.

To solve this, add a trace_array_tracer_options_get() helper function that will take the address of the element that is passed to the open function by the inode->i_private pointer and search all the trace_arrays under a lock to find the one that the element's address is in the range of the trace_arrays topts array elements. When a match happens, that trace_array's reference would be increased.

Note, there's a race where if an admin was deleting and creating trace instances at the same time and the memory of the old trace_array's array matched the memory of the new trace_array that it could in theory open the option from the wrong trace array. But we do not care because it would be stupid to perform that kind of action. As long as the only thing that can happen is that the option from the wrong trace array is used and doesn't crash the kernel it will only make the user confused. But if they are doing something stupid like this, they are already confused, so no harm done.

---
 kernel/trace/trace.c | 46 +++++++++++++++++++++++++++++++++++++++++++-
 kernel/trace/trace.h |  1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

index 8658cad53cb5..e4a490d3d08c 100644
@@ -7717,12 +7717,55 @@ trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	return cnt;
 }
 
+static bool tr_option_match(struct trace_array *tr, void *topt)
+{
+	for (int i = 0; i < tr->nr_topts; i++) {
+		struct trace_options *tr_topts = &tr->topts[i];
+
+		if (topt >= (void *)&tr_topts->topts[0] &&
+		    topt < (void *)&tr_topts->topts[tr_topts->nr_topts])
+			return true;
+	}
+	return false;
+}
+
+/*
+ * The topt is the address of a trace_array->topts[] element that holds the
+ * the tracer options descriptor. But since the trace_array reference has not
+ * been taken yet, it cannot be dereferenced as it could have been freed by
+ * a rmdir of the instance the trace_array represents.
+ *
+ * Search the list of trace_arrays and compare the topt to the address of
+ * the entire trace_array topts array for each trace_array in the list.
+ * If one is matched, then take the reference and return it. If not, the
+ * trace_array no longer exits.
+ */
+static int trace_array_tracer_options_get(void *topt)
+{
+	struct trace_array *tr;
+	int ret;
+
+	ret = security_locked_down(LOCKDOWN_TRACEFS);
+	if (ret)
+		return ret;
+
+	if (tracing_disabled)
+		return -ENODEV;
+
+	guard(mutex)(&trace_types_lock);
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		if (tr_option_match(tr, topt))
+			return __trace_array_get(tr);
+	}
+	return -ENODEV;
+}
+
 static int tracing_open_options(struct inode *inode, struct file *filp)
 {
 	struct trace_option_dentry *topt = inode->i_private;
 	int ret;
 
-	ret = tracing_check_open_get_tr(topt->tr);
+	ret = trace_array_tracer_options_get(topt);
 	if (ret)
 		return ret;
 

@@ -7984,6 +8027,7 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer,
 	tr->topts = tr_topts;
 	tr->topts[tr->nr_topts].tracer = tracer;
 	tr->topts[tr->nr_topts].topts = topts;
+	tr->topts[tr->nr_topts].nr_topts = cnt;
 	tr->nr_topts++;
 
 	for (cnt = 0; opts[cnt].name; cnt++) {

index 5e76f94e7a80..bd3c8f80300f 100644
@@ -227,6 +227,7 @@ struct array_buffer {
 struct trace_options {
 	struct tracer			*tracer;
 	struct trace_option_dentry	*topts;
+	int				nr_topts;
 };
 
 struct trace_pid_list *trace_pid_list_alloc(void);

2.53.0

From: Sebastian Andrzej Siewior <[email protected]>

rb_wake_up_waiters() is a irq_work callback which is initialized with init_irq_work(). As such it will be invoked in thread context on PREEMPT_RT. Invoking the callback in IRQ context on PREEMPT_RT is not an option due its usage of wake_up_all(). Since this callback may run in thread context, it needs to acquire ring_buffer_per_cpu::reader_lock with disabling interrupts and may not assume that they are disabled.

Use raw_spinlock_irqsave() to acquire ring_buffer_per_cpu::reader_lock.

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

index 0e30c7bd6045..9bc8ce8c5676 100644
@@ -904,14 +904,13 @@ static void rb_wake_up_waiters(struct irq_work *work)
 		struct ring_buffer_per_cpu *cpu_buffer =
 			container_of(rbwork, struct ring_buffer_per_cpu, irq_work);
 
-		/* Called from interrupt context */
-		raw_spin_lock(&cpu_buffer->reader_lock);
-		rbwork->wakeup_full = false;
-		rbwork->full_waiters_pending = false;
+		scoped_guard(raw_spinlock_irqsave, &cpu_buffer->reader_lock) {
+			rbwork->wakeup_full = false;
+			rbwork->full_waiters_pending = false;
 
-		/* Waking up all waiters, they will reset the shortest full */
-		cpu_buffer->shortest_full = 0;
-		raw_spin_unlock(&cpu_buffer->reader_lock);
+			/* Waking up all waiters, they will reset the shortest full */
+			cpu_buffer->shortest_full = 0;
+		}
 
 		wake_up_all(&rbwork->full_waiters);
 	}

2.53.0