All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] fuse: Some fixes for submounts
@ 2021-05-25 15:02 ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, Vivek Goyal, virtio-fs, Greg Kurz

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 (4):
  fuse: Fix crash in fuse_dentry_automount() error path
  fuse: Fix infinite loop in sget_fc()
  fuse: Call vfs_get_tree() for submounts
  fuse: Make fuse_fill_super_submount() static

 fs/fuse/dir.c       | 46 ++++++++++---------------------------------
 fs/fuse/fuse_i.h    |  9 +++------
 fs/fuse/inode.c     | 48 +++++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 62 insertions(+), 44 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Virtio-fs] [PATCH 0/4] fuse: Some fixes for submounts
@ 2021-05-25 15:02 ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

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 (4):
  fuse: Fix crash in fuse_dentry_automount() error path
  fuse: Fix infinite loop in sget_fc()
  fuse: Call vfs_get_tree() for submounts
  fuse: Make fuse_fill_super_submount() static

 fs/fuse/dir.c       | 46 ++++++++++---------------------------------
 fs/fuse/fuse_i.h    |  9 +++------
 fs/fuse/inode.c     | 48 +++++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 62 insertions(+), 44 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/4] fuse: Fix crash in fuse_dentry_automount() error path
  2021-05-25 15:02 ` [Virtio-fs] " Greg Kurz
@ 2021-05-25 15:02   ` Greg Kurz
  -1 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, Vivek Goyal, virtio-fs, Greg Kurz,
	mreitz, stable

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>
---
 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Virtio-fs] [PATCH 1/4] fuse: Fix crash in fuse_dentry_automount() error path
@ 2021-05-25 15:02   ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, stable, mreitz, virtio-fs, linux-fsdevel, Vivek Goyal

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>
---
 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/4] fuse: Fix infinite loop in sget_fc()
  2021-05-25 15:02 ` [Virtio-fs] " Greg Kurz
@ 2021-05-25 15:02   ` Greg Kurz
  -1 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, Vivek Goyal, virtio-fs, Greg Kurz,
	mreitz, stable

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 should go away. 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 01559061cbfb..3b0482738741 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -346,6 +346,16 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out_put_sb;
 	}
 
+	/*
+	 * 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;
+
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
 	/* We are done configuring the superblock, so unlock it */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Virtio-fs] [PATCH 2/4] fuse: Fix infinite loop in sget_fc()
@ 2021-05-25 15:02   ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, stable, mreitz, virtio-fs, linux-fsdevel, Vivek Goyal

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 should go away. 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 01559061cbfb..3b0482738741 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -346,6 +346,16 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 		goto out_put_sb;
 	}
 
+	/*
+	 * 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;
+
 	sb->s_flags |= SB_ACTIVE;
 	fsc->root = dget(sb->s_root);
 	/* We are done configuring the superblock, so unlock it */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/4] fuse: Call vfs_get_tree() for submounts
  2021-05-25 15:02 ` [Virtio-fs] " Greg Kurz
@ 2021-05-25 15:02   ` Greg Kurz
  -1 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, Vivek Goyal, virtio-fs, 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 a new dedicated
fuse_get_tree_submount() handler for submounts and call vfs_get_tree().

The fs_private field of the filesystem context isn't used with
submounts : hijack it to pass the FUSE inode of the mount point
down to fuse_get_tree_submount().

Finally, adapt virtiofs to use this.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c       | 58 +++++++--------------------------------------
 fs/fuse/fuse_i.h    |  6 +++++
 fs/fuse/inode.c     | 44 ++++++++++++++++++++++++++++++++++
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3b0482738741..97649dcfeccd 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,47 +319,17 @@ 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;
-
-	fsc->s_fs_info = fm;
-	sb = sget_fc(fsc, NULL, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		err = PTR_ERR(sb);
-		kfree(fm);
-		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;
-	}
-
 	/*
-	 * 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.
+	 * Hijack fsc->fs_private to pass the mount point inode to
+	 * fuse_get_tree_submount(). It *must* be NULLified afterwards
+	 * to avoid the inode pointer to be passed to kfree() when
+	 * the context gets freed.
 	 */
