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