[PATCH v2 0/7] fuse: Some fixes for submounts

[PATCH v2 0/7] fuse: Some fixes for submounts

[Greg Kurz]

v2:

- add an extra fix (patch 2) : mount is now added to the list before
  unlocking sb->s_umount
- set SB_BORN just before unlocking sb->s_umount, just like it would
  happen when using fc_mount() (Max)
- don't allocate a FUSE context for the submounts (Max)
- introduce a dedicated context ops for submounts
- add a extra cleanup : simplify the code even more with fc_mount()

v1:

While working on adding syncfs() support in FUSE, I've hit some severe
bugs with submounts (a crash and an infinite loop). The fix for the
crash is straightforward (patch 1), but the fix for the infinite loop
is more invasive : as suggested by Miklos, a simple bug fix is applied
first (patch 2) and the final fix (patch 3) is applied on top.

Greg Kurz (7):
  fuse: Fix crash in fuse_dentry_automount() error path
  fuse: Fix crash if superblock of submount gets killed early
  fuse: Fix infinite loop in sget_fc()
  fuse: Add dedicated filesystem context ops for submounts
  fuse: Call vfs_get_tree() for submounts
  fuse: Switch to fc_mount() for submounts
  fuse: Make fuse_fill_super_submount() static

 fs/fuse/dir.c       | 58 ++++++---------------------------------------
 fs/fuse/fuse_i.h    | 14 ++++-------
 fs/fuse/inode.c     | 56 +++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 69 insertions(+), 62 deletions(-)

-- 
2.31.1

[Virtio-fs] [PATCH v2 0/7] fuse: Some fixes for submounts

[Greg Kurz]

v2:

- add an extra fix (patch 2) : mount is now added to the list before
  unlocking sb->s_umount
- set SB_BORN just before unlocking sb->s_umount, just like it would
  happen when using fc_mount() (Max)
- don't allocate a FUSE context for the submounts (Max)
- introduce a dedicated context ops for submounts
- add a extra cleanup : simplify the code even more with fc_mount()

v1:

While working on adding syncfs() support in FUSE, I've hit some severe
bugs with submounts (a crash and an infinite loop). The fix for the
crash is straightforward (patch 1), but the fix for the infinite loop
is more invasive : as suggested by Miklos, a simple bug fix is applied
first (patch 2) and the final fix (patch 3) is applied on top.

Greg Kurz (7):
  fuse: Fix crash in fuse_dentry_automount() error path
  fuse: Fix crash if superblock of submount gets killed early
  fuse: Fix infinite loop in sget_fc()
  fuse: Add dedicated filesystem context ops for submounts
  fuse: Call vfs_get_tree() for submounts
  fuse: Switch to fc_mount() for submounts
  fuse: Make fuse_fill_super_submount() static

 fs/fuse/dir.c       | 58 ++++++---------------------------------------
 fs/fuse/fuse_i.h    | 14 ++++-------
 fs/fuse/inode.c     | 56 +++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 69 insertions(+), 62 deletions(-)

-- 
2.31.1

[PATCH v2 1/7] fuse: Fix crash in fuse_dentry_automount() error path

[Greg Kurz]

If fuse_fill_super_submount() returns an error, the error path
triggers a crash:

[   26.206673] BUG: kernel NULL pointer dereference, address: 0000000000000000
[...]
[   26.226362] RIP: 0010:__list_del_entry_valid+0x25/0x90
[...]
[   26.247938] Call Trace:
[   26.248300]  fuse_mount_remove+0x2c/0x70 [fuse]
[   26.248892]  virtio_kill_sb+0x22/0x160 [virtiofs]
[   26.249487]  deactivate_locked_super+0x36/0xa0
[   26.250077]  fuse_dentry_automount+0x178/0x1a0 [fuse]

The crash happens because fuse_mount_remove() assumes that the FUSE
mount was already added to list under the FUSE connection, but this
only done after fuse_fill_super_submount() has returned success.

This means that until fuse_fill_super_submount() has returned success,
the FUSE mount isn't actually owned by the superblock. We should thus
reclaim ownership by clearing sb->s_fs_info, which will skip the call
to fuse_mount_remove(), and perform rollback, like virtio_fs_get_tree()
already does for the root sb.

Fixes: bf109c64040f ("fuse: implement crossmounts")
Cc: mreitz@redhat.com
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 fs/fuse/dir.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 1b6c001a7dd1..01559061cbfb 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -339,8 +339,12 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 
 	/* Initialize superblock, making @mp_fi its root */
 	err = fuse_fill_super_submount(sb, mp_fi);
-	if (err)
+	if (err) {
+		fuse_conn_put(fc);
+		kfree(fm);
+		sb->s_fs_info = NULL;
 		goto out_put_sb;
+	}
 
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
-- 
2.31.1

[Virtio-fs] [PATCH v2 1/7] fuse: Fix crash in fuse_dentry_automount() error path

[Greg Kurz]

If fuse_fill_super_submount() returns an error, the error path
triggers a crash:

[   26.206673] BUG: kernel NULL pointer dereference, address: 0000000000000000
[...]
[   26.226362] RIP: 0010:__list_del_entry_valid+0x25/0x90
[...]
[   26.247938] Call Trace:
[   26.248300]  fuse_mount_remove+0x2c/0x70 [fuse]
[   26.248892]  virtio_kill_sb+0x22/0x160 [virtiofs]
[   26.249487]  deactivate_locked_super+0x36/0xa0
[   26.250077]  fuse_dentry_automount+0x178/0x1a0 [fuse]

The crash happens because fuse_mount_remove() assumes that the FUSE
mount was already added to list under the FUSE connection, but this
only done after fuse_fill_super_submount() has returned success.

This means that until fuse_fill_super_submount() has returned success,
the FUSE mount isn't actually owned by the superblock. We should thus
reclaim ownership by clearing sb->s_fs_info, which will skip the call
to fuse_mount_remove(), and perform rollback, like virtio_fs_get_tree()
already does for the root sb.

Fixes: bf109c64040f ("fuse: implement crossmounts")
Cc: mreitz@redhat.com
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 fs/fuse/dir.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 1b6c001a7dd1..01559061cbfb 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -339,8 +339,12 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 
 	/* Initialize superblock, making @mp_fi its root */
 	err = fuse_fill_super_submount(sb, mp_fi);
