[PATCH 0/2] Small follow-up fixes for this cycle

[PATCH 0/2] Small follow-up fixes for this cycle

[Christian Brauner]

Hey Jan & Christoph,

Two small follow-up fixes for two brainos.
I plan on getting this merged tomorrow if there aren't any objections.

Christian

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (2):
      super: move lockdep assert
      super: ensure valid info

 fs/super.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)
---
base-commit: cd4284cfd3e11c7a49e4808f76f53284d47d04dd
change-id: 20230828-vfs-super-fixes-147bf51f7b43

[PATCH 1/2] super: move lockdep assert

[Christian Brauner]

Fix braino and move the lockdep assertion after put_super() otherwise we
risk a use-after-free.

Fixes: 2c18a63b760a ("super: wait until we passed kill super")
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/super.c b/fs/super.c
index ef87103e2a51..779247eb219c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -570,8 +570,8 @@ static bool grab_super_dead(struct super_block *sb)
 		return true;
 	}
 	wait_var_event(&sb->s_flags, wait_dead(sb));
-	put_super(sb);
 	lockdep_assert_not_held(&sb->s_umount);
+	put_super(sb);
 	return false;
 }
 

-- 
2.34.1

Re: [PATCH 1/2] super: move lockdep assert

[Jan Kara]

On Mon 28-08-23 13:26:23, Christian Brauner wrote:
> Fix braino and move the lockdep assertion after put_super() otherwise we
> risk a use-after-free.
> 
> Fixes: 2c18a63b760a ("super: wait until we passed kill super")
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Yeah, pretty obvious. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index ef87103e2a51..779247eb219c 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -570,8 +570,8 @@ static bool grab_super_dead(struct super_block *sb)
>  		return true;
>  	}
>  	wait_var_event(&sb->s_flags, wait_dead(sb));
> -	put_super(sb);
>  	lockdep_assert_not_held(&sb->s_umount);
> +	put_super(sb);
>  	return false;
>  }
>  
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] super: move lockdep assert

[Christoph Hellwig]

Looks good:

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

[PATCH 2/2] super: ensure valid info

[Christian Brauner]

For keyed filesystems that recycle superblocks based on s_fs_info or
information contained therein s_fs_info must be kept as long as the
superblock is on the filesystem type super list. This isn't guaranteed
as s_fs_info will be freed latest in sb->kill_sb().

The fix is simply to perform notification and list removal in
kill_anon_super(). Any filesystem needs to free s_fs_info after they
call the kill_*() helpers. If they don't they risk use-after-free right
now so fixing it here is guaranteed that s_fs_info remain valid.

For block backed filesystems notifying in pass sb->kill_sb() in
deactivate_locked_super() remains unproblematic and is required because
multiple other block devices can be shut down after kill_block_super()
has been called from a filesystem's sb->kill_sb() handler. For example,
ext4 and xfs close additional devices. Block based filesystems don't
depend on s_fs_info (btrfs does use s_fs_info but also uses
kill_anon_super() and not kill_block_super().).

Sorry for that braino. Goal should be to unify this behavior during this
cycle obviously. But let's please do a simple bugfix now.

Fixes: 2c18a63b760a ("super: wait until we passed kill super")
Fixes: syzbot+5b64180f8d9e39d3f061@syzkaller.appspotmail.com
Reported-by: syzbot+5b64180f8d9e39d3f061@syzkaller.appspotmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 779247eb219c..ad7ac3a24d38 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -434,6 +434,33 @@ void put_super(struct super_block *sb)
 	spin_unlock(&sb_lock);
 }
 
