From: Yu Zhao <yuzhao@google.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: "Theodore Y . Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fscrypt: fix lockdep warning
Date: Tue, 4 Oct 2022 19:57:48 -0600 [thread overview]
Message-ID: <CAOUHufZM3JefcJNc-fwCJG8hEVmvAx1yWxPGgTJyneTOT93ZwA@mail.gmail.com> (raw)
In-Reply-To: <YzzgSYiCCO8dbms8@sol.localdomain>
On Tue, Oct 4, 2022 at 7:39 PM Eric Biggers <ebiggers@kernel.org> 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@google.com>
> > ---
> > 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@syzkaller.appspotmail.com",
> as there's a syzbot report open already
> (https://lore.kernel.org/all/00000000000070395e05dd1fb4d7@google.com/T/#u).
>
> - Eric
prev parent reply other threads:[~2022-10-05 1:58 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-05 0:13 [PATCH] fscrypt: fix lockdep warning Yu Zhao
2022-10-05 1:39 ` Eric Biggers
2022-10-05 1:57 ` Yu Zhao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOUHufZM3JefcJNc-fwCJG8hEVmvAx1yWxPGgTJyneTOT93ZwA@mail.gmail.com \
--to=yuzhao@google.com \
--cc=ebiggers@kernel.org \
--cc=jaegeuk@kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).