-	if (err)
+	if (err) {
+		fuse_conn_put(fc);
+		kfree(fm);
+		sb->s_fs_info = NULL;
 		goto out_put_sb;
+	}
 
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
-- 
2.31.1

[PATCH v2 2/7] fuse: Fix crash if superblock of submount gets killed early

[Greg Kurz]

As soon as fuse_dentry_automount() does up_write(&sb->s_umount), the
superblock can theoretically be killed. If this happens before the
submount was added to the &fc->mounts list, fuse_mount_remove() later
crashes in list_del_init() because it assumes the submount to be
already there.

Add the submount before dropping sb->s_umount to fix the inconsistency.
It is okay to nest fc->killsb under sb->s_umount, we already do this
on the ->kill_sb() path.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 01559061cbfb..3fd1b71e546b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -346,15 +346,15 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out_put_sb;
 	}
 
+	down_write(&fc->killsb);
+	list_add_tail(&fm->fc_entry, &fc->mounts);
+	up_write(&fc->killsb);
+
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
 	/* We are done configuring the superblock, so unlock it */
 	up_write(&sb->s_umount);
 
-	down_write(&fc->killsb);
-	list_add_tail(&fm->fc_entry, &fc->mounts);
-	up_write(&fc->killsb);
-
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
 	if (IS_ERR(mnt)) {
-- 
2.31.1

[Virtio-fs] [PATCH v2 2/7] fuse: Fix crash if superblock of submount gets killed early

[Greg Kurz]

As soon as fuse_dentry_automount() does up_write(&sb->s_umount), the
superblock can theoretically be killed. If this happens before the
submount was added to the &fc->mounts list, fuse_mount_remove() later
crashes in list_del_init() because it assumes the submount to be
already there.

Add the submount before dropping sb->s_umount to fix the inconsistency.
It is okay to nest fc->killsb under sb->s_umount, we already do this
on the ->kill_sb() path.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 01559061cbfb..3fd1b71e546b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -346,15 +346,15 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out_put_sb;
 	}
 
+	down_write(&fc->killsb);
+	list_add_tail(&fm->fc_entry, &fc->mounts);
+	up_write(&fc->killsb);
+
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
 	/* We are done configuring the superblock, so unlock it */
 	up_write(&sb->s_umount);
 
-	down_write(&fc->killsb);
-	list_add_tail(&fm->fc_entry, &fc->mounts);
-	up_write(&fc->killsb);
-
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
 	if (IS_ERR(mnt)) {
-- 
2.31.1

Re: [PATCH v2 2/7] fuse: Fix crash if superblock of submount gets killed early

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> As soon as fuse_dentry_automount() does up_write(&sb->s_umount), the
> superblock can theoretically be killed. If this happens before the
> submount was added to the &fc->mounts list, fuse_mount_remove() later
> crashes in list_del_init() because it assumes the submount to be
> already there.
>
> Add the submount before dropping sb->s_umount to fix the inconsistency.
> It is okay to nest fc->killsb under sb->s_umount, we already do this
> on the ->kill_sb() path.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Virtio-fs] [PATCH v2 2/7] fuse: Fix crash if superblock of submount gets killed early

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> As soon as fuse_dentry_automount() does up_write(&sb->s_umount), the
> superblock can theoretically be killed. If this happens before the
> submount was added to the &fc->mounts list, fuse_mount_remove() later
> crashes in list_del_init() because it assumes the submount to be
> already there.
>
> Add the submount before dropping sb->s_umount to fix the inconsistency.
> It is okay to nest fc->killsb under sb->s_umount, we already do this
> on the ->kill_sb() path.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 3/7] fuse: Fix infinite loop in sget_fc()

[Greg Kurz]

We don't set the SB_BORN flag on submounts. This is wrong as these
superblocks are then considered as partially constructed or dying
in the rest of the code and can break some assumptions.

One such case is when you have a virtiofs filesystem with submounts
and you try to mount it again : virtio_fs_get_tree() tries to obtain
a superblock with sget_fc(). The logic in sget_fc() is to loop until
it has either found an existing matching superblock with SB_BORN set
or to create a brand new one. It is assumed that a superblock without
SB_BORN is transient and the loop is restarted. Forgetting to set
SB_BORN on submounts hence causes sget_fc() to retry forever.

Setting SB_BORN requires special care, i.e. a write barrier for
super_cache_count() which can check SB_BORN without taking any lock.
We should call vfs_get_tree() to deal with that but this requires
to have a proper ->get_tree() implementation for submounts, which
is a bigger piece of work. Go for a simple bug fix in the meatime.

Fixes: bf109c64040f ("fuse: implement crossmounts")
Cc: mreitz@redhat.com
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3fd1b71e546b..3fa8604c21d5 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -352,6 +352,17 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
+
+	/*
+	 * FIXME: setting SB_BORN requires a write barrier for
+	 *        super_cache_count(). We should actually come
+	 *        up with a proper ->get_tree() implementation
+	 *        for submounts and call vfs_get_tree() to take
+	 *        care of the write barrier.
+	 */
+	smp_wmb();
+	sb->s_flags |= SB_BORN;
+
 	/* We are done configuring the superblock, so unlock it */
 	up_write(&sb->s_umount);
 
-- 
2.31.1

[Virtio-fs] [PATCH v2 3/7] fuse: Fix infinite loop in sget_fc()

[Greg Kurz]

We don't set the SB_BORN flag on submounts. This is wrong as these
superblocks are then considered as partially constructed or dying
in the rest of the code and can break some assumptions.

One such case is when you have a virtiofs filesystem with submounts
and you try to mount it again : virtio_fs_get_tree() tries to obtain
a superblock with sget_fc(). The logic in sget_fc() is to loop until
it has either found an existing matching superblock with SB_BORN set
or to create a brand new one. It is assumed that a superblock without
SB_BORN is transient and the loop is restarted. Forgetting to set
SB_BORN on submounts hence causes sget_fc() to retry forever.

