| Message ID | 20221005001326.157644-1-yuzhao@google.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | fscrypt: fix lockdep warning | expand |
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. 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@syzkaller.appspotmail.com", as there's a syzbot report open already (https://lore.kernel.org/all/00000000000070395e05dd1fb4d7@google.com/T/#u). - Eric
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
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;
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(+)