Re: [PATCH v5] 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: tracefs_apply_options() overwrites eventfs_ro inode ownership
- [Medium] tracefs: eventfs_ro_fill_super() sets i_nlink to 2 on dynamic eventfs root
- [Low] tracefs: mismatched kernel-doc function name for __eventfs_root_lookup()
--

commit 8a085766e708c607f0750dab297b23f146dfe8b9
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 exposes trace event format and id files with mode
0444 to unprivileged users. It reuses the existing eventfs_inode
lazy-lookup infrastructure and tracefs inode allocator on a separate
read-only superblock.

> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 6e3513b13cfa2..43ae382cfbbfc 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -16,14 +16,96 @@
>   */
>  #include <linux/fsnotify.h>
>  #include <linux/fs.h>
> +#include <linux/fs_context.h>
>  #include <linux/namei.h>
>  #include <linux/workqueue.h>
>  #include <linux/security.h>
>  #include <linux/tracefs.h>
> +#include <linux/magic.h>
>  #include <linux/kref.h>
>  #include <linux/delay.h>
>  #include "internal.h"
>  
> +static struct vfsmount *eventfs_ro_mount;
> +static int eventfs_ro_mount_count;
> +
> +static int eventfs_ro_fill_super(struct super_block *sb, struct fs_context *fc)
> +{
> +	struct inode *inode;
> +	struct dentry *root;
> +
> +	sb->s_blocksize = PAGE_SIZE;
> +	sb->s_blocksize_bits = PAGE_SHIFT;
> +	sb->s_magic = EVENTFS_SUPER_MAGIC;
> +	sb->s_op = &eventfs_ro_super_operations;
> +	sb->s_time_gran = 1;
> +	sb->s_flags |= SB_RDONLY;
> +
> +	inode = new_inode(sb);
> +	if (!inode)
> +		return -ENOMEM;
> +
> +	inode->i_ino = 1;
> +	inode->i_mode = S_IFDIR | 0555;
> +	simple_inode_init_ts(inode);
> +	inode->i_op = &simple_dir_inode_operations;
> +	inode->i_fop = &simple_dir_operations;
> +	set_nlink(inode, 2);

[Severity: Medium]
Should the root inode keep i_nlink at 1 rather than calling
set_nlink(inode, 2) in eventfs_ro_fill_super()?

When eventfs_create_events_ro_copy() later configures this root inode as the
top-level eventfs directory with eventfs_ro_dir_inode_operations and
eventfs_ro_file_operations, its subsystem subdirectories are created
dynamically on demand in lookup_dir_entry() without incrementing the parent
directory link count.

As noted in eventfs_create_events_dir(), eventfs directories keep i_nlink == 1
so userspace tools do not trust the link count:

    /*
     * Keep all eventfs directories with i_nlink == 1.
     * Due to the dynamic nature of the dentry creations and not
     * wanting to add a pointer to the parent eventfs_inode in the
     * eventfs_inode structure, keeping the i_nlink in sync with the
     * number of directories would cause too much complexity for
     * something not worth much. Keeping directory links at 1
     * tells userspace not to trust the link number.
     */

With i_nlink set to 2 on /sys/kernel/events, won't userspace directory
traversal tools such as find and fts assume the directory has no
subdirectories (st_nlink == 2) and skip recursing into the event subsystem
directories?

[ ... ]

> @@ -308,12 +404,17 @@ void eventfs_remount(struct tracefs_inode *ti, bool update_uid, bool update_gid)
>  }
>  
>  static void update_inode_attr(struct inode *inode, umode_t mode,
> -			      struct eventfs_attr *attr, struct eventfs_root_inode *rei)
> +			      struct eventfs_attr *attr, struct eventfs_root_inode *rei,
> +			      bool ro)
>  {
> +	inode->i_mode = mode;
> +
> +	/* The read-only version does not allow updates to owership or permissions */
> +	if (ro)
> +		return;
> +
>  	if (attr && attr->mode & EVENTFS_SAVE_MODE)
>  		inode->i_mode = attr->mode & EVENTFS_MODE_MASK;
> -	else
> -		inode->i_mode = mode;
>  
>  	if (attr && attr->mode & EVENTFS_SAVE_UID)
>  		inode->i_uid = attr->uid;

[ ... ]

> @@ -407,22 +512,28 @@ static struct dentry *lookup_file(struct eventfs_inode *parent_ei,
>   * @dentry: the directory to look up
>   * @pei: Pointer to the parent eventfs_inode if available
>   * @ei: the eventfs_inode that represents the directory to create
> + * @ro: boolean to denote if this lookup is for the read-only version
>   *
>   * This function will look up a dentry for a directory represented by
>   * a eventfs_inode.
>   */
>  static struct dentry *lookup_dir_entry(struct dentry *dentry,
> -	struct eventfs_inode *pei, struct eventfs_inode *ei)
> +	struct eventfs_inode *pei, struct eventfs_inode *ei, bool ro)
>  {
>  	struct inode *inode;
>  	umode_t mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
>  
> -	inode = eventfs_get_inode(dentry, &ei->attr, mode, ei);
> +	inode = eventfs_get_inode(dentry, &ei->attr, mode, ei, ro);
>  	if (unlikely(!inode))
>  		return ERR_PTR(-ENOMEM);
>  
> -	inode->i_op = &eventfs_dir_inode_operations;
> -	inode->i_fop = &eventfs_file_operations;
> +	if (ro) {
> +		inode->i_op = &eventfs_ro_dir_inode_operations;
> +		inode->i_fop = &eventfs_ro_file_operations;
> +	} else {
> +		inode->i_op = &eventfs_dir_inode_operations;
> +		inode->i_fop = &eventfs_file_operations;
> +	}

[ ... ]

> @@ -521,14 +640,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]
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?

[ ... ]

> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index f3d6188a3b7b2..dafa0d587f04c 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -423,6 +423,14 @@ static const struct super_operations tracefs_super_operations = {
>  	.show_options	= tracefs_show_options,
>  };
>  
> +const struct super_operations eventfs_ro_super_operations = {
> +	.alloc_inode    = tracefs_alloc_inode,
> +	.free_inode     = tracefs_free_inode,
> +	.destroy_inode  = tracefs_destroy_inode,
> +	.drop_inode     = tracefs_drop_inode,
> +	.statfs		= simple_statfs,
> +};

[Severity: Medium]
As reported by Sashiko on v4 [1], because eventfs_ro_super_operations uses
tracefs_alloc_inode, all inodes allocated on the read-only eventfs
superblock (including the pinned root inode configured by
eventfs_create_events_ro_copy() and any cached subdirectories or files) are
linked into the global tracefs_inodes list.

While update_inode_attr() now returns early when ro is true during lookup,
when tracefs is mounted or remounted with uid= or gid= options,
tracefs_apply_options() still walks the global tracefs_inodes list without
checking if the inode belongs to the tracefs superblock:

fs/tracefs/inode.c:tracefs_apply_options() {
    ...
	list_for_each_entry_rcu(ti, &tracefs_inodes, list) {
		if (update_uid) {
			ti->flags &= ~TRACEFS_UID_PERM_SET;
			ti->vfs_inode.i_uid = fsi->uid;
		}

		if (update_gid) {
			ti->flags &= ~TRACEFS_GID_PERM_SET;
			ti->vfs_inode.i_gid = fsi->gid;
		}

		if (ti->flags & TRACEFS_EVENT_INODE)
			eventfs_remount(ti, update_uid, update_gid);
	}
    ...
}

Won't mounting or remounting tracefs with uid= or gid= overwrite i_uid and
i_gid on the pinned /sys/kernel/events root inode and all currently cached
eventfs_ro inodes, as well as invoke eventfs_remount() a second time on the
shared eventfs_inode via the eventfs_ro root inode?

Also, in lookup_dir_entry(), mode is initialized to:

	umode_t mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;

When ro is true, should lookup_dir_entry() clear S_IWUSR (or use 0555 to
match the root inode in eventfs_ro_fill_super()) so subdirectories in the
read-only eventfs mount do not report 0755 permissions?

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

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