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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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