Re: [PATCH v8 2/5] tracing: Use the stackmap from the ftrace stack recording path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] tracing: missing RCU grace period in update_marker_trace()
  on disable [1]
--

commit 7075e3df3e37a58db04cf4e9626607b29d78b5fc
Author: Pengfei Li <lipengfei28@xiaomi.com>

tracing: Use the stackmap from the ftrace stack recording path

This commit hooks the stackmap into __ftrace_trace_stack() when the new
stackmap trace option is enabled, recording a TRACE_STACK_ID event with
the stack cookie and adding a "free" command to stack_map to release the
map when disabled.

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 18710c190c924..cdae2976d224f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c

[ ... ]

> @@ -3996,6 +4101,15 @@ int set_tracer_flag(struct trace_array *tr, u64 mask, int enabled)
>  			return -EINVAL;
>  
>  	switch (mask) {
> +	case TRACE_ITER(STACKMAP):
> +		if (enabled) {
> +			int ret = stackmap_set_flag(tr);
> +
> +			if (ret)
> +				return ret;
> +		}
> +		break;
> +
>  	case TRACE_ITER(TRACE_PRINTK):

[Severity: Critical]
This problem wasn't introduced by this patch, but can disabling
TRACE_ITER(COPY_MARKER) in set_tracer_flag() cause a use-after-free of tr
or corrupt marker_copies [1]?

kernel/trace/trace.c:update_marker_trace() {
	...
	if (!(tr->trace_flags & TRACE_ITER(COPY_MARKER)))
		return false;

	list_del_rcu(&tr->marker_list);
	tr->trace_flags &= ~TRACE_ITER(COPY_MARKER);
	return true;
}

In set_tracer_flag(), the TRACE_ITER(COPY_MARKER) case calls
update_marker_trace() without waiting for an RCU grace period when
disabling the flag.  Meanwhile, tracing_mark_write() and
tracing_mark_raw_write() walk marker_copies under RCU without holding
event_mutex:

CPU0                                    CPU1
tracing_mark_write()
  rcu_read_lock_sched_notrace()
  list_for_each_entry_rcu(tr,
      &marker_copies, marker_list)
                                        set_tracer_flag(tr, COPY_MARKER, 0)
                                          update_marker_trace(tr, 0)
                                            list_del_rcu(&tr->marker_list)
                                            tr->trace_flags &= ~COPY_MARKER
                                        __remove_instance(tr)
                                          if (update_marker_trace(tr, 0))
                                            synchronize_rcu()  // skipped
                                          free_trace_buffers(tr)
                                          kfree(tr)
    __trace_array_vprintk(tr->...)  <-- use-after-free

Because COPY_MARKER was already cleared by set_tracer_flag(),
__remove_instance() sees update_marker_trace(tr, 0) return false, skips
synchronize_rcu(), and frees tr while CPU0 is still writing to it.
Similarly, toggling COPY_MARKER off and immediately back on re-inserts
tr->marker_list with list_add_rcu() before concurrent RCU readers have
finished traversing from it.

[1] https://sashiko.dev/#/bug/linux-3d780d38-c496-44e0-a5a4-9138bd4d11a0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107055139.3138633-1-lipengfei28@xiaomi.com?part=2