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

[Google]

Hi Steve,

I have some comments on this.

On Mon, 10 Aug 2026 16:07:08 -0400
Steven Rostedt <steven@xxxxxxxxxxx> 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;

[...]
> @@ -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.

> + *
> + * 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.)

> +
> +	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,

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>