Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] tracing: missing validation and truncation of buf_size in trace_remote - [Low] tracing: data race in trace_remote cmdline parsing due to missing WRITE_ONCE --
commit 6359480e2f0f5e3b0022cfdf6db7a85cba580136 Author: Vincent Donnefort <[email protected]> tracing/remotes: Add trace_remote cmdline options This patch adds a command-line interface to configure trace remotes upon registration. It parses options such as dump_on_panic, dmesg, buf_size, and poll parameters, and enables tracing automatically when events are activated. > diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c > index b2547eb117f85..de41f36e9a0e3 100644 > --- a/kernel/trace/trace_remote.c > +++ b/kernel/trace/trace_remote.c [ ... ] > +static bool trace_remote_apply_cmdline_opts(struct trace_remote *remote, > char *cmdline) > +{ > + bool dmesg_on = false; > + char *opt; > + > + while ((opt = strsep(&cmdline, "^"))) { > + if (!*opt) > + continue; > + > + if (!strcmp(opt, "dump_on_panic")) { > + WRITE_ONCE(remote->panic_on, true); > + } else if (!strcmp(opt, "dmesg")) { > + dmesg_on = true; > + } else if (!strncmp(opt, "buf_size=", 9)) { > + WRITE_ONCE(remote->trace_buffer_size, memparse(opt + 9, > NULL)); [Severity: Medium] Is it possible for this to silently truncate large buffer sizes on 32-bit architectures? Since memparse() returns a 64-bit unsigned long long, but remote->trace_buffer_size is an unsigned long (32 bits on 32-bit architectures), values like 4G might truncate. Additionally, what happens if a privileged user specifies buf_size=0 on the command line? It seems this bypasses validation and might cause undefined behavior in ring buffer allocation or remote driver initialization. > + } else if (!strncmp(opt, "poll=", 5)) { > + unsigned int poll_ms; > + > + if (!kstrtouint(opt + 5, 10, &poll_ms) && poll_ms > 0) > + remote->poll_ms = poll_ms; [Severity: Low] Should this assignment use WRITE_ONCE()? Concurrent lockless readers like poll_ms_show() access this variable via READ_ONCE(). A plain assignment here without WRITE_ONCE() violates kernel concurrency rules and could trigger a KCSAN data race warning if accessed while the boot cmdline is being applied during remote registration. > + else > + pr_warn("Invalid trace remote poll '%s'\n", > opt); > + } else { > + pr_warn("Unknown trace remote option '%s'\n", opt); > + } > + } > + > + return dmesg_on; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
