Re: [PATCH v5 15/18] tracing/remotes: Add poll_ms tracefs file

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