Setting SB_BORN requires special care, i.e. a write barrier for
super_cache_count() which can check SB_BORN without taking any lock.
We should call vfs_get_tree() to deal with that but this requires
to have a proper ->get_tree() implementation for submounts, which
is a bigger piece of work. Go for a simple bug fix in the meatime.

Fixes: bf109c64040f ("fuse: implement crossmounts")
Cc: mreitz@redhat.com
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3fd1b71e546b..3fa8604c21d5 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -352,6 +352,17 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
+
+	/*
+	 * FIXME: setting SB_BORN requires a write barrier for
+	 *        super_cache_count(). We should actually come
+	 *        up with a proper ->get_tree() implementation
+	 *        for submounts and call vfs_get_tree() to take
+	 *        care of the write barrier.
+	 */
+	smp_wmb();
+	sb->s_flags |= SB_BORN;
+
 	/* We are done configuring the superblock, so unlock it */
 	up_write(&sb->s_umount);
 
-- 
2.31.1

Re: [PATCH v2 3/7] fuse: Fix infinite loop in sget_fc()

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> We don't set the SB_BORN flag on submounts. This is wrong as these
> superblocks are then considered as partially constructed or dying
> in the rest of the code and can break some assumptions.
>
> One such case is when you have a virtiofs filesystem with submounts
> and you try to mount it again : virtio_fs_get_tree() tries to obtain
> a superblock with sget_fc(). The logic in sget_fc() is to loop until
> it has either found an existing matching superblock with SB_BORN set
> or to create a brand new one. It is assumed that a superblock without
> SB_BORN is transient and the loop is restarted. Forgetting to set
> SB_BORN on submounts hence causes sget_fc() to retry forever.
>
> Setting SB_BORN requires special care, i.e. a write barrier for
> super_cache_count() which can check SB_BORN without taking any lock.
> We should call vfs_get_tree() to deal with that but this requires
> to have a proper ->get_tree() implementation for submounts, which
> is a bigger piece of work. Go for a simple bug fix in the meatime.
>
> Fixes: bf109c64040f ("fuse: implement crossmounts")
> Cc: mreitz@redhat.com
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Virtio-fs] [PATCH v2 3/7] fuse: Fix infinite loop in sget_fc()

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> We don't set the SB_BORN flag on submounts. This is wrong as these
> superblocks are then considered as partially constructed or dying
> in the rest of the code and can break some assumptions.
>
> One such case is when you have a virtiofs filesystem with submounts
> and you try to mount it again : virtio_fs_get_tree() tries to obtain
> a superblock with sget_fc(). The logic in sget_fc() is to loop until
> it has either found an existing matching superblock with SB_BORN set
> or to create a brand new one. It is assumed that a superblock without
> SB_BORN is transient and the loop is restarted. Forgetting to set
> SB_BORN on submounts hence causes sget_fc() to retry forever.
>
> Setting SB_BORN requires special care, i.e. a write barrier for
> super_cache_count() which can check SB_BORN without taking any lock.
> We should call vfs_get_tree() to deal with that but this requires
> to have a proper ->get_tree() implementation for submounts, which
> is a bigger piece of work. Go for a simple bug fix in the meatime.
>
> Fixes: bf109c64040f ("fuse: implement crossmounts")
> Cc: mreitz@redhat.com
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 4/7] fuse: Add dedicated filesystem context ops for submounts

[Greg Kurz]

The creation of a submount is open-coded in fuse_dentry_automount().
This brings a lot of complexity and we recently had to fix bugs
because we weren't setting SB_BORN or because we were unlocking
sb->s_umount before sb was fully configured. Most of these could
have been avoided by using the mount API instead of open-coding.

Basically, this means coming up with a proper ->get_tree()
implementation for submounts and call vfs_get_tree(), or better
fc_mount().

The creation of the superblock for submounts is quite different from
the root mount. Especially, it doesn't require to allocate a FUSE
filesystem context, nor to parse parameters.

Introduce a dedicated context ops for submounts to make this clear.
This is just a placeholder for now, fuse_get_tree_submount() will
be populated in a subsequent patch.

