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

[PATCH v4 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.

Changes in v4:
- Simplify condition according to Christoph Hellwig's comments.

Changes in v3:
- Back to state tracking from v1
- Use s_iflag to mark superblocked ignored
- Only unregister private bdi in retire, without freeing

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         | 28 ++++++++++++++++++++++++++--
 include/linux/fs.h |  2 ++
 3 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.31.0

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

[Daniil Lunev]

From: Daniil Lunev <dlunev@chromium.org>

The function is to be called from filesystem-specific code to mark a
superblock to be ignored by superblock test and thus never re-used. The
function also unregisters bdi if the bdi is per-superblock to avoid
collision if a new superblock is created to represent the filesystem.
generic_shutdown_super() skips unregistering bdi for a retired
superlock as it assumes retire function has already done it.

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

Changes in v4:
- Simplify condition according to Christoph Hellwig's comments.

Changes in v3:
- Back to state tracking from v1
- Use s_iflag to mark superblocked ignored
- Only unregister private bdi in retire, without freeing

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

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

diff --git a/fs/super.c b/fs/super.c
index f1d4a193602d6..3fb9fc8d61160 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -422,6 +422,30 @@ bool trylock_super(struct super_block *sb)
 	return false;
 }
 
+/**
+ *	retire_super	-	prevernts superblock from being reused
+ *	@sb: superblock to retire
+ *
+ *	The function marks superblock to be ignored in superblock test, which
+ *	prevents it from being reused for any new mounts. If the superblock has
+ *	a private bdi, it also unregisters it, but doesn't reduce the refcount
+ *	of the superblock to prevent potential races. The refcount is reduced
+ *	by generic_shutdown_super(). The function can not be called concurrently
+ *	with generic_shutdown_super(). It is safe to call the function multiple
+ *	times, subsequent calls have no effect.
+ */
+void retire_super(struct super_block *sb)
+{
+	down_write(&sb->s_umount);
+	if (sb->s_iflags & SB_I_PERSB_BDI) {
+		bdi_unregister(sb->s_bdi);
+		sb->s_iflags &= ~SB_I_PERSB_BDI;
+	}
+	sb->s_iflags |= SB_I_RETIRED;
+	up_write(&sb->s_umount);
+}
+EXPORT_SYMBOL(retire_super);
+
 /**
  *	generic_shutdown_super	-	common helper for ->kill_sb()
  *	@sb: superblock to kill
@@ -1216,7 +1240,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_iflags & SB_I_RETIRED) && s->s_bdev == fc->sget_key;
 }
 
 /**
@@ -1307,7 +1331,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_iflags & SB_I_RETIRED) && (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..ef392fd2361bd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1411,6 +1411,7 @@ extern int send_sigurg(struct fown_struct *fown);
 #define SB_I_SKIP_SYNC	0x00000100	/* Skip superblock at global sync */
 #define SB_I_PERSB_BDI	0x00000200	/* has a per-sb bdi */
 #define SB_I_TS_EXPIRY_WARNED 0x00000400 /* warned about timestamp range expiry */
+#define SB_I_RETIRED	0x00000800	/* superblock shouldn't be reused */
 
 /* Possible states of 'frozen' field */
 enum {
@@ -2424,6 +2425,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

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Miklos Szeredi]

