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@xxxxxxxxxx>
> > > ---
> > > 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",
> > >
