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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] tracefs: bitwise OR in tracefs_apply_options() skips all inodes on remount
- [Medium] tracefs: missing TRACEFS_RO_EVENT_INODE on eventfs root inode
- [Low] tracefs: mismatched kernel-doc function name for __eventfs_root_lookup()
--

commit 922a8b2dbffd8c4ebf41d8e11ee0a6caecad79c2
Author: Anubhav Shelat <ashelat@redhat.com>

tracefs: Add read-only eventfs filesystem at /sys/kernel/events

This commit introduces a read-only eventfs pseudo-filesystem mounted at
/sys/kernel/events that reuses the eventfs_inode lazy-lookup infrastructure
to expose trace event format and id files to unprivileged users.

> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 6e3513b13cfa2..14a71dacf0c16 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c

[ ... ]

> @@ -506,14 +625,21 @@ static struct dentry *
>  lookup_file_dentry(struct dentry *dentry,
>  		   struct eventfs_inode *ei, int idx,
>  		   umode_t mode, void *data,
> -		   const struct file_operations *fops)
> +		   const struct file_operations *fops, bool ro)
>  {
>  	struct eventfs_attr *attr = NULL;
>  
> -	if (ei->entry_attrs)
> +	/*
> +	 * Read-only files gives all users read access,
> +	 * but also should make sure this is not writable.
> +	 * Read-only also does not allow modified attributes.
> +	 */
> +	if (ro)
> +		mode = (mode & ~0222) | 0444;
> +	else if (ei->entry_attrs)
>  		attr = &ei->entry_attrs[idx];
>  
> -	return lookup_file(ei, dentry, mode, attr, data, fops);
> +	return lookup_file(ei, dentry, mode, attr, data, fops, ro);
>  }
>  
>  /**
> @@ -521,14 +647,16 @@ lookup_file_dentry(struct dentry *dentry,
>   * @dir: in which a lookup is being done
>   * @dentry: file/dir dentry
>   * @flags: Just passed to simple_lookup()
> + * @ro: Denote if this is from a read-only eventfs
>   *
>   * Used to create dynamic file/dir with-in @dir, search with-in @ei
>   * list, if @dentry found go ahead and create the file/dir
>   */
>  
> -static struct dentry *eventfs_root_lookup(struct inode *dir,
> -					  struct dentry *dentry,
> -					  unsigned int flags)
> +static struct dentry *__eventfs_root_lookup(struct inode *dir,
> +					    struct dentry *dentry,
> +					    unsigned int flags,
> +					    bool ro)

[Severity: Low]
As reported by Sashiko on v5 [1], this isn't a bug, but since
eventfs_root_lookup() was renamed to __eventfs_root_lookup(), should the
function name in the kernel-doc comment header above it also be updated to
__eventfs_root_lookup to avoid a W=1 kernel-doc warning?

[ ... ]

> @@ -837,6 +995,48 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
>  	return ERR_PTR(-ENOMEM);
>  }
>  
> +/**
> + * eventfs_create_events_ro_copy - create a read-only events directory
> + * @ei: The eventfs_inode to use to create a read-only copy from
> + *
> + * This function configures the eventfs filesystem root as a read-only
> + * trace event directory using the existing eventfs_inode lazy-lookup
> + * infrastructure.
> + */
> +int eventfs_create_events_ro_copy(struct eventfs_inode *ei)
> +{
> +	static struct dentry *dentry;
> +	struct tracefs_inode *ti;
> +	struct inode *inode;
> +	int ret;
> +
> +	/* Can only be called once. */
> +	if (dentry)
> +		return -EBUSY;
> +
> +	/* Reference acquired but never freed */
> +	dentry = eventfs_ro_get_root();
> +	if (IS_ERR(dentry)) {
> +		ret = PTR_ERR(dentry);
> +		dentry = NULL;
> +		return ret;
> +	}
> +
> +	inode = d_inode(dentry);
> +
> +	ti = get_tracefs(inode);
> +	ti->flags |= TRACEFS_EVENT_INODE;
> +	ti->private = ei;

[Severity: Medium]
As reported by Sashiko on v4 [2], should eventfs_create_events_ro_copy()
also set TRACEFS_RO_EVENT_INODE in ti->flags on the root inode?

In eventfs_ro_fill_super(), the root inode is allocated via new_inode(sb),
which calls tracefs_alloc_inode() and places the root tracefs_inode on the
global tracefs_inodes list. Because TRACEFS_RO_EVENT_INODE is only set on
child inodes in eventfs_get_inode(), remounting tracefs with uid= or gid=
mount options (once the bitwise check in tracefs_apply_options() is fixed)
will still match the pinned /sys/kernel/events root inode, overwrite its
i_uid and i_gid, and redundantly call eventfs_remount() on the global
events tree.

> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index f3d6188a3b7b2..f895ee9ed4540 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -341,6 +341,14 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
>  		srcu_idx = eventfs_remount_lock();
>  		rcu_read_lock();
>  		list_for_each_entry_rcu(ti, &tracefs_inodes, list) {
> +
> +			/*
> +			 * Ignore read-only eventfs inodes as they are not
> +			 * part of tracefs.
> +			 */
> +			if (ti->flags | TRACEFS_RO_EVENT_INODE)
> +				continue;

[Severity: Medium]
Should this check in tracefs_apply_options() use a bitwise AND (&) instead
of a bitwise OR (|)?

Because TRACEFS_RO_EVENT_INODE is non-zero, (ti->flags |
TRACEFS_RO_EVENT_INODE) always evaluates to true, causing the loop to skip
every inode in tracefs_inodes when tracefs is remounted with uid= or gid=
options.

[1] https://lore.kernel.org/r/sashiko-outbox-195864@kernel.org
[2] https://lore.kernel.org/r/sashiko-outbox-194145@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103154159.1467e7b5@gandalf.local.home?part=1