On Wed, 1 Jun 2022 at 03:11, Daniil Lunev <dlunev@chromium.org> wrote:
>
> From: Daniil Lunev <dlunev@chromium.org>
>
> The function is to be called from filesystem-specific code to mark a
> superblock to be ignored by superblock test and thus never re-used. The
> function also unregisters bdi if the bdi is per-superblock to avoid
> collision if a new superblock is created to represent the filesystem.
> generic_shutdown_super() skips unregistering bdi for a retired
> superlock as it assumes retire function has already done it.
>
> Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> Signed-off-by: Daniil Lunev <dlunev@google.com>
> ---
>
> Changes in v4:
> - Simplify condition according to Christoph Hellwig's comments.
>
> Changes in v3:
> - Back to state tracking from v1
> - Use s_iflag to mark superblocked ignored
> - Only unregister private bdi in retire, without freeing
>
> Changes in v2:
> - Remove super from list of superblocks instead of using a flag
>
>  fs/super.c         | 28 ++++++++++++++++++++++++++--
>  include/linux/fs.h |  2 ++
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index f1d4a193602d6..3fb9fc8d61160 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -422,6 +422,30 @@ bool trylock_super(struct super_block *sb)
>         return false;
>  }
>
> +/**
> + *     retire_super    -       prevernts superblock from being reused

s/prevernts/prevents/

> + *     @sb: superblock to retire
> + *
> + *     The function marks superblock to be ignored in superblock test, which
> + *     prevents it from being reused for any new mounts.

This works for block supers and nothing else, at least as this patch
stands.  That might be okay, but should at least be documented.

Thanks,
Miklos

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

[Daniil Lunev]

Hi Miklos,
Thanks for your response and apologies for my delayed reply.  Do I understand
correctly that to cover non-block devices I would need to add the same check
to test_keyed_super and to test_single_super? Am I missing any other place?
--Daniil

On Mon, Jul 18, 2022 at 7:51 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 1 Jun 2022 at 03:11, Daniil Lunev <dlunev@chromium.org> wrote:
> >
> > From: Daniil Lunev <dlunev@chromium.org>
> >
> > The function is to be called from filesystem-specific code to mark a
> > superblock to be ignored by superblock test and thus never re-used. The
> > function also unregisters bdi if the bdi is per-superblock to avoid
> > collision if a new superblock is created to represent the filesystem.
> > generic_shutdown_super() skips unregistering bdi for a retired
> > superlock as it assumes retire function has already done it.
> >
> > Signed-off-by: Daniil Lunev <dlunev@chromium.org>
> > Signed-off-by: Daniil Lunev <dlunev@google.com>
> > ---
> >
> > Changes in v4:
> > - Simplify condition according to Christoph Hellwig's comments.
> >
> > Changes in v3:
> > - Back to state tracking from v1
> > - Use s_iflag to mark superblocked ignored
> > - Only unregister private bdi in retire, without freeing
> >
> > Changes in v2:
> > - Remove super from list of superblocks instead of using a flag
> >
> >  fs/super.c         | 28 ++++++++++++++++++++++++++--
> >  include/linux/fs.h |  2 ++
> >  2 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/super.c b/fs/super.c
> > index f1d4a193602d6..3fb9fc8d61160 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -422,6 +422,30 @@ bool trylock_super(struct super_block *sb)
> >         return false;
> >  }
> >
> > +/**
> > + *     retire_super    -       prevernts superblock from being reused
>
> s/prevernts/prevents/
>
> > + *     @sb: superblock to retire
> > + *
> > + *     The function marks superblock to be ignored in superblock test, which
> > + *     prevents it from being reused for any new mounts.
>
> This works for block supers and nothing else, at least as this patch
> stands.  That might be okay, but should at least be documented.
>
> Thanks,
> Miklos

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

[Daniil Lunev]

From: Daniil Lunev <dlunev@chromium.org>

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>

Signed-off-by: Daniil Lunev <dlunev@google.com>
---

(no changes since v3)

Changes in v3:
- No changes

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 v4 2/2] FUSE: Retire superblock on force unmount

[Daniil Lunev]

Hi Miklos,
Can you please take a look at the current patchset and see if you are
ok with it?
Thanks,
Daniil

On Wed, Jun 1, 2022 at 11:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
>
> From: Daniil Lunev <dlunev@chromium.org>
>
> 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>
>
> Signed-off-by: Daniil Lunev <dlunev@google.com>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - No changes
>
> 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 v4 2/2] FUSE: Retire superblock on force unmount

[Miklos Szeredi]

On Wed, 1 Jun 2022 at 03:11, Daniil Lunev <dlunev@chromium.org> wrote:
>
> From: Daniil Lunev <dlunev@chromium.org>
>
> 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>
>
> Signed-off-by: Daniil Lunev <dlunev@google.com>

Why the double sign-off?

> ---
>
> (no changes since v3)
>
> Changes in v3:
> - No changes
>
> 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);

And this is called for both block and non-block supers.  Which means
that the bdi will be unregistered, yet the sb could still be reused
(see fuse_test_super()).

Thanks,
Miklos

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

[Daniil Lunev]

Hi Miklos,
Thanks for your response and apologies for my delayed reply.

> Why the double sign-off?
Some misconfiguration on my side. I will remove the extra line in the
next patch version

> And this is called for both block and non-block supers.  Which means
> that the bdi will be unregistered, yet the sb could still be reused
> (see fuse_test_super()).

Just to confirm my understanding, fuse_test_super needs to have the
same check as the super.c test_* function, correct?
--Daniil

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

[Miklos Szeredi]

On Fri, 22 Jul 2022 at 02:50, Daniil Lunev <dlunev@chromium.org> wrote:
>
> Hi Miklos,
> Thanks for your response and apologies for my delayed reply.
>
> > Why the double sign-off?
> Some misconfiguration on my side. I will remove the extra line in the
> next patch version
>
> > And this is called for both block and non-block supers.  Which means
> > that the bdi will be unregistered, yet the sb could still be reused
> > (see fuse_test_super()).
>
> Just to confirm my understanding, fuse_test_super needs to have the
> same check as the super.c test_* function, correct?

Or make calling retire_super() conditional on sb->s_bdev != NULL.

Please only enable this for non-bdev fuse (which is the vast majority
of cases) if it's justified.  Otherwise it will just be a source of
bugs.

Thanks,
Miklos

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

[Daniil Lunev]

Hi Miklos and Alexander,
Do you have any more concerns or comments regarding the patchset or do
you think we can proceed with it?
--Daniil

On Wed, Jun 1, 2022 at 11:11 AM Daniil Lunev <dlunev@chromium.org> wrote:
>
> 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.
>
> Changes in v4:
> - Simplify condition according to Christoph Hellwig's comments.
>
> Changes in v3:
> - Back to state tracking from v1
> - Use s_iflag to mark superblocked ignored
> - Only unregister private bdi in retire, without freeing
>
> 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         | 28 ++++++++++++++++++++++++++--
>  include/linux/fs.h |  2 ++
>  3 files changed, 33 insertions(+), 4 deletions(-)
>
> --
> 2.31.0
>