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

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

[Daniil Lunev]

Force unmount of fuse severes the connection between FUSE driver and its
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.
The patchset adds a possibility to mark the superblock  "defunc", which
will prevent its re-use by the subsequent mounts, and uses the
functionality in FUSE driver.


Daniil Lunev (2):
  fs/super: Add a flag to mark super block defunc
  FUSE: Mark super block defunc on force unmount

 fs/fuse/inode.c    | 11 +++++++++--
 fs/super.c         |  4 ++--
 include/linux/fs.h |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.31.0

[PATCH 1/2] fs/super: Add a flag to mark super block defunc

[Daniil Lunev]

File system can mark a block "defunc" in order to prevent matching
against it in a new mount.

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

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

diff --git a/fs/super.c b/fs/super.c
index f1d4a193602d6..fc5b3efe0cd01 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1216,7 +1216,7 @@ static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
 
 static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
 {
-	return s->s_bdev == fc->sget_key;
+	return !s->s_defunc && s->s_bdev == fc->sget_key;
 }
 
 /**
@@ -1307,7 +1307,7 @@ EXPORT_SYMBOL(get_tree_bdev);
 
 static int test_bdev_super(struct super_block *s, void *data)
 {
-	return (void *)s->s_bdev == data;
+	return !s->s_defunc && (void *)s->s_bdev == data;
 }
 
 struct dentry *mount_bdev(struct file_system_type *fs_type,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23a..76feb3fe9bb9e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1448,6 +1448,7 @@ struct super_block {
 	struct rw_semaphore	s_umount;
 	int			s_count;
 	atomic_t		s_active;
+	bool			s_defunc;
 #ifdef CONFIG_SECURITY
 	void                    *s_security;
 #endif
-- 
2.31.0

Re: [PATCH 1/2] fs/super: Add a flag to mark super block defunc

[Theodore Ts'o]

On Wed, May 11, 2022 at 11:30:56AM +1000, Daniil Lunev wrote:
> File system can mark a block "defunc" in order to prevent matching
> against it in a new mount.

Spelling nit: s/defunc/defunct/

I would suggest changing s_defunc to s_defunct in the patch below, to
ease in readability.

Cheers,

					- Ted

Re: [PATCH 1/2] fs/super: Add a flag to mark super block defunc

[Christoph Hellwig]

On Wed, May 11, 2022 at 11:30:56AM +1000, Daniil Lunev wrote:
> File system can mark a block "defunc" in order to prevent matching
> against it in a new mount.

Why use a bool instead of using s_iflags?

Also I suspect we should never reuse a force mounted superblock,
but at least for block devices we should also not allow allocating
a new one for that case but just refuse the mount.

Re: [PATCH 1/2] fs/super: Add a flag to mark super block defunc

[Daniil Lunev]

On Thu, May 12, 2022 at 12:54 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, May 11, 2022 at 11:30:56AM +1000, Daniil Lunev wrote:
> > File system can mark a block "defunc" in order to prevent matching
> > against it in a new mount.
>
> Why use a bool instead of using s_iflags?
Oh, I was looking at the flag field, but for some reason got confused
about its values.
Looking again, I totally can use it. However, the other option, that
Miklos proposed in
a thread on the cover letter mail, is to remove the superblock from
"type->fs_supers".
I plan to upload a new version with that option. Which of those two
would you prefer?

> Also I suspect we should never reuse a force mounted superblock,
> but at least for block devices we should also not allow allocating
> a new one for that case but just refuse the mount.
Re-use of a force unmounted superblock "just works" for at least ext4
- in fact, it
continues to work as if nothing happened to it. Changing that may
break existing use
cases. For FUSE that unfortunately unachievable, given the daemon is
disconnected
from the driver and likely killed after force unmount, but ability to
re-mount is essential,
because force unmount is the only way to reliably achieve suspend
while using FUSE,
and if re-mount doesn't work, the connected device/share is missing
after resume.

Thanks,
Daniil

[PATCH 2/2] FUSE: Mark super block defunc 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>
---

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

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8c0665c5dff88..e2ad3c9b2d5c5 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -476,8 +476,15 @@ 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;
+
+	sb->s_defunc = true;
+	if (sb->s_bdi != &noop_backing_dev_info) {
+		bdi_put(sb->s_bdi);
+		sb->s_bdi = &noop_backing_dev_info;
+	}
+	fuse_abort_conn(fc);
 }
 
 static void fuse_send_destroy(struct fuse_mount *fm)
-- 
2.31.0

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

[Miklos Szeredi]

On Wed, 11 May 2022 at 03:31, Daniil Lunev <dlunev@chromium.org> wrote:
>
> Force unmount of fuse severes the connection between FUSE driver and its
> userspace counterpart.

Why is forced umount being used in the first place?

Thanks,
Miklos

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

[Daniil Lunev]

On Wed, May 11, 2022 at 5:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 11 May 2022 at 03:31, Daniil Lunev <dlunev@chromium.org> wrote:
> >
> > Force unmount of fuse severes the connection between FUSE driver and its
> > userspace counterpart.
>
> Why is forced umount being used in the first place?

To correctly suspend-resume. We have been using this force unmount historically
to circumvent the suspend-resume issues which periodically occur with fuse.
We observe FUSE rejecting to remount the device because of the issue this
patchset attempts to address after the resume if there are still open
file handles
holding old super blocks. I am not sure if fuse's interaction with suspend is
something that has been resolved systematically (we are also trying to
figure that
out). Regardless of that, doing force unmount of a mount point is a legitimate
operation, and with FUSE it may leave the system in a state that is returning
errors for other legitimate operations.

Thanks,
Daniil

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

[Miklos Szeredi]

On Wed, 11 May 2022 at 09:36, Daniil Lunev <dlunev@chromium.org> wrote:
>
> On Wed, May 11, 2022 at 5:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 11 May 2022 at 03:31, Daniil Lunev <dlunev@chromium.org> wrote:
> > >
> > > Force unmount of fuse severes the connection between FUSE driver and its
> > > userspace counterpart.
> >
> > Why is forced umount being used in the first place?
>
> To correctly suspend-resume. We have been using this force unmount historically
> to circumvent the suspend-resume issues which periodically occur with fuse.
> We observe FUSE rejecting to remount the device because of the issue this
> patchset attempts to address after the resume if there are still open
> file handles
> holding old super blocks. I am not sure if fuse's interaction with suspend is
> something that has been resolved systematically (we are also trying to
> figure that
> out).

No progress has been made in the past decade with regard to suspend.
I mainly put that down to lack of interest.

>  Regardless of that, doing force unmount of a mount point is a legitimate
> operation, and with FUSE it may leave the system in a state that is returning
> errors for other legitimate operations.

It is a legitimate operation, but one that is not guaranteed to leave
the system in a clean state.

Thanks,
Miklos

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

[Daniil Lunev]

> No progress has been made in the past decade with regard to suspend.
> I mainly put that down to lack of interest.
>
That is unfortunate.

> It is a legitimate operation, but one that is not guaranteed to leave
> the system in a clean state.
Sure, I don't think I can argue about it. The current behaviour is a problem,
however, since there is no other way to ensure the system can suspend
reliably but force unmount - we try normal unmount first and proceed with
force if that fails. Do you think that the approach proposed in this patchset
is a reasonable path to mitigate the issue?

Thanks,
Daniil

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

[Miklos Szeredi]

On Wed, 11 May 2022 at 11:37, Daniil Lunev <dlunev@chromium.org> wrote:
>
> > No progress has been made in the past decade with regard to suspend.
> > I mainly put that down to lack of interest.
> >
> That is unfortunate.
>
> > It is a legitimate operation, but one that is not guaranteed to leave
> > the system in a clean state.
> Sure, I don't think I can argue about it. The current behaviour is a problem,
> however, since there is no other way to ensure the system can suspend
> reliably but force unmount - we try normal unmount first and proceed with
> force if that fails. Do you think that the approach proposed in this patchset
> is a reasonable path to mitigate the issue?

At a glance it's a gross hack.   I can think of more than one way in
which this could be achieved without adding a new field to struct
super_block.

But...  what I'd really prefer is if the underlying issue of fuse vs.
suspend was properly addressed instead of adding band-aids.  And that
takes lots more resources, for sure, and the result is not guaranteed.
But you could at least give it a try.

Thanks,
Miklos

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

[Daniil Lunev]

> At a glance it's a gross hack.   I can think of more than one way in
> which this could be achieved without adding a new field to struct
> super_block.
Can you advise what would be a better way to achieve that?

> But...  what I'd really prefer is if the underlying issue of fuse vs.
> suspend was properly addressed instead of adding band-aids.  And that
> takes lots more resources, for sure, and the result is not guaranteed.
> But you could at least give it a try.
We do have a limited success with userspace level sequencing of processes,
but on the kernel level - it is all quite untrivial, as you mentioned.
I did some
research, and what I found pretty much a 9 years old thread which went
nowhere at the end [1]. We would also prefer if suspend just worked (and
we have a person looking into what is actually breaking with suspend), but
there is an unbounded amount of time for how long the investigation and
search for a solution may be ongoing given the complexity of the problem,
and in the meantime there is no way to work around the problem.

Thanks,
Daniil

[1] https://linux-kernel.vger.kernel.narkive.com/UeBWfN1V/patch-fuse-make-fuse-daemon-frozen-along-with-kernel-threads

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

[Bernd Schubert]

On 5/11/22 13:19, Daniil Lunev wrote:
>> At a glance it's a gross hack.   I can think of more than one way in
>> which this could be achieved without adding a new field to struct
>> super_block.
> Can you advise what would be a better way to achieve that?
> 
>> But...  what I'd really prefer is if the underlying issue of fuse vs.
>> suspend was properly addressed instead of adding band-aids.  And that
>> takes lots more resources, for sure, and the result is not guaranteed.
>> But you could at least give it a try.
> We do have a limited success with userspace level sequencing of processes,
> but on the kernel level - it is all quite untrivial, as you mentioned.
> I did some
> research, and what I found pretty much a 9 years old thread which went
> nowhere at the end [1]. We would also prefer if suspend just worked (and
> we have a person looking into what is actually breaking with suspend), but
> there is an unbounded amount of time for how long the investigation and
> search for a solution may be ongoing given the complexity of the problem,
> and in the meantime there is no way to work around the problem.
> 
> Thanks,
> Daniil
> 
> [1] https://linux-kernel.vger.kernel.narkive.com/UeBWfN1V/patch-fuse-make-fuse-daemon-frozen-along-with-kernel-threads

So that sounds like anything that is waiting for a response cannot be 
frozen? Assuming there is an outstanding NFS request and the NFS server 
is down, suspend would not work until the NFS server comes back?

Thanks,
Bernd

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

[Miklos Szeredi]

On Wed, 11 May 2022 at 13:19, Daniil Lunev <dlunev@chromium.org> wrote:
>
> > At a glance it's a gross hack.   I can think of more than one way in
> > which this could be achieved without adding a new field to struct
> > super_block.
> Can you advise what would be a better way to achieve that?

I think it would be easiest to remove the super block from the
type->fs_supers list.

Thanks,
Miklos

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

[Daniil Lunev]

> I think it would be easiest to remove the super block from the
> type->fs_supers list.
I will try tomorrow and upload an updated patchset.
Thanks,
Daniil.