-	smp_wmb();
-	sb->s_flags |= SB_BORN;
-
-	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);
+	fsc->fs_private = mp_fi;
+	err = vfs_get_tree(fsc);
+	fsc->fs_private = NULL;
+	if (err)
+		goto out_put_fsc;
 
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
@@ -375,12 +341,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/fuse_i.h b/fs/fuse/fuse_i.h
index 7e463e220053..d7fcf59a6a0e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1090,6 +1090,12 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx);
 int fuse_fill_super_submount(struct super_block *sb,
 			     struct fuse_inode *parent_fi);
 
+/*
+ * Get the mountable root for the submount
+ * @fsc: superblock configuration context
+ */
+int fuse_get_tree_submount(struct fs_context *fsc);
+
 /*
  * Remove the mount from the connection
  *
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 393e36b74dc4..433ca2b13046 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,6 +1313,50 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+/* Filesystem context private data holds the FUSE inode of the mount point */
+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;
+	}
+
+	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);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fuse_get_tree_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..e12e5190352c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1420,6 +1420,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 	unsigned int virtqueue_size;
 	int err = -EIO;
 
+	if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
+		return fuse_get_tree_submount(fsc);
+
 	/* This gets a reference on virtio_fs object. This ptr gets installed
 	 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
 	 * to drop the reference to this object.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Virtio-fs] [PATCH 3/4] fuse: Call vfs_get_tree() for submounts
@ 2021-05-25 15:02   ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

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 a new dedicated
fuse_get_tree_submount() handler for submounts and call vfs_get_tree().

The fs_private field of the filesystem context isn't used with
submounts : hijack it to pass the FUSE inode of the mount point
down to fuse_get_tree_submount().

Finally, adapt virtiofs to use this.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/fuse/dir.c       | 58 +++++++--------------------------------------
 fs/fuse/fuse_i.h    |  6 +++++
 fs/fuse/inode.c     | 44 ++++++++++++++++++++++++++++++++++
 fs/fuse/virtio_fs.c |  3 +++
 4 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3b0482738741..97649dcfeccd 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,47 +319,17 @@ 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;
-
-	fsc->s_fs_info = fm;
-	sb = sget_fc(fsc, NULL, set_anon_super_fc);
-	if (IS_ERR(sb)) {
-		err = PTR_ERR(sb);
-		kfree(fm);
-		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;
-	}
-
 	/*
-	 * 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.
+	 * Hijack fsc->fs_private to pass the mount point inode to
+	 * fuse_get_tree_submount(). It *must* be NULLified afterwards
+	 * to avoid the inode pointer to be passed to kfree() when
+	 * the context gets freed.
 	 */
-	smp_wmb();
-	sb->s_flags |= SB_BORN;
-
-	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);
+	fsc->fs_private = mp_fi;
+	err = vfs_get_tree(fsc);
+	fsc->fs_private = NULL;
+	if (err)
+		goto out_put_fsc;
 
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
@@ -375,12 +341,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/fuse_i.h b/fs/fuse/fuse_i.h
index 7e463e220053..d7fcf59a6a0e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1090,6 +1090,12 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx);
 int fuse_fill_super_submount(struct super_block *sb,
 			     struct fuse_inode *parent_fi);
 
+/*
+ * Get the mountable root for the submount
+ * @fsc: superblock configuration context
+ */
+int fuse_get_tree_submount(struct fs_context *fsc);
+
 /*
  * Remove the mount from the connection
  *
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 393e36b74dc4..433ca2b13046 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,6 +1313,50 @@ int fuse_fill_super_submount(struct super_block *sb,
 	return 0;
 }
 
+/* Filesystem context private data holds the FUSE inode of the mount point */
+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;
+	}
+
+	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);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fuse_get_tree_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..e12e5190352c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1420,6 +1420,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 	unsigned int virtqueue_size;
 	int err = -EIO;
 
