[PATCH v2 0/2] Prevent re-use of FUSE superblock after force unmount

[PATCH v2 0/2] Prevent re-use of FUSE superblock after force unmount

[Daniil Lunev]

userspace counterpart. However, open file handles will prevent the
superblock from being reclaimed. An attempt to remount the filesystem at
the same endpoint will try re-using the superblock, if still present.
Since the superblock re-use path doesn't go through the fs-specific
superblock setup code, its state in FUSE case is already disfunctional,
and that will prevent the mount from succeeding.

Changes in v2:
- Remove super from list of superblocks instead of using a flag

Daniil Lunev (2):
  fs/super: function to prevent super re-use
  FUSE: Retire superblock on force unmount

 fs/fuse/inode.c    |  7 +++++--
 fs/super.c         | 51 ++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h |  1 +
 3 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.31.0

[PATCH v2 1/2] fs/super: function to prevent super re-use

[Daniil Lunev]

The function is called from generic_shutdown_super upon super
destruction, but also can be called by a filesystem during unmount to
ensure a new superblock is created upon the next mount. This is
necessary for the cases, where force unmount of a filesystem renders
superblock disfunctional, in a state that can not be re-used (e.g. FUSE
severes the connection of the userspace daemon to the superblock which
prevents any further communications between them).

Signed-off-by: Daniil Lunev <dlunev@chromium.org>
---

Changes in v2:
- Remove super from list of superblocks instead of using a flag

 fs/super.c         | 51 ++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h |  1 +
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index f1d4a193602d6..f23e45434de15 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -422,6 +422,45 @@ bool trylock_super(struct super_block *sb)
 	return false;
 }
 
+/**
+ *	retire_locked_super	-	prevent superblock from re-use
+ *	@sb: superblock that shouldn't be re-used
+ *
+ *	retire_locked_super excludes the super from the list of superblocks and
+ *	removes the link between superblock and it bdi. After a call to this
+ *	function, any subsequent mount will allocate a new superblock, even if
+ *	the retired one would have been re-used otherwise. It is safe to call
+ *	the function multiple times for the same superblock, any subsequent
+ *	invocations after the first one have no effect.
+ */
+static void retire_locked_super(struct super_block *sb)
+{
+	spin_lock(&sb_lock);
+	hlist_del_init(&sb->s_instances);
+	spin_unlock(&sb_lock);
+	up_write(&sb->s_umount);
+	if (sb->s_bdi != &noop_backing_dev_info) {
+		if (sb->s_iflags & SB_I_PERSB_BDI)
+			bdi_unregister(sb->s_bdi);
+		bdi_put(sb->s_bdi);
+		sb->s_bdi = &noop_backing_dev_info;
+	}
+}
+
+/**
+ *	retire_super	-	prevent superblock from re-use
+ *	@sb: superblock that shouldn't be re-used
+ *
+ *	Variant of retire_locked_super, except that superblock is *not* locked
+ *	by caller.
+ */
+void retire_super(struct super_block *sb)
+{
+	down_write(&sb->s_umount);
+	retire_locked_super(sb);
+}
+EXPORT_SYMBOL(retire_super);
+
 /**
  *	generic_shutdown_super	-	common helper for ->kill_sb()
  *	@sb: superblock to kill
@@ -467,17 +506,7 @@ void generic_shutdown_super(struct super_block *sb)
 			   sb->s_id);
 		}
 	}
-	spin_lock(&sb_lock);
-	/* should be initialized for __put_super_and_need_restart() */
-	hlist_del_init(&sb->s_instances);
-	spin_unlock(&sb_lock);
-	up_write(&sb->s_umount);
-	if (sb->s_bdi != &noop_backing_dev_info) {
-		if (sb->s_iflags & SB_I_PERSB_BDI)
-			bdi_unregister(sb->s_bdi);
-		bdi_put(sb->s_bdi);
-		sb->s_bdi = &noop_backing_dev_info;
-	}
+	retire_locked_super(sb);
 }
 
 EXPORT_SYMBOL(generic_shutdown_super);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23a..dc9151b7f0102 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2424,6 +2424,7 @@ extern struct dentry *mount_nodev(struct file_system_type *fs_type,
 	int flags, void *data,
 	int (*fill_super)(struct super_block *, void *, int));
 extern struct dentry *mount_subtree(struct vfsmount *mnt, const char *path);
+void retire_super(struct super_block *sb);
 void generic_shutdown_super(struct super_block *sb);
 void kill_block_super(struct super_block *sb);
 void kill_anon_super(struct super_block *sb);
-- 
2.31.0

[PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

Force unmount of FUSE severes the connection with the user space, even
if there are still open files. Subsequent remount tries to re-use the
superblock held by the open files, which is meaningless in the FUSE case
after disconnect - reused super block doesn't have userspace counterpart
attached to it and is incapable of doing any IO.

Signed-off-by: Daniil Lunev <dlunev@chromium.org>
---

Changes in v2:
- Use an exported function instead of directly modifying superblock

 fs/fuse/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8c0665c5dff88..8875361544b2a 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -476,8 +476,11 @@ static void fuse_umount_begin(struct super_block *sb)
 {
 	struct fuse_conn *fc = get_fuse_conn_super(sb);
 
-	if (!fc->no_force_umount)
-		fuse_abort_conn(fc);
+	if (fc->no_force_umount)
+		return;
+
+	fuse_abort_conn(fc);
+	retire_super(sb);
 }
 
 static void fuse_send_destroy(struct fuse_mount *fm)
-- 
2.31.0

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Al Viro]

On Thu, May 12, 2022 at 08:29:10AM +1000, Daniil Lunev wrote:
> Force unmount of FUSE severes the connection with the user space, even
> if there are still open files. Subsequent remount tries to re-use the
> superblock held by the open files, which is meaningless in the FUSE case
> after disconnect - reused super block doesn't have userspace counterpart
> attached to it and is incapable of doing any IO.

	Why not simply have those simply rejected by fuse_test_super()?
Looks like that would be much smaller and less invasive patch...
Confused...

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Al Viro]

On Tue, May 17, 2022 at 10:20:06PM +0000, Al Viro wrote:
> On Thu, May 12, 2022 at 08:29:10AM +1000, Daniil Lunev wrote:
> > Force unmount of FUSE severes the connection with the user space, even
> > if there are still open files. Subsequent remount tries to re-use the
> > superblock held by the open files, which is meaningless in the FUSE case
> > after disconnect - reused super block doesn't have userspace counterpart
> > attached to it and is incapable of doing any IO.
> 
> 	Why not simply have those simply rejected by fuse_test_super()?
> Looks like that would be much smaller and less invasive patch...
> Confused...

... because Miklos had suggested that, apparently ;-/  I disagree -
that approach has more side effects.  "mount will skip that sucker" is,
AFAICS, the only effect of modiyfing test_super callback(s); yours, OTOH...

Note that generic_shutdown_super() is *not* called while superblock is
mounted anywhere.  And it doesn't get to eviction from the list while it still
has live dentries.  Or inodes, for that matter.

So this
        if (sb->s_bdi != &noop_backing_dev_info) {
		if (sb->s_iflags & SB_I_PERSB_BDI)
			bdi_unregister(sb->s_bdi);
		bdi_put(sb->s_bdi);
		sb->s_bdi = &noop_backing_dev_info;
	}
is almost certainly not safe to be done on a live superblock.

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

Hi Al,
Here is my v1 version of the patch which does skip the node in testing super
https://lore.kernel.org/linux-fsdevel/20220511013057.245827-1-dlunev@chromium.org/T/#u
I am not particular which version is to be used. However note, it still needs
to remove the bdi node since otherwise the new mount will have a conflict.
The bdi put should not kill the bdi if it has more references (and then
conflict would still occur, but chances of bdi being opened for a long time
is low AFAIU), thus this should be safe, but I might be missing something.
Note, that FUSE super block at the time is barely alive, it may have open
filehandles/references, but the connection with user space is already cut
and all of the ops would return transport error.
Let me know what you think and how we can address it better maybe?
Thanks,
Daniil

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Miklos Szeredi]

On Wed, 18 May 2022 at 00:32, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, May 17, 2022 at 10:20:06PM +0000, Al Viro wrote:
> > On Thu, May 12, 2022 at 08:29:10AM +1000, Daniil Lunev wrote:
> > > Force unmount of FUSE severes the connection with the user space, even
> > > if there are still open files. Subsequent remount tries to re-use the
> > > superblock held by the open files, which is meaningless in the FUSE case
> > > after disconnect - reused super block doesn't have userspace counterpart
> > > attached to it and is incapable of doing any IO.
> >
> >       Why not simply have those simply rejected by fuse_test_super()?
> > Looks like that would be much smaller and less invasive patch...
> > Confused...
>
> ... because Miklos had suggested that, apparently ;-/  I disagree -
> that approach has more side effects.  "mount will skip that sucker" is,
> AFAICS, the only effect of modiyfing test_super callback(s); yours, OTOH...

Yep, messing with the bdi doesn't look good.  Fuse always uses a
private bdi, so it's not even necessary.

Just removing from type->fs_supers should not have any side effects,
at least I can't spot any.

Fixing fuse_test_super() is not sufficient, as the fuseblk type goes
though get_tree_bdev().  That could be tweaked as well, but it would
end up with more complexity.

Thanks,
Miklos

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

> Yep, messing with the bdi doesn't look good.  Fuse always uses a
> private bdi, so it's not even necessary.

The reason I needed to remove the bdi is name collision - fuse
generates a fixed name for its bdi based on the underlying block
device. However, those collisions of mine were conducted on a
version prior to the private bdi introduction, I am not sure if that
is supposed to fix the collision issue. Need to check

Thanks,
Daniil

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