Only visible change is that we stop allocating/freeing a useless FUSE
filesystem context with submounts.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/fuse_i.h    |  5 +++++
 fs/fuse/inode.c     | 16 ++++++++++++++++
 fs/fuse/virtio_fs.c |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 7e463e220053..862ad317bc89 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1097,6 +1097,11 @@ int fuse_fill_super_submount(struct super_block *sb,
  */
 bool fuse_mount_remove(struct fuse_mount *fm);
 
+/*
+ * Setup context ops for submounts
+ */
+int fuse_init_fs_context_submount(struct fs_context *fsc);
+
 /*
  * Shut down the connection (possibly sending DESTROY request).
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 393e36b74dc4..fa96a3762ea2 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,6 +1313,22 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+static int fuse_get_tree_submount(struct fs_context *fsc)
+{
+	return 0;
+}
+
+static const struct fs_context_operations fuse_context_submount_ops = {
+	.get_tree	= fuse_get_tree_submount,
+};
+
+int fuse_init_fs_context_submount(struct fs_context *fsc)
+{
+	fsc->ops = &fuse_context_submount_ops;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
+
 int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 {
 	struct fuse_dev *fud = NULL;
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bcb8a02e2d8b..e68467cc765c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1496,6 +1496,9 @@ static int virtio_fs_init_fs_context(struct fs_context *fsc)
 {
 	struct fuse_fs_context *ctx;
 
+	if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
+		return fuse_init_fs_context_submount(fsc);
+
 	ctx = kzalloc(sizeof(struct fuse_fs_context), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
-- 
2.31.1

[Virtio-fs] [PATCH v2 4/7] fuse: Add dedicated filesystem context ops for submounts

[Greg Kurz]

The creation of a submount is open-coded in fuse_dentry_automount().
This brings a lot of complexity and we recently had to fix bugs
because we weren't setting SB_BORN or because we were unlocking
sb->s_umount before sb was fully configured. Most of these could
have been avoided by using the mount API instead of open-coding.

Basically, this means coming up with a proper ->get_tree()
implementation for submounts and call vfs_get_tree(), or better
fc_mount().

The creation of the superblock for submounts is quite different from
the root mount. Especially, it doesn't require to allocate a FUSE
filesystem context, nor to parse parameters.

Introduce a dedicated context ops for submounts to make this clear.
This is just a placeholder for now, fuse_get_tree_submount() will
be populated in a subsequent patch.

Only visible change is that we stop allocating/freeing a useless FUSE
filesystem context with submounts.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/fuse_i.h    |  5 +++++
 fs/fuse/inode.c     | 16 ++++++++++++++++
 fs/fuse/virtio_fs.c |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 7e463e220053..862ad317bc89 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1097,6 +1097,11 @@ int fuse_fill_super_submount(struct super_block *sb,
  */
 bool fuse_mount_remove(struct fuse_mount *fm);
 
+/*
+ * Setup context ops for submounts
+ */
+int fuse_init_fs_context_submount(struct fs_context *fsc);
+
 /*
  * Shut down the connection (possibly sending DESTROY request).
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 393e36b74dc4..fa96a3762ea2 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,6 +1313,22 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+static int fuse_get_tree_submount(struct fs_context *fsc)
+{
+	return 0;
+}
+
+static const struct fs_context_operations fuse_context_submount_ops = {
+	.get_tree	= fuse_get_tree_submount,
+};
+
+int fuse_init_fs_context_submount(struct fs_context *fsc)
+{
+	fsc->ops = &fuse_context_submount_ops;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
+
 int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 {
 	struct fuse_dev *fud = NULL;
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bcb8a02e2d8b..e68467cc765c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1496,6 +1496,9 @@ static int virtio_fs_init_fs_context(struct fs_context *fsc)
 {
 	struct fuse_fs_context *ctx;
 
+	if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
+		return fuse_init_fs_context_submount(fsc);
+
 	ctx = kzalloc(sizeof(struct fuse_fs_context), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
-- 
2.31.1

Re: [PATCH v2 4/7] fuse: Add dedicated filesystem context ops for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> The creation of a submount is open-coded in fuse_dentry_automount().
> This brings a lot of complexity and we recently had to fix bugs
> because we weren't setting SB_BORN or because we were unlocking
> sb->s_umount before sb was fully configured. Most of these could
> have been avoided by using the mount API instead of open-coding.
>
> Basically, this means coming up with a proper ->get_tree()
> implementation for submounts and call vfs_get_tree(), or better
> fc_mount().
>
> The creation of the superblock for submounts is quite different from
> the root mount. Especially, it doesn't require to allocate a FUSE
> filesystem context, nor to parse parameters.
>
> Introduce a dedicated context ops for submounts to make this clear.
> This is just a placeholder for now, fuse_get_tree_submount() will
> be populated in a subsequent patch.
>
> Only visible change is that we stop allocating/freeing a useless FUSE
> filesystem context with submounts.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/fuse_i.h    |  5 +++++
>   fs/fuse/inode.c     | 16 ++++++++++++++++
>   fs/fuse/virtio_fs.c |  3 +++
>   3 files changed, 24 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Virtio-fs] [PATCH v2 4/7] fuse: Add dedicated filesystem context ops for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> The creation of a submount is open-coded in fuse_dentry_automount().
> This brings a lot of complexity and we recently had to fix bugs
> because we weren't setting SB_BORN or because we were unlocking
> sb->s_umount before sb was fully configured. Most of these could
> have been avoided by using the mount API instead of open-coding.
>
> Basically, this means coming up with a proper ->get_tree()
> implementation for submounts and call vfs_get_tree(), or better
> fc_mount().
>
> The creation of the superblock for submounts is quite different from
> the root mount. Especially, it doesn't require to allocate a FUSE
> filesystem context, nor to parse parameters.
>
> Introduce a dedicated context ops for submounts to make this clear.
> This is just a placeholder for now, fuse_get_tree_submount() will
> be populated in a subsequent patch.
>
> Only visible change is that we stop allocating/freeing a useless FUSE
> filesystem context with submounts.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/fuse_i.h    |  5 +++++
>   fs/fuse/inode.c     | 16 ++++++++++++++++
>   fs/fuse/virtio_fs.c |  3 +++
>   3 files changed, 24 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 5/7] fuse: Call vfs_get_tree() for submounts

[Greg Kurz]

We recently fixed an infinite loop by setting the SB_BORN flag on
submounts along with the write barrier needed by super_cache_count().
This is the job of vfs_get_tree() and FUSE shouldn't have to care
about the barrier at all.

Split out some code from fuse_dentry_automount() to the dedicated
fuse_get_tree_submount() handler for submounts and call vfs_get_tree().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c   | 53 +++++--------------------------------------------
 fs/fuse/inode.c | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3fa8604c21d5..b88e5785a3dd 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -309,12 +309,8 @@ static int fuse_dentry_delete(const struct dentry *dentry)
 static struct vfsmount *fuse_dentry_automount(struct path *path)
 {
 	struct fs_context *fsc;
-	struct fuse_mount *parent_fm = get_fuse_mount_super(path->mnt->mnt_sb);
-	struct fuse_conn *fc = parent_fm->fc;
-	struct fuse_mount *fm;
 	struct vfsmount *mnt;
 	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
-	struct super_block *sb;
 	int err;
 
 	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
@@ -323,48 +319,15 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out;
 	}
 
-	err = -ENOMEM;
-	fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
-	if (!fm)
-		goto out_put_fsc;
+	/* Pass the FUSE inode of the mount for fuse_get_tree_submount() */
+	fsc->fs_private = mp_fi;
 
-	fsc->s_fs_info = fm;
-	sb = sget_fc(fsc, NULL, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		err = PTR_ERR(sb);
-		kfree(fm);
+	err = vfs_get_tree(fsc);
+	if (err)
 		goto out_put_fsc;
-	}
-	fm->fc = fuse_conn_get(fc);
-
-	/* Initialize superblock, making @mp_fi its root */
-	err = fuse_fill_super_submount(sb, mp_fi);
-	if (err) {
-		fuse_conn_put(fc);
-		kfree(fm);
-		sb->s_fs_info = NULL;
-		goto out_put_sb;
-	}
-
-	down_write(&fc->killsb);
-	list_add_tail(&fm->fc_entry, &fc->mounts);
-	up_write(&fc->killsb);
-
-	sb->s_flags |= SB_ACTIVE;
-	fsc->root = dget(sb->s_root);
-
-	/*
-	 * FIXME: setting SB_BORN requires a write barrier for
-	 *        super_cache_count(). We should actually come
-	 *        up with a proper ->get_tree() implementation
-	 *        for submounts and call vfs_get_tree() to take
-	 *        care of the write barrier.
-	 */
-	smp_wmb();
-	sb->s_flags |= SB_BORN;
 
 	/* We are done configuring the superblock, so unlock it */
-	up_write(&sb->s_umount);
+	up_write(&fsc->root->d_sb->s_umount);
 
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
@@ -376,12 +339,6 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	put_fs_context(fsc);
 	return mnt;
 
-out_put_sb:
-	/*
-	 * Only jump here when fsc->root is NULL and sb is still locked
-	 * (otherwise put_fs_context() will put the superblock)
-	 */
-	deactivate_locked_super(sb);
 out_put_fsc:
 	put_fs_context(fsc);
 out:
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fa96a3762ea2..bec1676811d4 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,8 +1313,44 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+/* Filesystem context private data holds the FUSE inode of the mount point */
 static int fuse_get_tree_submount(struct fs_context *fsc)
 {
+	struct fuse_mount *fm;
+	struct fuse_inode *mp_fi = fsc->fs_private;
+	struct fuse_conn *fc = get_fuse_conn(&mp_fi->inode);
+	struct super_block *sb;
+	int err;
+
+	fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
+	if (!fm)
+		return -ENOMEM;
+
+	fsc->s_fs_info = fm;
+	sb = sget_fc(fsc, NULL, set_anon_super_fc);
+	if (IS_ERR(sb)) {
+		kfree(fm);
+		return PTR_ERR(sb);
+	}
+	fm->fc = fuse_conn_get(fc);
+
+	/* Initialize superblock, making @mp_fi its root */
+	err = fuse_fill_super_submount(sb, mp_fi);
+	if (err) {
+		fuse_conn_put(fc);
+		kfree(fm);
+		sb->s_fs_info = NULL;
+		deactivate_locked_super(sb);
+		return err;
+	}
+
+	down_write(&fc->killsb);
+	list_add_tail(&fm->fc_entry, &fc->mounts);
+	up_write(&fc->killsb);
+
+	sb->s_flags |= SB_ACTIVE;
+	fsc->root = dget(sb->s_root);
+
 	return 0;
 }
 
-- 
2.31.1

[Virtio-fs] [PATCH v2 5/7] fuse: Call vfs_get_tree() for submounts

[Greg Kurz]

We recently fixed an infinite loop by setting the SB_BORN flag on
submounts along with the write barrier needed by super_cache_count().
This is the job of vfs_get_tree() and FUSE shouldn't have to care
about the barrier at all.

Split out some code from fuse_dentry_automount() to the dedicated
fuse_get_tree_submount() handler for submounts and call vfs_get_tree().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c   | 53 +++++--------------------------------------------
 fs/fuse/inode.c | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3fa8604c21d5..b88e5785a3dd 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -309,12 +309,8 @@ static int fuse_dentry_delete(const struct dentry *dentry)
 static struct vfsmount *fuse_dentry_automount(struct path *path)
 {
 	struct fs_context *fsc;
-	struct fuse_mount *parent_fm = get_fuse_mount_super(path->mnt->mnt_sb);
-	struct fuse_conn *fc = parent_fm->fc;
-	struct fuse_mount *fm;
 	struct vfsmount *mnt;
 	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
-	struct super_block *sb;
 	int err;
 
 	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
@@ -323,48 +319,15 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out;
 	}
 
-	err = -ENOMEM;
-	fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
-	if (!fm)
-		goto out_put_fsc;
+	/* Pass the FUSE inode of the mount for fuse_get_tree_submount() */
+	fsc->fs_private = mp_fi;
 
-	fsc->s_fs_info = fm;
-	sb = sget_fc(fsc, NULL, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		err = PTR_ERR(sb);
-		kfree(fm);
+	err = vfs_get_tree(fsc);
+	if (err)
 		goto out_put_fsc;
-	}
-	fm->fc = fuse_conn_get(fc);
-
-	/* Initialize superblock, making @mp_fi its root */
-	err = fuse_fill_super_submount(sb, mp_fi);
-	if (err) {
-		fuse_conn_put(fc);
-		kfree(fm);
-		sb->s_fs_info = NULL;
-		goto out_put_sb;
-	}
-
-	down_write(&fc->killsb);
-	list_add_tail(&fm->fc_entry, &fc->mounts);
-	up_write(&fc->killsb);
-
-	sb->s_flags |= SB_ACTIVE;
-	fsc->root = dget(sb->s_root);
-
-	/*
-	 * FIXME: setting SB_BORN requires a write barrier for
-	 *        super_cache_count(). We should actually come
-	 *        up with a proper ->get_tree() implementation
-	 *        for submounts and call vfs_get_tree() to take
-	 *        care of the write barrier.
-	 */
-	smp_wmb();
-	sb->s_flags |= SB_BORN;
 
 	/* We are done configuring the superblock, so unlock it */
-	up_write(&sb->s_umount);
+	up_write(&fsc->root->d_sb->s_umount);
 
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
@@ -376,12 +339,6 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	put_fs_context(fsc);
 	return mnt;
 
-out_put_sb:
-	/*
-	 * Only jump here when fsc->root is NULL and sb is still locked
-	 * (otherwise put_fs_context() will put the superblock)
-	 */
-	deactivate_locked_super(sb);
 out_put_fsc:
 	put_fs_context(fsc);
 out:
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fa96a3762ea2..bec1676811d4 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,8 +1313,44 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+/* Filesystem context private data holds the FUSE inode of the mount point */
 static int fuse_get_tree_submount(struct fs_context *fsc)
 {
+	struct fuse_mount *fm;
+	struct fuse_inode *mp_fi = fsc->fs_private;
+	struct fuse_conn *fc = get_fuse_conn(&mp_fi->inode);
+	struct super_block *sb;
+	int err;
+
+	fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
+	if (!fm)
+		return -ENOMEM;
+
+	fsc->s_fs_info = fm;
+	sb = sget_fc(fsc, NULL, set_anon_super_fc);
+	if (IS_ERR(sb)) {
+		kfree(fm);
+		return PTR_ERR(sb);
+	}
+	fm->fc = fuse_conn_get(fc);
+
+	/* Initialize superblock, making @mp_fi its root */
+	err = fuse_fill_super_submount(sb, mp_fi);
+	if (err) {
+		fuse_conn_put(fc);
+		kfree(fm);
+		sb->s_fs_info = NULL;
+		deactivate_locked_super(sb);
+		return err;
+	}
+
+	down_write(&fc->killsb);
+	list_add_tail(&fm->fc_entry, &fc->mounts);
+	up_write(&fc->killsb);
+
+	sb->s_flags |= SB_ACTIVE;
+	fsc->root = dget(sb->s_root);
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH v2 5/7] fuse: Call vfs_get_tree() for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> We recently fixed an infinite loop by setting the SB_BORN flag on
> submounts along with the write barrier needed by super_cache_count().
> This is the job of vfs_get_tree() and FUSE shouldn't have to care
> about the barrier at all.
>
> Split out some code from fuse_dentry_automount() to the dedicated
> fuse_get_tree_submount() handler for submounts and call vfs_get_tree().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c   | 53 +++++--------------------------------------------
>   fs/fuse/inode.c | 36 +++++++++++++++++++++++++++++++++
>   2 files changed, 41 insertions(+), 48 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Virtio-fs] [PATCH v2 5/7] fuse: Call vfs_get_tree() for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> We recently fixed an infinite loop by setting the SB_BORN flag on
> submounts along with the write barrier needed by super_cache_count().
> This is the job of vfs_get_tree() and FUSE shouldn't have to care
> about the barrier at all.
>
> Split out some code from fuse_dentry_automount() to the dedicated
> fuse_get_tree_submount() handler for submounts and call vfs_get_tree().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c   | 53 +++++--------------------------------------------
>   fs/fuse/inode.c | 36 +++++++++++++++++++++++++++++++++
>   2 files changed, 41 insertions(+), 48 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Greg Kurz]

