Re: [PATCH v4 01/46] fs: move fscrypt keyring destruction to after ->put_super

Eric Biggers
2023-12-05T01:58:02Z

On Fri, Dec 01, 2023 at 05:10:58PM -0500, Josef Bacik wrote:
> 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>
> ---
> fs/super.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 076392396e72..faf7d248145d 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -681,12 +681,6 @@ void generic_shutdown_super(struct super_block *sb)
> 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;
> @@ -695,6 +689,12 @@ void generic_shutdown_super(struct super_block *sb)
> 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);
> +

This patch will cause a NULL dereference on f2fs, since f2fs_put_super() frees
->s_fs_info, and then fscrypt_destroy_keyring() can call f2fs_get_devices() (via
fscrypt_operations::get_devices) which dereferences it. (ext4 also frees
->s_fs_info in its ->put_super, but ext4 doesn't implement ->get_devices.)

I think we need to move the fscrypt keyring destruction into ->put_super for
each filesystem.

- Eric