Re: [PATCH 1/2] tracing: Make the backup instance readonly

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 7 Jan 2026 11:41:33 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed,  7 Jan 2026 23:45:59 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since there is no reason to reuse the backup instance, make it
> > readonly. Note that only backup instances are readonly, because
> > other trace instances will be empty unless it is writable.
> > Only backup instances have copy entries from the original.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace.c        |   91 ++++++++++++++++++++++++++++++++-----------
> >  kernel/trace/trace.h        |    6 +++
> >  kernel/trace/trace_events.c |   14 +++++--
> >  3 files changed, 84 insertions(+), 27 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 38f7a7a55c23..725930f5980e 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -4888,6 +4888,9 @@ static int tracing_open(struct inode *inode, struct file *file)
> >  		int cpu = tracing_get_cpu(inode);
> >  		struct array_buffer *trace_buf = &tr->array_buffer;
> >  
> > +		if (trace_array_is_readonly(tr))
> > +			return -EPERM;
> 
> So this fails if someone opens a file in RDONLY mode?
> 
> Why?

Ah, that's a bug. Let me fix it.

> 
> 
> > +
> >  #ifdef CONFIG_TRACER_MAX_TRACE
> >  		if (tr->current_trace->print_max)
> >  			trace_buf = &tr->max_buffer;
> > @@ -6077,6 +6080,9 @@ static int __tracing_resize_ring_buffer(struct trace_array *tr,
> >  ssize_t tracing_resize_ring_buffer(struct trace_array *tr,
> >  				  unsigned long size, int cpu_id)
> >  {
> > +	if (trace_array_is_readonly(tr))
> > +		return -EPERM;
> 
> In fact, I don't think we need any of these.

Would you mean we should check readonly 

> 
> > +
> >  	guard(mutex)(&trace_types_lock);
> >  
> >  	if (cpu_id != RING_BUFFER_ALL_CPUS) {
> 
> 
> 
> > @@ -9353,12 +9381,16 @@ static void
> >  tracing_init_tracefs_percpu(struct trace_array *tr, long cpu)
> >  {
> >  	struct dentry *d_percpu = tracing_dentry_percpu(tr, cpu);
> > +	umode_t writable_mode = TRACE_MODE_WRITE;
> >  	struct dentry *d_cpu;
> >  	char cpu_dir[30]; /* 30 characters should be more than enough */
> >  
> >  	if (!d_percpu)
> >  		return;
> >  
> > +	if (trace_array_is_readonly(tr))
> > +		writable_mode = TRACE_MODE_READ;
> 
> This is more like what we should do with all the files in a read-only
> instance. Just make all files not allow writes.

Actually, that's my first prototype but it did not work (at least on
tracefs).
Superuser can write anything unless the file does not have .write
operation.

> 
> We may need to make sure they can't be changed to write as well. But that
> will require a change to tracefs (and eventfs).

Ah, you mean the permission check is not correctly done in tracefs/eventfs yet?

Thank you,

> 
> -- Steve
> 
> 
> > +
> >  	snprintf(cpu_dir, 30, "cpu%ld", cpu);
> >  	d_cpu = tracefs_create_dir(cpu_dir, d_percpu);
> >  	if (!d_cpu) {
> > @@ -9371,7 +9403,7 @@ tracing_init_tracefs_percpu(struct trace_array *tr, long cpu)
> >  				tr, cpu, &tracing_pipe_fops);
> >  
> >  	/* per cpu trace */
> > -	trace_create_cpu_file("trace", TRACE_MODE_WRITE, d_cpu,
> > +	trace_create_cpu_file("trace", writable_mode, d_cpu,
> >  				tr, cpu, &tracing_fops);
> >  
> >  	trace_create_cpu_file("trace_pipe_raw", TRACE_MODE_READ, d_cpu,
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>