Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] Data race on `remote->poll_ms` due to lockless reads. - [Low] Improper mixing of goto-based error handling and scope-based cleanup (__free) in `trace_remote_iter`. --
--- Patch [15]: [PATCH v5 15/18] tracing/remotes: Add poll_ms tracefs file --- commit a661ee191c6d806ed0a716f67237aca890febe25 Author: Vincent Donnefort <[email protected]> tracing/remotes: Add poll_ms tracefs file Add a tracefs file to configure the trace remote polling period. Keep the default value to 100ms. > diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c > index 5536e0df43795..b1c75666ab638 100644 > --- a/kernel/trace/trace_remote.c > +++ b/kernel/trace/trace_remote.c > @@ -1135,6 +1135,40 @@ static int dump_on_panic_show(struct seq_file *s, void > *unused) > } > DEFINE_TRACE_REMOTE_ATTRIBUTE(dump_on_panic); > > +static ssize_t poll_ms_write(struct file *filp, const char __user *ubuf, > size_t cnt, loff_t *ppos) > +{ > + struct seq_file *seq = filp->private_data; > + struct trace_remote *remote = seq->private; > + unsigned int val; > + int ret; > + > + ret = kstrtouint_from_user(ubuf, cnt, 10, &val); > + if (ret) > + return ret; > + > + if (!val) > + return -EINVAL; > + > + guard(mutex)(&remote->lock); > + > + if (val < remote->poll_ms && remote->poll_cnt) > + mod_delayed_work(system_percpu_wq, &remote->poll_work, > msecs_to_jiffies(val)); > + > + remote->poll_ms = val; > + > + return cnt; > +} > + > +static int poll_ms_show(struct seq_file *s, void *unused) > +{ > + struct trace_remote *remote = s->private; > + > + seq_printf(s, "%u\n", remote->poll_ms); [Severity: Low] Does this code introduce a data race on remote->poll_ms? In poll_ms_write(), remote->poll_ms is modified while holding remote->lock. However, in poll_ms_show(), remote->poll_ms is read without holding remote->lock. It appears this could also cause a data race with lockless reads in trace_remote_do_poll(): kernel/trace/trace_remote.c:trace_remote_do_poll() { ... schedule_delayed_work(&remote->poll_work, yield ? 0 : msecs_to_jiffies(remote->poll_ms)); } Could this concurrent read and write cause a data race, potentially leading to torn reads or violating kernel concurrency expectations? [ ... ] Also, looking at trace_remote_iter() in kernel/trace/trace_remote.c: kernel/trace/trace_remote.c:trace_remote_iter() { struct trace_remote_iterator *iter __free(kfree) = kzalloc_obj(*iter); ... if (WARN_ON_ONCE(remote->dmesg)) { ret = -EBUSY; goto err; } ... err: switch (type) { case TRI_PANIC: break; default: trace_remote_put(remote); } return ERR_PTR(ret); } [Severity: Low] Does this code incorrectly mix goto-based error handling and scope-based cleanup (__free)? The cleanup subsystem guidelines prohibit mixing goto-based error paths with __free() variable declarations in the same function. While it does not appear to cause a concrete resource leak or double-free in this specific instance, could this create confusing ownership semantics and risk future bugs during refactoring? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
