On Tue, 03 Nov 2026 01:05:27 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] tracing: successful marker write overwritten by tail error in
> marker_copies
> --
>
> commit 2345280f3b3625fa6e36116fc50947e792b25719
> Author: Jaewon Jung <[email protected]>
>
> tracing: Keep copying markers when one instance is stopped
>
> This commit removes the early break on write errors in tracing_mark_write()
> and tracing_mark_raw_write() when iterating over marker_copies so that
> stopped instances do not prevent remaining instances from receiving markers.
>
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 08614af8fad1c..0b66c55d378b1 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -6330,8 +6330,6 @@ tracing_mark_write(struct file *filp, const char
> > __user *ubuf,
> > guard(rcu)();
> > list_for_each_entry_rcu(tr, &marker_copies, marker_list) {
> > written = write_marker_to_buffer(tr, buf, cnt, ip);
> > - if (written < 0)
> > - break;
> > }
> > } else {
> > written = write_marker_to_buffer(tr, buf, cnt, ip);
> > @@ -6409,8 +6407,6 @@ tracing_mark_raw_write(struct file *filp, const char
> > __user *ubuf,
> > guard(rcu)();
> > list_for_each_entry_rcu(tr, &marker_copies, marker_list) {
> > written = write_raw_marker_to_buffer(tr, buf, cnt);
> > - if (written < 0)
> > - break;
>
> [Severity: Medium]
> As reported by Steven Rostedt on v1 [1], should errors from stopped
> instances in marker_copies be prevented from overwriting a successful write
> return value?
>
> When instances enable copy_trace_marker, update_marker_trace() adds each
> instance to the head of marker_copies:
>
> kernel/trace/trace.c:update_marker_trace() {
> if (enabled) {
> if (tr->trace_flags & TRACE_ITER(COPY_MARKER))
> return false;
>
> list_add_rcu(&tr->marker_list, &marker_copies);
> tr->trace_flags |= TRACE_ITER(COPY_MARKER);
> return true;
> }
>
> This leaves earlier-enabled instances and global_trace at the tail of
> marker_copies.
>
> Because tracing_mark_write() and tracing_mark_raw_write() unconditionally
> assign written on every iteration of marker_copies, if an earlier instance
> in the list is active and receives the marker (setting written = cnt), but
> the last instance in marker_copies has tracing disabled,
> write_marker_to_buffer() or write_raw_marker_to_buffer() will return -EBADF
> on that tail instance. Won't that overwrite written and cause the syscall
> to return -EBADF to userspace even though the marker was successfully
> written to the active instance(s)?
>
We only want to record the the output of the global array, so we should have it
be:
if (tr == &global_trace) {
guard(rcu)();
list_for_each_entry_rcu(tr, &marker_copies, marker_list) {
ssize_t w = write_marker_to_buffer(tr, buf, cnt, ip);
if (tr == @global_trace)
written = w;
}
-- Steve
