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.
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",
> >