So, I tried this patchset with open bdi elements during force unmount
and a random file open [1], and didn't see any major drama with
force unmounting the node, after re-mounting, read on sysfs node
returned "no such device", which is expected.
With private bdi flag patch, unless bdi is unregister on force unmount
in fuse, it will complain on name collision [2] (because the patch
actually doesn't do much but unregisters the bdi on unmount, which
seems to happen ok even if node is busy).
Let me know if I am missing anything or if there are any other
concerns, and advise what would be the best way to move this
forward.

Thanks,
Daniil.


[1] Python shell
>>> f1 = open('/sys/class/bdi/8:0-fuseblk/read_ahead_kb', 'r')
>>> f2 = open('/media/removable/USB Drive/m1', 'w')

[2]
[  149.826508] sysfs: cannot create duplicate filename
'/devices/virtual/bdi/8:0-fuseblk'

On Thu, May 19, 2022 at 8:55 AM Daniil Lunev <dlunev@chromium.org> wrote:
>
> > Yep, messing with the bdi doesn't look good.  Fuse always uses a
> > private bdi, so it's not even necessary.
>
> The reason I needed to remove the bdi is name collision - fuse
> generates a fixed name for its bdi based on the underlying block
> device. However, those collisions of mine were conducted on a
> version prior to the private bdi introduction, I am not sure if that
> is supposed to fix the collision issue. Need to check
>
> Thanks,
> Daniil

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Miklos Szeredi]

On Mon, 23 May 2022 at 02:25, Daniil Lunev <dlunev@chromium.org> wrote:
>
> So, I tried this patchset with open bdi elements during force unmount
> and a random file open [1], and didn't see any major drama with
> force unmounting the node, after re-mounting, read on sysfs node
> returned "no such device", which is expected.
> With private bdi flag patch, unless bdi is unregister on force unmount
> in fuse, it will complain on name collision [2] (because the patch
> actually doesn't do much but unregisters the bdi on unmount, which
> seems to happen ok even if node is busy).

Calling bdi_unregister() might be okay, and that should fix this.  I'm
not familiar enough with that part to say for sure.

But freeing sb->s_bdi while the superblock is active looks problematic.

Thanks,
Miklos

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

> Calling bdi_unregister() might be okay, and that should fix this.  I'm
> not familiar enough with that part to say for sure.
> But freeing sb->s_bdi while the superblock is active looks problematic.
Tracing the code, I see that, yes, in general that might not be safe to call
the "bdi_put" function for any FS - because it might have in-flight ops even
with force, where they will routinely access the members of the bdi struct
without locks. However, we do replace the struct with a no_op version,
and specifically for the FUSE case we sever the connection first, so no
in-flight ops can actually be there. And I am not sure if other FS may
need to do this retirement, given they don't have these
user-space/kernel split. It would make sense however to delay the actual
put till  the actual super block destruction, but that would require
introducing extra state tracking to see if the block is or is not registered
anymore. It can be piggybacked on the v1 patch where I have an explicit
state flag, but not on v2.
Miklos, Al, will the following work and be acceptable?
Get v1 patchset[1], in fuse_umount_begin do bdi_unregister and set
the flag, but do not do bdi_put or replacement with no_op. And then in
generic shutdown super if the bdi is not no_op and the 'defunct' flag is
set, skip unregister part.
Thanks,
Daniil

[1]
https://lore.kernel.org/linux-fsdevel/20220511013057.245827-1-dlunev@chromium.org/T/#u

Re: [PATCH v2 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

I have prepared the v3 patch as described in my previous email. PTAL.

Thanks,
Daniil

On Wed, May 25, 2022 at 8:44 AM Daniil Lunev <dlunev@chromium.org> wrote:
>
> > Calling bdi_unregister() might be okay, and that should fix this.  I'm
> > not familiar enough with that part to say for sure.
> > But freeing sb->s_bdi while the superblock is active looks problematic.
> Tracing the code, I see that, yes, in general that might not be safe to call
> the "bdi_put" function for any FS - because it might have in-flight ops even
> with force, where they will routinely access the members of the bdi struct
> without locks. However, we do replace the struct with a no_op version,
> and specifically for the FUSE case we sever the connection first, so no
> in-flight ops can actually be there. And I am not sure if other FS may
> need to do this retirement, given they don't have these
> user-space/kernel split. It would make sense however to delay the actual
> put till  the actual super block destruction, but that would require
> introducing extra state tracking to see if the block is or is not registered
> anymore. It can be piggybacked on the v1 patch where I have an explicit
> state flag, but not on v2.
> Miklos, Al, will the following work and be acceptable?
> Get v1 patchset[1], in fuse_umount_begin do bdi_unregister and set
> the flag, but do not do bdi_put or replacement with no_op. And then in
> generic shutdown super if the bdi is not no_op and the 'defunct' flag is
> set, skip unregister part.
> Thanks,
> Daniil
>
> [1]
> https://lore.kernel.org/linux-fsdevel/20220511013057.245827-1-dlunev@chromium.org/T/#u