Re: [PATCH] fscrypt: fix lockdep warning

[Eric Biggers]

On Tue, Oct 04, 2022 at 06:13:26PM -0600, Yu Zhao wrote:
> fscrypt_initialize() shouldn't allocate memory without GFP_NOFS.
> 
> The problem seems to go back to 2015
> commit 57e5055b0a5e ("f2fs crypto: add f2fs encryption facilities")
> but I have never heard of any complaints, hence not CC'ing stable.
> 
>   ======================================================
>   WARNING: possible circular locking dependency detected
>   6.0.0-lockdep #1 Not tainted
>   ------------------------------------------------------
>   kswapd0/77 is trying to acquire lock:
>   71ffff808b254a18 (jbd2_handle){++++}-{0:0}, at: start_this_handle+0x76c/0x8dc
> 
>   but task is already holding lock:
>   ffffffea26533310 (fs_reclaim){+.+.}-{0:0}, at: 0x1
> 
>   which lock already depends on the new lock.
> 
>   <snipped>
> 
>   other info that might help us debug this:
> 
>   Chain exists of:
>     jbd2_handle --> fscrypt_init_mutex --> fs_reclaim
> 
>    Possible unsafe locking scenario:
> 
>          CPU0                    CPU1
>          ----                    ----
>     lock(fs_reclaim);
>                                  lock(fscrypt_init_mutex);
>                                  lock(fs_reclaim);
>     lock(jbd2_handle);
> 
>    *** DEADLOCK ***
> 
>   3 locks held by kswapd0/77:
>    #0: ffffffea26533310 (fs_reclaim){+.+.}-{0:0}, at: 0x1
>    #1: ffffffea26529220 (shrinker_rwsem){++++}-{3:3}, at: shrink_slab+0x54/0x464
>    #2: 6dffff808abe90e8 (&type->s_umount_key#47){++++}-{3:3}, at: trylock_super+0x2c/0x8c
> 
>   <snipped>
> 
> Signed-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
> ---
>  fs/crypto/crypto.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index e78be66bbf01..e10fc30142a6 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -316,6 +316,7 @@ EXPORT_SYMBOL(fscrypt_decrypt_block_inplace);
>  int fscrypt_initialize(unsigned int cop_flags)
>  {
>  	int err = 0;
> +	unsigned int flags;
>  
>  	/* No need to allocate a bounce page pool if this FS won't use it. */
>  	if (cop_flags & FS_CFLG_OWN_PAGES)
> @@ -326,8 +327,10 @@ int fscrypt_initialize(unsigned int cop_flags)
>  		goto out_unlock;
>  
>  	err = -ENOMEM;
> +	flags = memalloc_nofs_save();
>  	fscrypt_bounce_page_pool =
>  		mempool_create_page_pool(num_prealloc_crypto_pages, 0);
> +	memalloc_nofs_restore(flags);
>  	if (!fscrypt_bounce_page_pool)
>  		goto out_unlock;

Thanks, but this isn't the correct fix.  The real problem is that ext4 is
calling fscrypt_get_encryption_info() from within a jbd2 transaction, which is
fundamentally unsafe.  It's a known regression from commit a80f7fcf1867
("ext4: fixup ext4_fc_track_* functions' signature"), which extended the scope
of the transaction in ext4_unlink() too far.

Sorry for not getting around to fixing this earlier.  Are you interested in
sending a patch for it?  If you do, please make sure to include
"Reported-by: syzbot+1a748d0007eeac3ab079@xxxxxxxxxxxxxxxxxxxxxxxxx",
as there's a syzbot report open already
(https://lore.kernel.org/all/00000000000070395e05dd1fb4d7@xxxxxxxxxx/T/#u).

- Eric