diff mbox series

[3/3] fs: move fscrypt keyring destruction to after ->put_super

Message ID 20231213040018.73803-4-ebiggers@kernel.org (mailing list archive)
State Superseded
Headers show
Series Move fscrypt keyring destruction to after ->put_super | expand

Commit Message

Eric Biggers Dec. 13, 2023, 4 a.m. UTC
From: Josef Bacik <josef@toxicpanda.com>

btrfs has a variety of asynchronous things we do with inodes that can
potentially last until ->put_super, when we shut everything down and
clean up all of our async work.  Due to this we need to move
fscrypt_destroy_keyring() to after ->put_super, otherwise we get
warnings about still having active references on the master key.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Dec. 13, 2023, 4:15 p.m. UTC | #1
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Neal Gompa Dec. 13, 2023, 8:54 p.m. UTC | #2
On Tue, Dec 12, 2023 at 11:01 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Josef Bacik <josef@toxicpanda.com>
>
> btrfs has a variety of asynchronous things we do with inodes that can
> potentially last until ->put_super, when we shut everything down and
> clean up all of our async work.  Due to this we need to move
> fscrypt_destroy_keyring() to after ->put_super, otherwise we get
> warnings about still having active references on the master key.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/super.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 076392396e724..faf7d248145d2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -674,34 +674,34 @@ void generic_shutdown_super(struct super_block *sb)
>                 /* Evict all inodes with zero refcount. */
>                 evict_inodes(sb);
>
>                 /*
>                  * Clean up and evict any inodes that still have references due
>                  * to fsnotify or the security policy.
>                  */
>                 fsnotify_sb_delete(sb);
>                 security_sb_delete(sb);
>
> -               /*
> -                * Now that all potentially-encrypted inodes have been evicted,
> -                * the fscrypt keyring can be destroyed.
> -                */
> -               fscrypt_destroy_keyring(sb);
> -
>                 if (sb->s_dio_done_wq) {
>                         destroy_workqueue(sb->s_dio_done_wq);
>                         sb->s_dio_done_wq = NULL;
>                 }
>
>                 if (sop->put_super)
>                         sop->put_super(sb);
>
> +               /*
> +                * Now that all potentially-encrypted inodes have been evicted,
> +                * the fscrypt keyring can be destroyed.
> +                */
> +               fscrypt_destroy_keyring(sb);
> +
>                 if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
>                                 "VFS: Busy inodes after unmount of %s (%s)",
>                                 sb->s_id, sb->s_type->name)) {
>                         /*
>                          * Adding a proper bailout path here would be hard, but
>                          * we can at least make it more likely that a later
>                          * iput_final() or such crashes cleanly.
>                          */
>                         struct inode *inode;
>
> --
> 2.43.0
>
>

This makes sense to me.

Reviewed-by: Neal Gompa <neal@gompa.dev>



--
真実はいつも一つ!/ Always, there's only one truth!
diff mbox series

Patch

diff --git a/fs/super.c b/fs/super.c
index 076392396e724..faf7d248145d2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -674,34 +674,34 @@  void generic_shutdown_super(struct super_block *sb)
 		/* Evict all inodes with zero refcount. */
 		evict_inodes(sb);
 
 		/*
 		 * Clean up and evict any inodes that still have references due
 		 * to fsnotify or the security policy.
 		 */
 		fsnotify_sb_delete(sb);
 		security_sb_delete(sb);
 
-		/*
-		 * Now that all potentially-encrypted inodes have been evicted,
-		 * the fscrypt keyring can be destroyed.
-		 */
-		fscrypt_destroy_keyring(sb);
-
 		if (sb->s_dio_done_wq) {
 			destroy_workqueue(sb->s_dio_done_wq);
 			sb->s_dio_done_wq = NULL;
 		}
 
 		if (sop->put_super)
 			sop->put_super(sb);
 
+		/*
+		 * Now that all potentially-encrypted inodes have been evicted,
+		 * the fscrypt keyring can be destroyed.
+		 */
+		fscrypt_destroy_keyring(sb);
+
 		if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
 				"VFS: Busy inodes after unmount of %s (%s)",
 				sb->s_id, sb->s_type->name)) {
 			/*
 			 * Adding a proper bailout path here would be hard, but
 			 * we can at least make it more likely that a later
 			 * iput_final() or such crashes cleanly.
 			 */
 			struct inode *inode;