fc_mount() already handles the vfs_get_tree(), sb->s_umount
unlocking and vfs_create_mount() sequence. Using it greatly
simplifies fuse_dentry_automount().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index b88e5785a3dd..fc9eddf7f9b2 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	struct fs_context *fsc;
 	struct vfsmount *mnt;
 	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
-	int err;
 
 	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
-	if (IS_ERR(fsc)) {
-		err = PTR_ERR(fsc);
-		goto out;
-	}
+	if (IS_ERR(fsc))
+		return (struct vfsmount *) fsc;
 
 	/* Pass the FUSE inode of the mount for fuse_get_tree_submount() */
 	fsc->fs_private = mp_fi;
 
-	err = vfs_get_tree(fsc);
-	if (err)
-		goto out_put_fsc;
-
-	/* We are done configuring the superblock, so unlock it */
-	up_write(&fsc->root->d_sb->s_umount);
-
 	/* Create the submount */
-	mnt = vfs_create_mount(fsc);
-	if (IS_ERR(mnt)) {
-		err = PTR_ERR(mnt);
+	mnt = fc_mount(fsc);
+	if (IS_ERR(mnt))
 		goto out_put_fsc;
-	}
 	mntget(mnt);
-	put_fs_context(fsc);
-	return mnt;
-
 out_put_fsc:
 	put_fs_context(fsc);
