Re: [PATCH] virtio-fs: Query rootmode during mount

[Miklos Szeredi]

On Thu, 7 Nov 2024 at 18:59, Hanna Czenczek <hreitz@xxxxxxxxxx> wrote:

> > Regardless, something smells here: fuse_mount_remove() is only called
> > if sb->s_root is set (both plain fuse and virtiofs).  The top level
> > fuse_mount is added to fc->mounts in fuse_conn_init(), way before
> > sb->s_root is set...
> >
> > Will look into this.

This is the deal:

1) When the top sb is created the fm->fc_entry is chained on
fc->mounts in fuse_conn_init() which is called near the top of
fuse_get_tree() and virtio_fs_get_tree(). If things fail during
->get_tree() or an existing fc is used instead of the newly allocated
one, then fuse_mount_destroy() is called, which frees the fm and
fm->fc, ignoring the mounts list.  This is okay, though a bit
confusing.

2) When a submount is created, then fm is only chained onto fc->mounts
towards the end of fuse_get_tree_submount() in the success case.

There should be no way for fuse_mount_destroy() to see fm chained onto
fc yet fc having other fuse mounts.  The below patch reflects this
with a WARN_ON().  Not tested yet, but there would be memory
corruption if it wasn't the case.

As for your patch, the condition (sb->s_root || (fm &&
fm->fc->initialized)) should work AFAICS.

Thanks,
Miklos

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fd3321e29a3e..0c4eb5b89e71 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1653,6 +1653,7 @@ static int fuse_get_tree_submount(struct fs_context *fsc)
        if (!fm)
                return -ENOMEM;

+       INIT_LIST_HEAD(&fm->fc_entry);
        fm->fc = fuse_conn_get(fc);
        fsc->s_fs_info = fm;
        sb = sget_fc(fsc, NULL, set_anon_super_fc);
@@ -1976,6 +1977,13 @@ static void fuse_sb_destroy(struct super_block *sb)

 void fuse_mount_destroy(struct fuse_mount *fm)
 {
+       /*
+        * We can get here in case of an error before the top sb is fully set
+        * up.  The sole reference to the fc must come from fm in that case
+        * otherwise may end up with corruption on fc->mounts list.
+        */
+       WARN_ON(!list_empty(&fm->fc_entry) &&
refcount_read(&fm->fc->count) != 1);
+
        fuse_conn_put(fm->fc);
        kfree_rcu(fm, rcu);
 }