Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent <raven@themaw.net>]

On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > Add the fs_context_operations method .get_tree that validates
> > > mount options and fills the super block as previously done
> > > by the file_system_type .mount method.
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > ---
> > >  fs/xfs/xfs_super.c |   50
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > >  	return error;
> > >  }
> > >  
> > > +STATIC int
> > > +xfs_fill_super(
> > > +	struct super_block	*sb,
> > > +	struct fs_context	*fc)
> > > +{
> > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > +	int			silent = fc->sb_flags & SB_SILENT;
> > > +	int			error = -ENOMEM;
> > > +
> > > +	mp->m_super = sb;
> > > +
> > > +	/*
> > > +	 * set up the mount name first so all the errors will refer to
> > > the
> > > +	 * correct device.
> > > +	 */
> > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > > +	if (!mp->m_fsname)
> > > +		return -ENOMEM;
> > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > +
> > > +	error = xfs_validate_params(mp, ctx, false);
> > > +	if (error)
> > > +		goto out_free_fsname;
> > > +
> > > +	error = __xfs_fs_fill_super(mp, silent);
> > > +	if (error)
> > > +		goto out_free_fsname;
> > > +
> > > +	return 0;
> > > +
> > > + out_free_fsname:
> > > +	sb->s_fs_info = NULL;
> > > +	xfs_free_fsname(mp);
> > > +
> > 
> > I'm still not following the (intended) lifecycle of mp here.
> > Looking
> > ahead in the series, we allocate mp in xfs_init_fs_context() and
> > set
> > some state. It looks like at some point we grow an xfs_fc_free()
> > callback that frees mp, but that doesn't exist as of yet. So is
> > that
> > a
> > memory leak as of this patch?
> > 
> > We also call xfs_free_fsname() here (which doesn't reset pointers
> > to
> > NULL) and open-code kfree()'s of a couple of the same fields in
> > xfs_fc_free(). Those look like double frees to me.
> > 
> > Hmm.. I guess I'm kind of wondering why we lift the mp alloc out of
> > the
> > fill super call in the first place. At a glance, it doesn't look
> > like
> > we
> > do anything in that xfs_init_fs_context() call that we couldn't do
> > a
> > bit
> > later..
> 
> Umm ... yes ...
> 
> I think I've got the active code path right ...
> 
> At this point .mount == xfs_fs_mount() which will calls
> xfs_fs_fill_super() to fill the super block.
> 
> xfs_fs_fill_super() allocates the super block info struct and sets
> it in the super block private info field, then calls xfs_parseargs()
> which still allocates mp->m_fsname at this point, to accomodate a
> similar free pattern in xfs_test_remount_options().
> 
> It then calls __xfs_fs_fill_super() which doesn't touch those fsname
> fields or mp to fit in with what will be done later.
> 
> If an error occurs both the fsname fields (xfs_free_fsname()) and mp
> are freed by the main caller, xfs_fs_fill_super().
> 
> I think that process is ok.
> 
> The mount api process that isn't active yet is a bit different.
> 
> The context (ctx), a temporary working space, is allocated then saved
> in the mount context (fc) and the super block info is also allocated
> and saved in the mount context in it's field of the same name as the
> private super block info field, s_fs_info.
> 
> The function xfs_fill_super() is called as a result of the
> .get_tree()
> mount context operation to fill the super block.
> 
> During this process, when the VFS successfully allocates the super
> block s_fs_info is set in the super block and the mount context
> field set to NULL. From this point freeing the private super block
> info becomes part of usual freeing of the super block with the super
> operation .kill_sb().
> 
> But if the super block allocation fails then the mount context
> s_fs_info field remains set and is the responsibility of the
> mount context operations .fc_free() method to clean up.
> 
> Now the VFS calls to xfs_fill_super() after this.
> 
> I should have been able to leave xfs_fill_super() it as it
> was with:
>         sb->s_fs_info = NULL;
>         xfs_free_fsname(mp);
>         kfree(mp);
> and that should have been ok but it wasn't, there was some sort of
> allocation problem, possibly a double free, causing a crash.
> 
> Strictly speaking this cleanup process should be carried out by
> either the mount context .fc_free() or super operation .kill_sb()
> and that's what I want to do.

Umm ... but I can't actually do that ...

Looking back at xfs I realize that the filling of the super
block is meant to leave nothing allocated and set
sb->s_fs_info = NULL on error so that ->put_super() won't try
and cleanup a whole bunch of stuff that hasn't been done.

Which brings me back to what I originally had above ... which
we believe doesn't work ?

> 
> So I'm not sure the allocation time and the place this is done
> can (or should) be done differently.
> 
> And that freeing on error exit from xfs_fill_super() is definitely
> wrong now! Ha, and I didn't see any crashes myself when I tested
> it ... maybe I need a reproducer ...
> 
> Ian
> 
> > Brian
> > 
> > > +	return error;
> > > +}
> > > +
> > > +STATIC int
> > > +xfs_get_tree(
> > > +	struct fs_context	*fc)
> > > +{
> > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > +}
> > > +
> > >  STATIC void
> > >  xfs_fs_put_super(
> > >  	struct super_block	*sb)
> > > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > > xfs_super_operations = {
> > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > >  };
> > >  
> > > +static const struct fs_context_operations xfs_context_ops = {
> > > +	.parse_param = xfs_parse_param,
> > > +	.get_tree    = xfs_get_tree,
> > > +};
> > > +
> > >  static struct file_system_type xfs_fs_type = {
> > >  	.owner			= THIS_MODULE,
> > >  	.name			= "xfs",
> > >