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

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