On Wed, Feb 04, 2026 at 06:52:08PM -0500, Steven Rostedt wrote:
> On Sat, 31 Jan 2026 13:28:25 +0000
> Vincent Donnefort <[email protected]> wrote:
>
> > -static struct trace_remote_iterator *trace_remote_iter(struct trace_remote
> > *remote, int cpu)
> > +static void __free_ring_buffer_iter(struct trace_remote_iterator *iter,
> > int cpu)
> > +{
> > + if (!iter->rb_iter)
> > + return;
>
> Hmm, can't iter->rb_iter be NULL when iter->rb_iters[] is used?
Arg yes, I missed that when I removed the union. And actually I don't think this
can be called with iter->rb_iter or iter->rb_iter NULL anymore.
>
> > +
> > + if (cpu != RING_BUFFER_ALL_CPUS) {
> > + ring_buffer_read_finish(iter->rb_iter);
> > + return;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + if (iter->rb_iters[cpu])
> > + ring_buffer_read_finish(iter->rb_iters[cpu]);
> > + }
> > +
> > + kfree(iter->rb_iters);
> > +}
> > +
> > +static int __alloc_ring_buffer_iter(struct trace_remote_iterator *iter,
> > int cpu)
> > +{
> > + if (cpu != RING_BUFFER_ALL_CPUS) {
> > + iter->rb_iter =
> > ring_buffer_read_start(iter->remote->trace_buffer, cpu, GFP_KERNEL);
> > +
> > + return iter->rb_iter ? 0 : -ENOMEM;
> > + }
> > +
> > + iter->rb_iters = kcalloc(nr_cpu_ids, sizeof(*iter->rb_iters),
> > GFP_KERNEL);
> > + if (!iter->rb_iters)
> > + return -ENOMEM;
> > +
> > + for_each_possible_cpu(cpu) {
> > + iter->rb_iters[cpu] =
> > ring_buffer_read_start(iter->remote->trace_buffer, cpu,
> > + GFP_KERNEL);
> > + if (!iter->rb_iters[cpu]) {
> > + __free_ring_buffer_iter(iter, RING_BUFFER_ALL_CPUS);
>
> For instance, we call __free_ring_buffer_iter() here, but I don't see
> iter->rb_iter being set.
>
> -- Steve
>
>
> > + return -ENOMEM;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
[...]
> > +static void *trace_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > + struct trace_remote_iterator *iter = m->private;
> > +
> > + ++*pos;
> > +
> > + if (!iter || !trace_remote_iter_read_event(iter))
> > + return NULL;
> > +
> > + trace_remote_iter_move(iter);
> > + iter->pos++;
> > +
> > + return iter;
> > +}
> > +
> > +static void *trace_start(struct seq_file *m, loff_t *pos)
> > +{
> > + struct trace_remote_iterator *iter = m->private;
> > + loff_t i;
> > +
>
> FYI, this is where you take locks for iteration of files.
>
> > + if (!iter)
> > + return NULL;
> > +
> > + if (!*pos) {
> > + iter->pos = -1;
> > + return trace_next(m, NULL, &i);
> > + }
> > +
> > + i = iter->pos;
> > + while (i < *pos) {
> > + iter = trace_next(m, NULL, &i);
> > + if (!iter)
> > + return NULL;
> > + }
> > +
> > + return iter;
> > +}
> > +
> > +static int trace_show(struct seq_file *m, void *v)
> > +{
> > + struct trace_remote_iterator *iter = v;
> > +
> > + trace_seq_init(&iter->seq);
> > +
> > + if (trace_remote_iter_print_event(iter)) {
> > + seq_printf(m, "[EVENT %d PRINT TOO BIG]\n", iter->evt->id);
> > + return 0;
> > + }
> > +
> > + return trace_print_seq(m, &iter->seq);
> > +}
> > +
> > +static void trace_stop(struct seq_file *s, void *v) { }
>
> And stop is where you release the locks.
>
> > +
> > +static const struct seq_operations trace_sops = {
> > + .start = trace_start,
> > + .next = trace_next,
> > + .show = trace_show,
> > + .stop = trace_stop,
> > +};
> > +
> > +static int trace_open(struct inode *inode, struct file *filp)
> > +{
> > + struct trace_remote *remote = inode->i_private;
> > + struct trace_remote_iterator *iter = NULL;
> > + int cpu = tracing_get_cpu(inode);
> > + int ret;
> > +
> > + if (!(filp->f_mode & FMODE_READ))
> > + return 0;
> > +
> > + guard(mutex)(&remote->lock);
> > +
> > + iter = trace_remote_iter(remote, cpu, TRI_NONCONSUMING);
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
>
> So if iter is bad we exit out here.
>
> > +
> > + ret = seq_open(filp, &trace_sops);
> > + if (ret) {
> > + trace_remote_iter_free(iter);
> > + return ret;
> > + }
> > +
> > + if (iter)
>
> Why test if iter exists here?
We only test IS_ERR. iter will be NULL if the buffer isn't loaded and the
userspace output would be empty. But anyway if I move the locking into
start/stop this line will go away!
>
> > + trace_remote_iter_read_start(iter);
>
> But still, the above grabs locks in the open, where it can return to user
> space while still holding the locks? That's a no-no.
>
> You can use the seq file start and stop for locking.
>
> -- Steve
>
[...]
