Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 8 Sep 2023 16:45:53 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hi, 
> 
> On Wed, 06 Sep 2023 22:47:11 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Currently when rmdir on an instance is done, eventfs_remove_events_dir()
> > is called and it does a dput on the dentry and then frees the
> > eventfs_inode that represents the events directory.
> > 
> > But there's no protection against a reader reading the top level events
> > directory at the same time and we can get a use after free error. Instead,
> > use the dput() associated to the dentry to also free the eventfs_inode
> > associated to the events directory, as that will get called when the last
> > reference to the directory is released.
> > 
> > Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/
> > 
> > Cc: Ajay Kaher <akaher@vmware.com>
> > Fixes: 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")
> > Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> > Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230905183332.628d7cc0@gandalf.local.home
> >  - Removed left over "ei" variable (kernel test robot)
> > 
> >  fs/tracefs/event_inode.c | 17 ++++++++++++-----
> >  fs/tracefs/inode.c       |  2 +-
> >  fs/tracefs/internal.h    |  5 +++--
> >  3 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> > index fa1a1679a886..609ccb5b7cfc 100644
> > --- a/fs/tracefs/event_inode.c
> > +++ b/fs/tracefs/event_inode.c
> > @@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
> >  
> >  /**
> >   * eventfs_set_ef_status_free - set the ef->status to free
> > + * @ti: the tracefs_inode of the dentry
> >   * @dentry: dentry who's status to be freed
> >   *
> >   * eventfs_set_ef_status_free will be called if no more
> >   * references remain
> >   */
> > -void eventfs_set_ef_status_free(struct dentry *dentry)
> > +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> >  {
> >  	struct tracefs_inode *ti_parent;
> > +	struct eventfs_inode *ei;
> >  	struct eventfs_file *ef;
> >  
> >  	mutex_lock(&eventfs_mutex);
> > +
> > +	/* The top level events directory may be freed by this */
> > +	if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
> > +		ei = ti->private;
> > +		kfree(ei);  
> 
> Don't we need to clear 'ti->private' here to avoid accessing
> (or double free) ti->private somewhare?

I don't think it's needed but I did add it to

  https://lore.kernel.org/linux-trace-kernel/20230907175859.6fedbaa2@gandalf.local.home/

Which you reviewed.

> 
> > +		goto out;
> > +	}
> > +
> >  	ti_parent = get_tracefs(dentry->d_parent->d_inode);
> >  	if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
> >  		goto out;  
> 
> Here, I guess this "!(ti_parent->flags & TRACEFS_EVENT_INODE)" means this
> inode is TRACEFS_EVENT_TOP_INODE, so this check may not be needed,
> is this correct?

The check isn't needed but I like to keep it because it will break things
badly if it is every called on something that is not an EVENT_INODE.

We could add a WARN() here if not, but this code is not critical if it is
called without it set.

-- Steve