-out:
-	return ERR_PTR(err);
+	return mnt;
 }
 
 const struct dentry_operations fuse_dentry_operations = {
-- 
2.31.1

[Virtio-fs] [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Greg Kurz]

fc_mount() already handles the vfs_get_tree(), sb->s_umount
unlocking and vfs_create_mount() sequence. Using it greatly
simplifies fuse_dentry_automount().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index b88e5785a3dd..fc9eddf7f9b2 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	struct fs_context *fsc;
 	struct vfsmount *mnt;
 	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
-	int err;
 
 	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
-	if (IS_ERR(fsc)) {
-		err = PTR_ERR(fsc);
-		goto out;
-	}
+	if (IS_ERR(fsc))
+		return (struct vfsmount *) fsc;
 
 	/* Pass the FUSE inode of the mount for fuse_get_tree_submount() */
 	fsc->fs_private = mp_fi;
 
-	err = vfs_get_tree(fsc);
-	if (err)
-		goto out_put_fsc;
-
-	/* We are done configuring the superblock, so unlock it */
-	up_write(&fsc->root->d_sb->s_umount);
-
 	/* Create the submount */
-	mnt = vfs_create_mount(fsc);
-	if (IS_ERR(mnt)) {
-		err = PTR_ERR(mnt);
+	mnt = fc_mount(fsc);
+	if (IS_ERR(mnt))
 		goto out_put_fsc;
-	}
 	mntget(mnt);
-	put_fs_context(fsc);
-	return mnt;
-
 out_put_fsc:
 	put_fs_context(fsc);
-out:
-	return ERR_PTR(err);
+	return mnt;
 }
 
 const struct dentry_operations fuse_dentry_operations = {
-- 
2.31.1

Re: [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> fc_mount() already handles the vfs_get_tree(), sb->s_umount
> unlocking and vfs_create_mount() sequence. Using it greatly
> simplifies fuse_dentry_automount().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 26 +++++---------------------
>   1 file changed, 5 insertions(+), 21 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index b88e5785a3dd..fc9eddf7f9b2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
>   	struct fs_context *fsc;
>   	struct vfsmount *mnt;
>   	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> -	int err;
>   
>   	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> -	if (IS_ERR(fsc)) {
> -		err = PTR_ERR(fsc);
> -		goto out;
> -	}
> +	if (IS_ERR(fsc))
> +		return (struct vfsmount *) fsc;

I think ERR_CAST(fsc) would be nicer.

Apart from that:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Virtio-fs] [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> fc_mount() already handles the vfs_get_tree(), sb->s_umount
> unlocking and vfs_create_mount() sequence. Using it greatly
> simplifies fuse_dentry_automount().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c | 26 +++++---------------------
>   1 file changed, 5 insertions(+), 21 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index b88e5785a3dd..fc9eddf7f9b2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
>   	struct fs_context *fsc;
>   	struct vfsmount *mnt;
>   	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> -	int err;
>   
>   	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> -	if (IS_ERR(fsc)) {
> -		err = PTR_ERR(fsc);
> -		goto out;
> -	}
> +	if (IS_ERR(fsc))
> +		return (struct vfsmount *) fsc;

I think ERR_CAST(fsc) would be nicer.

Apart from that:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Greg Kurz]

On Tue, 8 Jun 2021 17:51:03 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 04.06.21 18:11, Greg Kurz wrote:
> > fc_mount() already handles the vfs_get_tree(), sb->s_umount
> > unlocking and vfs_create_mount() sequence. Using it greatly
> > simplifies fuse_dentry_automount().
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   fs/fuse/dir.c | 26 +++++---------------------
> >   1 file changed, 5 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index b88e5785a3dd..fc9eddf7f9b2 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
> >   	struct fs_context *fsc;
> >   	struct vfsmount *mnt;
> >   	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> > -	int err;
> >   
> >   	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> > -	if (IS_ERR(fsc)) {
> > -		err = PTR_ERR(fsc);
> > -		goto out;
> > -	}
> > +	if (IS_ERR(fsc))
> > +		return (struct vfsmount *) fsc;
> 
> I think ERR_CAST(fsc) would be nicer.
> 

Indeed. I'll fix that if I need to repost.

> Apart from that:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

Re: [Virtio-fs] [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Greg Kurz]

On Tue, 8 Jun 2021 17:51:03 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 04.06.21 18:11, Greg Kurz wrote:
> > fc_mount() already handles the vfs_get_tree(), sb->s_umount
> > unlocking and vfs_create_mount() sequence. Using it greatly
> > simplifies fuse_dentry_automount().
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   fs/fuse/dir.c | 26 +++++---------------------
> >   1 file changed, 5 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index b88e5785a3dd..fc9eddf7f9b2 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
> >   	struct fs_context *fsc;
> >   	struct vfsmount *mnt;
> >   	struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> > -	int err;
> >   
> >   	fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> > -	if (IS_ERR(fsc)) {
> > -		err = PTR_ERR(fsc);
> > -		goto out;
> > -	}
> > +	if (IS_ERR(fsc))
> > +		return (struct vfsmount *) fsc;
> 
> I think ERR_CAST(fsc) would be nicer.
> 

Indeed. I'll fix that if I need to repost.

> Apart from that:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

Re: [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Miklos Szeredi]

On Wed, 9 Jun 2021 at 09:45, Greg Kurz <groug@kaod.org> wrote:
>
> On Tue, 8 Jun 2021 17:51:03 +0200
> Max Reitz <mreitz@redhat.com> wrote:
>
> > On 04.06.21 18:11, Greg Kurz wrote:
> > > fc_mount() already handles the vfs_get_tree(), sb->s_umount
> > > unlocking and vfs_create_mount() sequence. Using it greatly
> > > simplifies fuse_dentry_automount().
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >   fs/fuse/dir.c | 26 +++++---------------------
> > >   1 file changed, 5 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index b88e5785a3dd..fc9eddf7f9b2 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
> > >     struct fs_context *fsc;
> > >     struct vfsmount *mnt;
> > >     struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> > > -   int err;
> > >
> > >     fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> > > -   if (IS_ERR(fsc)) {
> > > -           err = PTR_ERR(fsc);
> > > -           goto out;
> > > -   }
> > > +   if (IS_ERR(fsc))
> > > +           return (struct vfsmount *) fsc;
> >
> > I think ERR_CAST(fsc) would be nicer.
> >
>
> Indeed. I'll fix that if I need to repost.

Fixed.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH v2 6/7] fuse: Switch to fc_mount() for submounts

