Re: [PATCH] fscrypt: fix lockdep warning

[Yu Zhao]

On Tue, Oct 4, 2022 at 7:39 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> 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.

 Thanks. So the jbd2_handle -> fscrypt_init_mutex dependency is bad.

> 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?

I have no idea how to move fscrypt_get_encryption_info() out of jbd2
transactions. I'll just leave it to you.

> 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