+	if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
+		return fuse_get_tree_submount(fsc);
+
 	/* This gets a reference on virtio_fs object. This ptr gets installed
 	 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
 	 * to drop the reference to this object.
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/4] fuse: Make fuse_fill_super_submount() static
  2021-05-25 15:02 ` [Virtio-fs] " Greg Kurz
@ 2021-05-25 15:02   ` Greg Kurz
  -1 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, Vivek Goyal, virtio-fs, 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>
---
 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 d7fcf59a6a0e..e2f5c8617e0d 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);
-
 /*
  * Get the mountable root for the submount
  * @fsc: superblock configuration context
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 433ca2b13046..f591956c01b3 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Virtio-fs] [PATCH 4/4] fuse: Make fuse_fill_super_submount() static
@ 2021-05-25 15:02   ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-25 15:02 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

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>
---
 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 d7fcf59a6a0e..e2f5c8617e0d 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);
-
 /*
  * Get the mountable root for the submount
  * @fsc: superblock configuration context
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 433ca2b13046..f591956c01b3 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 1/4] fuse: Fix crash in fuse_dentry_automount() error path
  2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
@ 2021-05-27  9:51     ` Max Reitz
  -1 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2021-05-27  9:51 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: linux-kernel, stable, virtio-fs, linux-fsdevel, Vivek Goyal

On 25.05.21 17:02, Greg Kurz wrote:
> 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>
> ---
>   fs/fuse/dir.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 1/4] fuse: Fix crash in fuse_dentry_automount() error path
@ 2021-05-27  9:51     ` Max Reitz
  0 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2021-05-27  9:51 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: virtio-fs, linux-fsdevel, linux-kernel, stable, Vivek Goyal

On 25.05.21 17:02, Greg Kurz wrote:
> 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>
> ---
>   fs/fuse/dir.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 2/4] fuse: Fix infinite loop in sget_fc()
  2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
@ 2021-05-27 10:08     ` Max Reitz
  -1 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2021-05-27 10:08 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: linux-kernel, stable, virtio-fs, linux-fsdevel, Vivek Goyal

On 25.05.21 17:02, 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 should go away. 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 01559061cbfb..3b0482738741 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -346,6 +346,16 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
>   		goto out_put_sb;
>   	}
>   
> +	/*
> +	 * 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;
> +

I’m not sure whether we should have the order be exactly the same as in 
vfs_get_tree(), i.e. whether this should be put after fsc->root has been 
set.  Or maybe even after fm has been added to fc->mounts, because that 
too was part of the fuse_get_tree_submount() function of your “fuse: 
Call vfs_get_tree() for submounts” patch.

 From a quick look at SB_BORN users, it doesn’t seem to make a 
difference to me, though, so:

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

>   	sb->s_flags |= SB_ACTIVE;
>   	fsc->root = dget(sb->s_root);
>   	/* We are done configuring the superblock, so unlock it */
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 2/4] fuse: Fix infinite loop in sget_fc()
@ 2021-05-27 10:08     ` Max Reitz
  0 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2021-05-27 10:08 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: virtio-fs, linux-fsdevel, linux-kernel, stable, Vivek Goyal

On 25.05.21 17:02, 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 should go away. 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 01559061cbfb..3b0482738741 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -346,6 +346,16 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
>   		goto out_put_sb;
>   	}
>   
> +	/*
> +	 * 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;
> +

I’m not sure whether we should have the order be exactly the same as in 
vfs_get_tree(), i.e. whether this should be put after fsc->root has been 
set.  Or maybe even after fm has been added to fc->mounts, because that 
too was part of the fuse_get_tree_submount() function of your “fuse: 
Call vfs_get_tree() for submounts” patch.

 From a quick look at SB_BORN users, it doesn’t seem to make a 
difference to me, though, so:

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

>   	sb->s_flags |= SB_ACTIVE;
>   	fsc->root = dget(sb->s_root);
>   	/* We are done configuring the superblock, so unlock it */
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 2/4] fuse: Fix infinite loop in sget_fc()
  2021-05-27 10:08     ` Max Reitz
  (?)
