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

[Brian Foster <bfoster@redhat.com>]

On Thu, Sep 26, 2019 at 11:27:40AM +0800, Ian Kent wrote:
> On Wed, 2019-09-25 at 10:34 -0400, Brian Foster wrote:
> > On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> > > 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 ?
> > > 
> > 
> > It looks like perhaps the assignment of sb->s_fs_info was lost as
> > well?
> > Skipping to the end, I see xfs_init_fs_context() alloc mp and assign
> > fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which
> > somehow
> > gets mp from sb->s_fs_info (not fc->...), but then resets sb-
> > >s_fs_info
> > on error and frees the names, leaving fs->s_fs_info so presumably
> > xfs_fc_free() can free mp along with a couple of the names (again). I
> > can't really make heads or tails of what this is even attempting to
> > do.
> 
> Ha, it seems a bit mysterious, but it's actually much simpler
> than it appears.
> 

Feel free to explain any of the above..? Where do you currently assign
sb->s_fs_info, for example?

> > 
> > That aside, it's not clear to me why the new code can't follow a
> > similar
> > pattern as the old code with regard to allocation. Allocate mp in
> > xfs_fill_super() and set up sb/fc pointers, reset pointers and free
> > mp
> > on error return. Otherwise, xfs_fc_free() checks for fc->s_fs_info !=
> > NULL and frees mp from there. Is there some reason we can't continue
> > to
> > do that?
> 
> I think not without a fairly significant re-design.
> 
> The main difference is the mount-api will allocate the super
> block later than the old mount code.
> 
> Basically, if file system parameter parsing fails the super
> block won't get allocated.
> 
> So the super block isn't available during parameter parsing
> but the file system private data structure may be needed for
> it, so it comes from the file system context at that point.
> 
> When the super block is successfully allocated the file system
> private data structure is set in the super block (and the field
> NULLed in the context) and things progress much the same as
> before from that point.
> 
> That's the essential difference in the process AFAICS.
> 

I see. This is probably something that should be noted in the commit
log (that the ordering changes from before such that we need to allocate
mp a bit earlier). This is reasonable because even though the current
code allocs mp in the fill_super callback, we parse arguments
immediately after the mp allocation and don't otherwise rely on the sb
in that code.

If I follow correctly, it sounds like perhaps we need to separate the
management of sb->s_fs_info from the "ownership" of mp. For example,
allocate mp, assign fc->s_fs_info and free via xfs_fc_free() as you do
now. In the xfs_fill_super() callback, pull mp from fc->s_fs_info and
assign it to sb->s_fs_info. If we fail at this point, reset
sb->s_fs_info to NULL and let the fc infrastructure deal with freeing mp
in its own callback.

What I'm not clear on is whether something like xfs_fs_put_super()
should still free mp as well. Once the filesystem successfully mounts,
are we still going to see an xfs_fc_free() callback, or is this all just
transient mount path stuff? If the former, perhaps put_super() should
also not free mp and just reset its own ->s_fs_info reference. If the
latter, then I guess we just need to understand at what point during a
successful mount responsibility to free transfers from one place to the
other. Thoughts?

Brian

> By the time fill_super() is called everything is set and you
> should be able to proceed almost the same as before.
> 
> Ian
> 
> > Brian
> > 
> > > > 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",
> > > > > > 
>