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