[Miklos Szeredi]

On Wed, 9 Jun 2021 at 09:45, Greg Kurz <groug@kaod.org> wrote:
>
> On Tue, 8 Jun 2021 17:51:03 +0200
> Max Reitz <mreitz@redhat.com> wrote:
>
> > On 04.06.21 18:11, Greg Kurz wrote:
> > > fc_mount() already handles the vfs_get_tree(), sb->s_umount
> > > unlocking and vfs_create_mount() sequence. Using it greatly
> > > simplifies fuse_dentry_automount().
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >   fs/fuse/dir.c | 26 +++++---------------------
> > >   1 file changed, 5 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index b88e5785a3dd..fc9eddf7f9b2 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -311,38 +311,22 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
> > >     struct fs_context *fsc;
> > >     struct vfsmount *mnt;
> > >     struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
> > > -   int err;
> > >
> > >     fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
> > > -   if (IS_ERR(fsc)) {
> > > -           err = PTR_ERR(fsc);
> > > -           goto out;
> > > -   }
> > > +   if (IS_ERR(fsc))
> > > +           return (struct vfsmount *) fsc;
> >
> > I think ERR_CAST(fsc) would be nicer.
> >
>
> Indeed. I'll fix that if I need to repost.

Fixed.

Thanks,
Miklos

[PATCH v2 7/7] fuse: Make fuse_fill_super_submount() static

[Greg Kurz]

This function used to be called from fuse_dentry_automount(). This code
was moved to fuse_get_tree_submount() in the same file since then. It
is unlikely there will ever be another user. No need to be extern in
this case.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 fs/fuse/fuse_i.h | 9 ---------
 fs/fuse/inode.c  | 4 ++--
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 862ad317bc89..1d32251a439a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1081,15 +1081,6 @@ void fuse_send_init(struct fuse_mount *fm);
  */
 int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx);
 
