On Wed, Aug 12, 2026 at 05:13:04PM +0000, [email protected] wrote:
> 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?
Fair enough... I'll WRITE_ONCE/READ_ONCE.
--
Vincent
>
> [ ... ]
>
> 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