+static void kill_super_notify(struct super_block *sb)
+{
+	lockdep_assert_not_held(&sb->s_umount);
+
+	/* already notified earlier */
+	if (sb->s_flags & SB_DEAD)
+		return;
+
+	/*
+	 * Remove it from @fs_supers so it isn't found by new
+	 * sget{_fc}() walkers anymore. Any concurrent mounter still
+	 * managing to grab a temporary reference is guaranteed to
+	 * already see SB_DYING and will wait until we notify them about
+	 * SB_DEAD.
+	 */
+	spin_lock(&sb_lock);
+	hlist_del_init(&sb->s_instances);
+	spin_unlock(&sb_lock);
+
+	/*
+	 * Let concurrent mounts know that this thing is really dead.
+	 * We don't need @sb->s_umount here as every concurrent caller
+	 * will see SB_DYING and either discard the superblock or wait
+	 * for SB_DEAD.
+	 */
+	super_wake(sb, SB_DEAD);
+}
 
 /**
  *	deactivate_locked_super	-	drop an active reference to superblock
@@ -453,6 +480,8 @@ void deactivate_locked_super(struct super_block *s)
 		unregister_shrinker(&s->s_shrink);
 		fs->kill_sb(s);
 
+		kill_super_notify(s);
+
 		/*
 		 * Since list_lru_destroy() may sleep, we cannot call it from
 		 * put_super(), where we hold the sb_lock. Therefore we destroy
@@ -461,25 +490,6 @@ void deactivate_locked_super(struct super_block *s)
 		list_lru_destroy(&s->s_dentry_lru);
 		list_lru_destroy(&s->s_inode_lru);
 
-		/*
-		 * Remove it from @fs_supers so it isn't found by new
-		 * sget{_fc}() walkers anymore. Any concurrent mounter still
-		 * managing to grab a temporary reference is guaranteed to
-		 * already see SB_DYING and will wait until we notify them about
-		 * SB_DEAD.
-		 */
-		spin_lock(&sb_lock);
-		hlist_del_init(&s->s_instances);
-		spin_unlock(&sb_lock);
-
-		/*
-		 * Let concurrent mounts know that this thing is really dead.
-		 * We don't need @sb->s_umount here as every concurrent caller
-		 * will see SB_DYING and either discard the superblock or wait
-		 * for SB_DEAD.
-		 */
-		super_wake(s, SB_DEAD);
-
 		put_filesystem(fs);
 		put_super(s);
 	} else {
@@ -1260,6 +1270,7 @@ void kill_anon_super(struct super_block *sb)
 {
 	dev_t dev = sb->s_dev;
 	generic_shutdown_super(sb);
+	kill_super_notify(sb);
 	free_anon_bdev(dev);
 }
 EXPORT_SYMBOL(kill_anon_super);

-- 
2.34.1

Re: [PATCH 2/2] super: ensure valid info

[Christoph Hellwig]

> For block backed filesystems notifying in pass sb->kill_sb() in

I can't parse the 'in pass' here.

> @@ -1260,6 +1270,7 @@ void kill_anon_super(struct super_block *sb)
>  {
>  	dev_t dev = sb->s_dev;
>  	generic_shutdown_super(sb);
> +	kill_super_notify(sb);
>  	free_anon_bdev(dev);

Maybe I didn't read the commit log carefully enough, but why do we
need to call kill_super_notify before free_anon_bdev and any potential
action in ->kill_sb after calling kill_anon_super here given that
we already add a call to kill_super_notify after ->kill_sb?

Re: [PATCH 2/2] super: ensure valid info

[Christian Brauner]

> Maybe I didn't read the commit log carefully enough, but why do we
> need to call kill_super_notify before free_anon_bdev and any potential
> action in ->kill_sb after calling kill_anon_super here given that
> we already add a call to kill_super_notify after ->kill_sb?

Yeah, the commit log explains this. We leave the superblock on fs_supers
past sb->kill_sb() and notify after device closure. For block based
filesystems that's the correct thing. They don't rely on sb->s_fs_info
and we need to ensure that all devices are closed.

But for filesystems like kernfs that rely on get_keyed_super() they rely
on sb->s_fs_info to recycle sbs. sb->s_fs_info is currently always freed
in sb->kill_sb()

kernfs_kill_sb()
-> kill_anon_super()
   -> kfree(info)

For such fses sb->s_fs_info is freed with the superblock still on
fs_supers which means we get a UAF when the sb is still found on the
list. So for such filesystems we need to remove and notify before
sb->s_fs_info is freed. That's done in kill_anon_super(). For such
filesystems the call in deactivate_locked_super() is a nop.

Re: [PATCH 2/2] super: ensure valid info

[Christoph Hellwig]

On Mon, Aug 28, 2023 at 02:28:48PM +0200, Christian Brauner wrote:
> > Maybe I didn't read the commit log carefully enough, but why do we
> > need to call kill_super_notify before free_anon_bdev and any potential
> > action in ->kill_sb after calling kill_anon_super here given that
> > we already add a call to kill_super_notify after ->kill_sb?
> 
> Yeah, the commit log explains this. We leave the superblock on fs_supers
> past sb->kill_sb() and notify after device closure. For block based
> filesystems that's the correct thing. They don't rely on sb->s_fs_info
> and we need to ensure that all devices are closed.
> 
> But for filesystems like kernfs that rely on get_keyed_super() they rely
> on sb->s_fs_info to recycle sbs. sb->s_fs_info is currently always freed
> in sb->kill_sb()
> 
> kernfs_kill_sb()
> -> kill_anon_super()
>    -> kfree(info)
> 
> For such fses sb->s_fs_info is freed with the superblock still on
> fs_supers which means we get a UAF when the sb is still found on the
> list. So for such filesystems we need to remove and notify before
> sb->s_fs_info is freed. That's done in kill_anon_super(). For such
> filesystems the call in deactivate_locked_super() is a nop.

Ok, so I did fail to parse the commit log.

Looks good:

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

Re: [PATCH 2/2] super: ensure valid info

[Jan Kara]

On Mon 28-08-23 13:26:24, Christian Brauner wrote:
> For keyed filesystems that recycle superblocks based on s_fs_info or
> information contained therein s_fs_info must be kept as long as the
> superblock is on the filesystem type super list. This isn't guaranteed
> as s_fs_info will be freed latest in sb->kill_sb().
> 
> The fix is simply to perform notification and list removal in
> kill_anon_super(). Any filesystem needs to free s_fs_info after they
> call the kill_*() helpers. If they don't they risk use-after-free right
> now so fixing it here is guaranteed that s_fs_info remain valid.
> 
> For block backed filesystems notifying in pass sb->kill_sb() in
> deactivate_locked_super() remains unproblematic and is required because
> multiple other block devices can be shut down after kill_block_super()
> has been called from a filesystem's sb->kill_sb() handler. For example,
> ext4 and xfs close additional devices. Block based filesystems don't
> depend on s_fs_info (btrfs does use s_fs_info but also uses
> kill_anon_super() and not kill_block_super().).
> 
> Sorry for that braino. Goal should be to unify this behavior during this
> cycle obviously. But let's please do a simple bugfix now.
> 
> Fixes: 2c18a63b760a ("super: wait until we passed kill super")
> Fixes: syzbot+5b64180f8d9e39d3f061@syzkaller.appspotmail.com
> Reported-by: syzbot+5b64180f8d9e39d3f061@syzkaller.appspotmail.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>

So AFAICT this fixes the UAF issues. It does reintroduce the EBUSY problems
with mount racing with umount e.g. for EROFS which calls bdev_release()
after calling kill_anon_super() but I think we can live with that until we
come up with a neater solution for this problem. So feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c | 49 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 779247eb219c..ad7ac3a24d38 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -434,6 +434,33 @@ void put_super(struct super_block *sb)
>  	spin_unlock(&sb_lock);
>  }
>  
> +static void kill_super_notify(struct super_block *sb)
> +{
> +	lockdep_assert_not_held(&sb->s_umount);
> +
> +	/* already notified earlier */
> +	if (sb->s_flags & SB_DEAD)
> +		return;
> +
> +	/*
> +	 * Remove it from @fs_supers so it isn't found by new
> +	 * sget{_fc}() walkers anymore. Any concurrent mounter still
> +	 * managing to grab a temporary reference is guaranteed to
> +	 * already see SB_DYING and will wait until we notify them about
> +	 * SB_DEAD.
> +	 */
> +	spin_lock(&sb_lock);
> +	hlist_del_init(&sb->s_instances);
> +	spin_unlock(&sb_lock);
> +
> +	/*
> +	 * Let concurrent mounts know that this thing is really dead.
> +	 * We don't need @sb->s_umount here as every concurrent caller
> +	 * will see SB_DYING and either discard the superblock or wait
> +	 * for SB_DEAD.
> +	 */
> +	super_wake(sb, SB_DEAD);
> +}
>  
>  /**
>   *	deactivate_locked_super	-	drop an active reference to superblock
> @@ -453,6 +480,8 @@ void deactivate_locked_super(struct super_block *s)
>  		unregister_shrinker(&s->s_shrink);
>  		fs->kill_sb(s);
>  
> +		kill_super_notify(s);
> +
>  		/*
>  		 * Since list_lru_destroy() may sleep, we cannot call it from
>  		 * put_super(), where we hold the sb_lock. Therefore we destroy
> @@ -461,25 +490,6 @@ void deactivate_locked_super(struct super_block *s)
>  		list_lru_destroy(&s->s_dentry_lru);
>  		list_lru_destroy(&s->s_inode_lru);
>  
> -		/*
> -		 * Remove it from @fs_supers so it isn't found by new
> -		 * sget{_fc}() walkers anymore. Any concurrent mounter still
> -		 * managing to grab a temporary reference is guaranteed to
> -		 * already see SB_DYING and will wait until we notify them about
> -		 * SB_DEAD.
> -		 */
> -		spin_lock(&sb_lock);
> -		hlist_del_init(&s->s_instances);
> -		spin_unlock(&sb_lock);
> -
> -		/*
> -		 * Let concurrent mounts know that this thing is really dead.
> -		 * We don't need @sb->s_umount here as every concurrent caller
> -		 * will see SB_DYING and either discard the superblock or wait
> -		 * for SB_DEAD.
> -		 */
> -		super_wake(s, SB_DEAD);
> -
>  		put_filesystem(fs);
>  		put_super(s);
>  	} else {
> @@ -1260,6 +1270,7 @@ void kill_anon_super(struct super_block *sb)
>  {
>  	dev_t dev = sb->s_dev;
>  	generic_shutdown_super(sb);
> +	kill_super_notify(sb);
>  	free_anon_bdev(dev);
>  }
>  EXPORT_SYMBOL(kill_anon_super);
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] super: ensure valid info

[Christian Brauner]

> So AFAICT this fixes the UAF issues. It does reintroduce the EBUSY problems
> with mount racing with umount e.g. for EROFS which calls bdev_release()

It shouldn't as erofs doesn't use block devices when it is in fscache
mode which is when kill_anon_super() is called. For regular erofs
kill_block_super() is used and notification happens as before.

> after calling kill_anon_super() but I think we can live with that until we
> come up with a neater solution for this problem. So feel free to add:

I think ultimately we might just provide callback to kill_*_super() like
we do for sget_fc() or something. But let's keep thinking of course.

Re: [PATCH 0/2] Small follow-up fixes for this cycle

[Christian Brauner]

On Mon, 28 Aug 2023 13:26:22 +0200, Christian Brauner wrote:
> Hey Jan & Christoph,
> 
> Two small follow-up fixes for two brainos.
> I plan on getting this merged tomorrow if there aren't any objections.
> 
> Christian
> 
> [...]

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/2] super: move lockdep assert
      https://git.kernel.org/vfs/vfs/c/345a5c4a0b63
[2/2] super: ensure valid info
      https://git.kernel.org/vfs/vfs/c/dc3216b14160