-/*
- * Fill in superblock for submounts
- * @sb: partially-initialized superblock to fill in
- * @parent_fi: The fuse_inode of the parent filesystem where this submount is
- * 	       mounted
- */
-int fuse_fill_super_submount(struct super_block *sb,
-			     struct fuse_inode *parent_fi);
-
 /*
  * Remove the mount from the connection
  *
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index bec1676811d4..72d44121ea38 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1275,8 +1275,8 @@ static void fuse_sb_defaults(struct super_block *sb)
 		sb->s_xattr = fuse_no_acl_xattr_handlers;
 }
 
-int fuse_fill_super_submount(struct super_block *sb,
-			     struct fuse_inode *parent_fi)
+static int fuse_fill_super_submount(struct super_block *sb,
+				    struct fuse_inode *parent_fi)
 {
 	struct fuse_mount *fm = get_fuse_mount_super(sb);
 	struct super_block *parent_sb = parent_fi->inode.i_sb;
-- 
2.31.1

[Virtio-fs] [PATCH v2 7/7] fuse: Make fuse_fill_super_submount() static

[Greg Kurz]

This function used to be called from fuse_dentry_automount(). This code
was moved to fuse_get_tree_submount() in the same file since then. It
is unlikely there will ever be another user. No need to be extern in
this case.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 fs/fuse/fuse_i.h | 9 ---------
 fs/fuse/inode.c  | 4 ++--
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 862ad317bc89..1d32251a439a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1081,15 +1081,6 @@ void fuse_send_init(struct fuse_mount *fm);
  */
 int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx);
 
-/*
- * Fill in superblock for submounts
- * @sb: partially-initialized superblock to fill in
- * @parent_fi: The fuse_inode of the parent filesystem where this submount is
- * 	       mounted
- */
-int fuse_fill_super_submount(struct super_block *sb,
-			     struct fuse_inode *parent_fi);
-
 /*
  * Remove the mount from the connection
  *
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index bec1676811d4..72d44121ea38 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1275,8 +1275,8 @@ static void fuse_sb_defaults(struct super_block *sb)
 		sb->s_xattr = fuse_no_acl_xattr_handlers;
 }
 
-int fuse_fill_super_submount(struct super_block *sb,
-			     struct fuse_inode *parent_fi)
+static int fuse_fill_super_submount(struct super_block *sb,
+				    struct fuse_inode *parent_fi)
 {
 	struct fuse_mount *fm = get_fuse_mount_super(sb);
 	struct super_block *parent_sb = parent_fi->inode.i_sb;
-- 
2.31.1

Re: [PATCH v2 0/7] fuse: Some fixes for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> v2:
>
> - add an extra fix (patch 2) : mount is now added to the list before
>    unlocking sb->s_umount
> - set SB_BORN just before unlocking sb->s_umount, just like it would
>    happen when using fc_mount() (Max)
> - don't allocate a FUSE context for the submounts (Max)
> - introduce a dedicated context ops for submounts
> - add a extra cleanup : simplify the code even more with fc_mount()
>
> v1:
>
> While working on adding syncfs() support in FUSE, I've hit some severe
> bugs with submounts (a crash and an infinite loop). The fix for the
> crash is straightforward (patch 1), but the fix for the infinite loop
> is more invasive : as suggested by Miklos, a simple bug fix is applied
> first (patch 2) and the final fix (patch 3) is applied on top.

Thanks a lot for these patches. I’ve had a style nit on patch 6, but 
other than that, looks nice to me.

Max

Re: [Virtio-fs] [PATCH v2 0/7] fuse: Some fixes for submounts

[Max Reitz]

On 04.06.21 18:11, Greg Kurz wrote:
> v2:
>
> - add an extra fix (patch 2) : mount is now added to the list before
>    unlocking sb->s_umount
> - set SB_BORN just before unlocking sb->s_umount, just like it would
>    happen when using fc_mount() (Max)
> - don't allocate a FUSE context for the submounts (Max)
> - introduce a dedicated context ops for submounts
> - add a extra cleanup : simplify the code even more with fc_mount()
>
> v1:
>
> While working on adding syncfs() support in FUSE, I've hit some severe
> bugs with submounts (a crash and an infinite loop). The fix for the
> crash is straightforward (patch 1), but the fix for the infinite loop
> is more invasive : as suggested by Miklos, a simple bug fix is applied
> first (patch 2) and the final fix (patch 3) is applied on top.

Thanks a lot for these patches. I’ve had a style nit on patch 6, but 
other than that, looks nice to me.

Max

Re: [PATCH v2 0/7] fuse: Some fixes for submounts

[Greg Kurz]

On Tue, 8 Jun 2021 17:52:13 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 04.06.21 18:11, Greg Kurz wrote:
> > v2:
> >
> > - add an extra fix (patch 2) : mount is now added to the list before
> >    unlocking sb->s_umount
> > - set SB_BORN just before unlocking sb->s_umount, just like it would
> >    happen when using fc_mount() (Max)
> > - don't allocate a FUSE context for the submounts (Max)
> > - introduce a dedicated context ops for submounts
> > - add a extra cleanup : simplify the code even more with fc_mount()
> >
> > v1:
> >
> > While working on adding syncfs() support in FUSE, I've hit some severe
> > bugs with submounts (a crash and an infinite loop). The fix for the
> > crash is straightforward (patch 1), but the fix for the infinite loop
> > is more invasive : as suggested by Miklos, a simple bug fix is applied
> > first (patch 2) and the final fix (patch 3) is applied on top.
> 
> Thanks a lot for these patches. I’ve had a style nit on patch 6, but 
> other than that, looks nice to me.
> 

Thanks a lot for the review !

Cheers,

--
Greg

> Max
> 

Re: [Virtio-fs] [PATCH v2 0/7] fuse: Some fixes for submounts

[Greg Kurz]

On Tue, 8 Jun 2021 17:52:13 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 04.06.21 18:11, Greg Kurz wrote:
> > v2:
> >
> > - add an extra fix (patch 2) : mount is now added to the list before
> >    unlocking sb->s_umount
> > - set SB_BORN just before unlocking sb->s_umount, just like it would
> >    happen when using fc_mount() (Max)
> > - don't allocate a FUSE context for the submounts (Max)
> > - introduce a dedicated context ops for submounts
> > - add a extra cleanup : simplify the code even more with fc_mount()
> >
> > v1:
> >
> > While working on adding syncfs() support in FUSE, I've hit some severe
> > bugs with submounts (a crash and an infinite loop). The fix for the
> > crash is straightforward (patch 1), but the fix for the infinite loop
> > is more invasive : as suggested by Miklos, a simple bug fix is applied
> > first (patch 2) and the final fix (patch 3) is applied on top.
> 
> Thanks a lot for these patches. I’ve had a style nit on patch 6, but 
> other than that, looks nice to me.
> 

Thanks a lot for the review !

Cheers,

--
Greg

> Max
>