Re: [PATCH v2] tracefs: Add read-only eventfs filesystem at /sys/kernel/events

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