Re: [PATCH v3] tracefs: Add read-only eventfs filesystem at /sys/kernel/events

On Tue, 11 Aug 2026 10:28:18 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:

> Hi Steve,
> 
> I have some comments on this.
> 
> On Mon, 10 Aug 2026 16:07:08 -0400
> Steven Rostedt <[email protected]> wrote:
> 
> > @@ -551,20 +643,40 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> >  		if (strcmp(name, entry->name) != 0)
> >  			continue;
> >  
> > +		if (ro && !entry->read_only)
> > +			return NULL;
> > +
> >  		data = ei->data;
> >  		if (entry->callback(name, &mode, &data, &fops) <= 0)
> >  			return NULL;
> >  
> > +		if (ro)
> > +			mode |= 0444;  
> 
> Don't we need to clear writable bits? e.g.
> 
> 	mode = (mode & ~0222) | 0444;

Sure.

> 
> [...]
> > @@ -812,6 +937,52 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
> >  	return ERR_PTR(-ENOMEM);
> >  }
> >  
> > +/**
> > + * eventfs_create_events_dir_ro - create a read-only events directory
> > + * @name: The name of the top level directory to create.
> > + * @entries: A list of entries that represent the files under this directory
> > + * @size: The number of @entries
> > + * @data: The default data to pass to the files (an entry may override it).  
> 
> This document need to be updated too. It should be "eventfs_create_events_ro_copy"
> and only takes @name and @ei.

Bah, I thought I fixed that. I may have but lost the changes in a rebase.

> 
> > + *
> > + * This function configures the eventfs filesystem root as a read-only
> > + * trace event directory using the existing eventfs_inode lazy-lookup
> > + * infrastructure.
> > + *
> > + * See eventfs_create_dir() for use of @entries.
> > + */
> > +int eventfs_create_events_ro_copy(const char *name, struct eventfs_inode *ei)
> > +{
> > +	static struct dentry *dentry;
> > +	struct tracefs_inode *ti;
> > +	struct inode *inode;
> > +
> > +	/* Can only be called once. */
> > +	if (dentry)
> > +		return -EBUSY;
> > +
> > +	/* Reference acquired but never freed */
> > +	dentry = eventfs_ro_get_root();
> > +	if (IS_ERR(dentry))
> > +		return PTR_ERR(dentry);
> > +
> > +	inode = d_inode(dentry);
> > +
> > +	INIT_LIST_HEAD(&ei->children);
> > +	INIT_LIST_HEAD(&ei->list);  
> 
> Nit: This seems redundant because those lists are initialized in
> event_create_events_dir() already, and here we initialize again.
> Currently, there is no chance to add anything on these lists.
> (But if we add something on these lists, re-initializing will
> make the items orphaned silently.)

Nice catch. I'll fix it.

I also found some other issues with the superblock setup. This isn't
going to go into the next merge window as it's too late.

Thanks for looking at it,

-- Steve


> 
> > +
> > +	ti = get_tracefs(inode);
> > +	ti->flags |= TRACEFS_EVENT_INODE;
> > +	ti->private = ei;
> > +
> > +	inode->i_op = &eventfs_ro_dir_inode_operations;
> > +	inode->i_fop = &eventfs_ro_file_operations;
> > +
> > +	/* This is never freed */
> > +	dentry->d_fsdata = get_ei(ei);
> > +
> > +	return 0;
> > +}  
> 
> Thanks,
>