@ 2021-05-27 12:31     ` Greg Kurz
  -1 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-05-27 12:31 UTC (permalink / raw)
  To: Max Reitz
  Cc: Miklos Szeredi, linux-kernel, stable, virtio-fs, linux-fsdevel,
	Vivek Goyal

On Thu, 27 May 2021 12:08:36 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 25.05.21 17:02, 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 should go away. 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 | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 01559061cbfb..3b0482738741 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -346,6 +346,16 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
> >   		goto out_put_sb;
> >   	}
> >   
> > +	/*
> > +	 * 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;
> > +
> 
> I’m not sure whether we should have the order be exactly the same as in 
> vfs_get_tree(), i.e. whether this should be put after fsc->root has been 
> set.  Or maybe even after fm has been added to fc->mounts, because that 
> too was part of the fuse_get_tree_submount() function of your “fuse: 
> Call vfs_get_tree() for submounts” patch.
> 

Good catch !

>  From a quick look at SB_BORN users, it doesn’t seem to make a 
> difference to me, though, so:
> 

Same here but I'm fine with posting a new version that
preserves the order.

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

Thanks !

--
Greg

> >   	sb->s_flags |= SB_ACTIVE;
> >   	fsc->root = dget(sb->s_root);
> >   	/* We are done configuring the superblock, so unlock it */
> > 
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 3/4] fuse: Call vfs_get_tree() for submounts
  2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
  (?)
@ 2021-05-27 13:24   ` Max Reitz
  2021-06-03  7:34       ` Greg Kurz
  -1 siblings, 1 reply; 19+ messages in thread
From: Max Reitz @ 2021-05-27 13:24 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

On 25.05.21 17:02, 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 a new dedicated
> fuse_get_tree_submount() handler for submounts and call vfs_get_tree().
> 
> The fs_private field of the filesystem context isn't used with
> submounts : hijack it to pass the FUSE inode of the mount point
> down to fuse_get_tree_submount().

What exactly do you mean by “isn’t used”?  virtio_fs_init_fs_context() 
still sets it (it is non-NULL in fuse_dentry_automount() after 
fs_context_for_submount()).  It does appear like it is never read, but 
one thing that definitely would need to be done is for it to be freed 
before putting mp_fi there.

So I think it may technically be fine to use this field, but then 
virtio_fs_init_fs_context() shouldn’t set it for submounts (should be 
discernible with fsc->purpose), and perhaps that should be a separate patch.

(Apart from that, this patch looks good to me, though.)

Max

> Finally, adapt virtiofs to use this.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   fs/fuse/dir.c       | 58 +++++++--------------------------------------
>   fs/fuse/fuse_i.h    |  6 +++++
>   fs/fuse/inode.c     | 44 ++++++++++++++++++++++++++++++++++
>   fs/fuse/virtio_fs.c |  3 +++
>   4 files changed, 62 insertions(+), 49 deletions(-)


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 4/4] fuse: Make fuse_fill_super_submount() static
  2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
  (?)
@ 2021-05-27 13:28   ` Max Reitz
  -1 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2021-05-27 13:28 UTC (permalink / raw)
  To: Greg Kurz, Miklos Szeredi
  Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

