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 <[email protected]> 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/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
