On Tue, 03 Nov 2026 18:51:19 +0000
[email protected] wrote:
> > + 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?
Good point. Will update.
>
> [ ... ]
>
> > @@ -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?
Hmm, I rather keep it as is. Yeah, W=1 may warn, but it's still the first
function. Maybe I'll update this later as a cleanup.
>
> [ ... ]
>
> > 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?
OK, I'll add a flag to skip read-only event inodes.
>
> 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?
Honestly, all of tracefs directories should be read only except for the
instances directory. I may update that as a separate patch.
-- Steve