On 25.05.21 17:02, Greg Kurz wrote:
> 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>
> ---
>   fs/fuse/fuse_i.h | 9 ---------
>   fs/fuse/inode.c  | 4 ++--
>   2 files changed, 2 insertions(+), 11 deletions(-)

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 3/4] fuse: Call vfs_get_tree() for submounts
  2021-05-27 13:24   ` Max Reitz
@ 2021-06-03  7:34       ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-06-03  7:34 UTC (permalink / raw)
  To: Max Reitz
  Cc: Miklos Szeredi, linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

On Thu, 27 May 2021 15:24:40 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 25.05.21 17:02, 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 a new dedicated
> > fuse_get_tree_submount() handler for submounts and call vfs_get_tree().
> > 
> > The fs_private field of the filesystem context isn't used with
> > submounts : hijack it to pass the FUSE inode of the mount point
> > down to fuse_get_tree_submount().
> 
> What exactly do you mean by “isn’t used”?  virtio_fs_init_fs_context() 
> still sets it (it is non-NULL in fuse_dentry_automount() after 
> fs_context_for_submount()).  It does appear like it is never read, but 
> one thing that definitely would need to be done is for it to be freed 
> before putting mp_fi there.
> 

Oops... yes it should. Thanks for the catch !

> So I think it may technically be fine to use this field, but then 
> virtio_fs_init_fs_context() shouldn’t set it for submounts (should be 
> discernible with fsc->purpose), and perhaps that should be a separate patch.
> 

Yes, I'll do just that.

> (Apart from that, this patch looks good to me, though.)
> 
> Max
> 
> > Finally, adapt virtiofs to use this.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   fs/fuse/dir.c       | 58 +++++++--------------------------------------
> >   fs/fuse/fuse_i.h    |  6 +++++
> >   fs/fuse/inode.c     | 44 ++++++++++++++++++++++++++++++++++
> >   fs/fuse/virtio_fs.c |  3 +++
> >   4 files changed, 62 insertions(+), 49 deletions(-)
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Virtio-fs] [PATCH 3/4] fuse: Call vfs_get_tree() for submounts
@ 2021-06-03  7:34       ` Greg Kurz
  0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-06-03  7:34 UTC (permalink / raw)
  To: Max Reitz
  Cc: linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal, Miklos Szeredi

On Thu, 27 May 2021 15:24:40 +0200
Max Reitz <mreitz@redhat.com> wrote:

> On 25.05.21 17:02, 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 a new dedicated
> > fuse_get_tree_submount() handler for submounts and call vfs_get_tree().
> > 
> > The fs_private field of the filesystem context isn't used with
> > submounts : hijack it to pass the FUSE inode of the mount point
> > down to fuse_get_tree_submount().
> 
> What exactly do you mean by “isn’t used”?  virtio_fs_init_fs_context() 
> still sets it (it is non-NULL in fuse_dentry_automount() after 
> fs_context_for_submount()).  It does appear like it is never read, but 
> one thing that definitely would need to be done is for it to be freed 
> before putting mp_fi there.
> 

Oops... yes it should. Thanks for the catch !

> So I think it may technically be fine to use this field, but then 
> virtio_fs_init_fs_context() shouldn’t set it for submounts (should be 
> discernible with fsc->purpose), and perhaps that should be a separate patch.
> 

Yes, I'll do just that.

> (Apart from that, this patch looks good to me, though.)
> 
> Max
> 
> > Finally, adapt virtiofs to use this.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   fs/fuse/dir.c       | 58 +++++++--------------------------------------
> >   fs/fuse/fuse_i.h    |  6 +++++
> >   fs/fuse/inode.c     | 44 ++++++++++++++++++++++++++++++++++
> >   fs/fuse/virtio_fs.c |  3 +++
> >   4 files changed, 62 insertions(+), 49 deletions(-)
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-06-03  7:34 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25 15:02 [PATCH 0/4] fuse: Some fixes for submounts Greg Kurz
2021-05-25 15:02 ` [Virtio-fs] " Greg Kurz
2021-05-25 15:02 ` [PATCH 1/4] fuse: Fix crash in fuse_dentry_automount() error path Greg Kurz
2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
2021-05-27  9:51   ` Max Reitz
2021-05-27  9:51     ` Max Reitz
2021-05-25 15:02 ` [PATCH 2/4] fuse: Fix infinite loop in sget_fc() Greg Kurz
2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
2021-05-27 10:08   ` Max Reitz
2021-05-27 10:08     ` Max Reitz
2021-05-27 12:31     ` Greg Kurz
2021-05-25 15:02 ` [PATCH 3/4] fuse: Call vfs_get_tree() for submounts Greg Kurz
2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
2021-05-27 13:24   ` Max Reitz
2021-06-03  7:34     ` Greg Kurz
2021-06-03  7:34       ` Greg Kurz
2021-05-25 15:02 ` [PATCH 4/4] fuse: Make fuse_fill_super_submount() static Greg Kurz
2021-05-25 15:02   ` [Virtio-fs] " Greg Kurz
2021-05-27 13:28   ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.