Replying to Sashiko: https://sashiko.dev/#/patchset/20260808131104.2ad18f65%40robin
> --- Patch [1]: [PATCH v2] tracefs: Add read-only eventfs filesystem at > /sys/kernel/events --- > commit 29ceffbc05b0ff3121930ed049314a6007ea355b > Author: Anubhav Shelat <[email protected]> > > tracefs: Add read-only eventfs filesystem at /sys/kernel/events > > This commit introduces a new read-only pseudo-filesystem "eventfs" mounted > at /sys/kernel/events that exposes trace event format and id files to > unprivileged users. The eventfs file system reuses the existing > eventfs_inode lazy-lookup infrastructure. > > Link: https://patch.msgid.link/[email protected] > > > diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c > > index 39c7a34531e8a..007cdf1a67f30 100644 > > --- a/fs/tracefs/event_inode.c > > +++ b/fs/tracefs/event_inode.c > > [ ... ] > > > +int eventfs_create_events_ro_copy(const char *name, struct eventfs_inode > > *ei) > > +{ > > + struct dentry *dentry; > > + struct tracefs_inode *ti; > > + struct inode *inode; > > + > > + dentry = eventfs_ro_get_root(); > > + if (IS_ERR(dentry)) > > + return PTR_ERR(dentry); > > + > > + inode = d_inode(dentry); > > + > > + INIT_LIST_HEAD(&ei->children); > > + INIT_LIST_HEAD(&ei->list); > > + > > + ti = get_tracefs(inode); > > + ti->flags |= TRACEFS_EVENT_INODE; > > + ti->private = ei; > > + > > + inode->i_op = &eventfs_ro_dir_inode_operations; > > + inode->i_fop = &eventfs_ro_file_operations; > > + > > + /* This is never freed */ > > + dentry->d_fsdata = get_ei(ei); > > + > > + return 0; > > +} > > Does this code leak the dentry reference returned by eventfs_ro_get_root()? > > Looking at eventfs_ro_get_root() in fs/tracefs/inode.c, it returns the dentry > using dget(eventfs_ro_mount->mnt_root). However, > eventfs_create_events_ro_copy() > never calls dput(dentry) to balance the reference count, nor does it save the > dentry pointer to a global variable for later cleanup. It's not really a leak. Yes it is not freed, but once allocated, it is never freed. That's fine because it lasts for the remander of the boot and has only a single instance. The comment above assigning d_fsdata states this. I could add another comment bave the eventfs_ro_get_root(). (I wonder if Sashiko will understand it?) > > > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c > > index f3d6188a3b7b2..fd064d79d940e 100644 > > --- a/fs/tracefs/inode.c > > +++ b/fs/tracefs/inode.c > > [ ... ] > > > +void eventfs_ro_put_root(void) > > +{ > > + simple_release_fs(&eventfs_ro_mount, &eventfs_ro_mount_count); > > +} > > Is eventfs_ro_put_root() ever called? > > This teardown function appears to be explicitly introduced to balance the > internal mount pin created by simple_pin_fs() in eventfs_ro_get_root(), but > it > seems to remain completely unreferenced in the codebase. Hmm, right. This was leftover from Anubhav's first patch that could fail after the eventfs_ro_get_root(). This version does not fail so it doesn't need this function. I'll remove it. -- Steve
