| Message ID | 20240404095000.5872-1-mish.uxin2012@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: fix i_data_sem unlock order in ext4_ind_migrate() | expand |
On Thu, Apr 04, 2024 at 12:50:00PM +0300, Mikhail Ukhin wrote: > Fuzzing reports a possible deadlock in jbd2_log_wait_commit. > > The problem occurs in ext4_ind_migrate due to an incorrect order of > unlocking of the journal and write semaphores - the order of unlocking > must be the reverse of the order of locking. > > Found by Linux Verification Center (linuxtesting.org) with syzkaller. > > Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com> Does not match your From: line :( > --- > fs/ext4/migrate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c > index d98ac2af8199..a5e1492bbaaa 100644 > --- a/fs/ext4/migrate.c > +++ b/fs/ext4/migrate.c > @@ -663,8 +663,8 @@ int ext4_ind_migrate(struct inode *inode) > if (unlikely(ret2 && !ret)) > ret = ret2; > errout: > - ext4_journal_stop(handle); > up_write(&EXT4_I(inode)->i_data_sem); > + ext4_journal_stop(handle); > out_unlock: > ext4_writepages_up_write(inode->i_sb, alloc_ctx); > return ret; > -- > 2.25.1 > > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>
On Thu, Apr 04, 2024 at 12:50:00PM +0300, Mikhail Ukhin wrote: > Fuzzing reports a possible deadlock in jbd2_log_wait_commit. > > The problem occurs in ext4_ind_migrate due to an incorrect order of > unlocking of the journal and write semaphores - the order of unlocking > must be the reverse of the order of locking. > > Found by Linux Verification Center (linuxtesting.org) with syzkaller. This doesn't make any sense. Lock order matters; the order in which you unlock shouldn't (and doesn't) make a difference. This is also something which lockdep doesn't complain about --- because it's not a problem. So how was this "found by syzkaller"? - Ted
On Friday, April 5, 2024 5:26:51 AM MSK Theodore Ts'o wrote: > > This doesn't make any sense. Lock order matters; the order in which > you unlock shouldn't (and doesn't) make a difference. This is also > something which lockdep doesn't complain about --- because it's not a > problem. > > So how was this "found by syzkaller"? > > - Ted This issue only occurs when CONFIG_PROVE_LOCKING is set, in which case jbd2_might_wait_for_commit macro will lock jbd2_handle in jbd2_journal_stop function, which, while i_data_sem is locked, will trigger lockdep, because jbd2_journal_start function might also lock on same jbd2_handle at the same time Without CONFIG_PROVE_LOCKING this issue is not possible because jbd2_journal_stop never calls jbd2_might_wait_for_commit Since syzkaller report was local and wasn't inserted in initial patch message, I will put it in this message It wasn't been able to create a reproducer for that problem, so there's only a crash report ====================================================== WARNING: possible circular locking dependency detected 5.10.208-syzkaller #0 Not tainted ------------------------------------------------------ syz-fuzzer/1080 is trying to acquire lock: ffff88810b09e8e0 (jbd2_handle){++++}-{0:0}, at: jbd2_log_wait_commit+0x142/0x430 fs/jbd2/journal.c:693 but task is already holding lock: ffff88805eec5c78 (&ei->i_data_sem){++++}-{3:3}, at: ext4_ind_migrate+0x2fe/0x840 fs/ext4/migrate.c:633 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&ei->i_data_sem){++++}-{3:3}: down_read+0x96/0x420 kernel/locking/rwsem.c:1504 ext4_da_map_blocks fs/ext4/inode.c:1776 [inline] ext4_da_get_block_prep+0x5b4/0x11a0 fs/ext4/inode.c:1857 ext4_block_write_begin+0x479/0x1230 fs/ext4/inode.c:1076 ext4_da_write_begin+0x3ca/0x1060 fs/ext4/inode.c:3063 generic_perform_write+0x210/0x500 mm/filemap.c:3336 ext4_buffered_write_iter+0x232/0x4a0 fs/ext4/file.c:269 ext4_file_write_iter+0x429/0x1420 fs/ext4/file.c:683 call_write_iter include/linux/fs.h:1962 [inline] new_sync_write+0x427/0x660 fs/read_write.c:518 vfs_write+0x774/0xab0 fs/read_write.c:605 ksys_write+0x12d/0x260 fs/read_write.c:658 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x62/0xc7 -> #0 (jbd2_handle){++++}-{0:0}: check_prev_add kernel/locking/lockdep.c:2988 [inline] check_prevs_add kernel/locking/lockdep.c:3113 [inline] validate_chain kernel/locking/lockdep.c:3729 [inline] __lock_acquire+0x29c4/0x53c0 kernel/locking/lockdep.c:4955 lock_acquire kernel/locking/lockdep.c:5566 [inline] lock_acquire+0x19f/0x4a0 kernel/locking/lockdep.c:5531 jbd2_log_wait_commit+0x177/0x430 fs/jbd2/journal.c:696 jbd2_journal_stop+0x5b0/0xde0 fs/jbd2/transaction.c:1933 __ext4_journal_stop+0xde/0x1f0 fs/ext4/ext4_jbd2.c:127 ext4_ind_migrate+0x402/0x840 fs/ext4/migrate.c:666 ext4_ioctl_setflags+0xaef/0xc70 fs/ext4/ioctl.c:459 __ext4_ioctl+0x3742/0x4e20 fs/ext4/ioctl.c:870 vfs_ioctl fs/ioctl.c:48 [inline] __do_sys_ioctl fs/ioctl.c:753 [inline] __se_sys_ioctl fs/ioctl.c:739 [inline] __x64_sys_ioctl+0x19a/0x210 fs/ioctl.c:739 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x62/0xc7 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&ei->i_data_sem); lock(jbd2_handle); lock(&ei->i_data_sem); lock(jbd2_handle); *** DEADLOCK *** 4 locks held by syz-fuzzer/1080: #0: ffff88810bc42460 (sb_writers#4){.+.+}-{0:0}, at: __ext4_ioctl+0x76a/0x4e20 fs/ext4/ioctl.c:861 #1: ffff88805eec5e88 (&type->i_mutex_dir_key#3){++++}-{3:3}, at: inode_lock include/linux/fs.h:774 [inline] #1: ffff88805eec5e88 (&type->i_mutex_dir_key#3){++++}-{3:3}, at: __ext4_ioctl+0x799/0x4e20 fs/ext4/ioctl.c:865 #2: ffff88810bc44ac0 (&sbi->s_writepages_rwsem){++++}-{0:0}, at: ext4_ind_migrate+0x237/0x840 fs/ext4/migrate.c:625 #3: ffff88805eec5c78 (&ei->i_data_sem){++++}-{3:3}, at: ext4_ind_migrate+0x2fe/ 0x840 fs/ext4/migrate.c:633 stack backtrace: CPU: 0 PID: 1080 Comm: syz-fuzzer Not tainted 5.10.208-syzkaller #0 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x107/0x167 lib/dump_stack.c:118 check_noncircular+0x26c/0x320 kernel/locking/lockdep.c:2123 check_prev_add kernel/locking/lockdep.c:2988 [inline] check_prevs_add kernel/locking/lockdep.c:3113 [inline] validate_chain kernel/locking/lockdep.c:3729 [inline] __lock_acquire+0x29c4/0x53c0 kernel/locking/lockdep.c:4955 lock_acquire kernel/locking/lockdep.c:5566 [inline] lock_acquire+0x19f/0x4a0 kernel/locking/lockdep.c:5531 jbd2_log_wait_commit+0x177/0x430 fs/jbd2/journal.c:696 jbd2_journal_stop+0x5b0/0xde0 fs/jbd2/transaction.c:1933 __ext4_journal_stop+0xde/0x1f0 fs/ext4/ext4_jbd2.c:127 ext4_ind_migrate+0x402/0x840 fs/ext4/migrate.c:666 ext4_ioctl_setflags+0xaef/0xc70 fs/ext4/ioctl.c:459 __ext4_ioctl+0x3742/0x4e20 fs/ext4/ioctl.c:870 vfs_ioctl fs/ioctl.c:48 [inline] __do_sys_ioctl fs/ioctl.c:753 [inline] __se_sys_ioctl fs/ioctl.c:739 [inline] __x64_sys_ioctl+0x19a/0x210 fs/ioctl.c:739 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x62/0xc7 RIP: 0033:0x49e0bb Code: e8 0a 4b fc ff eb 88 cc cc cc cc cc cc cc cc e8 1b 8f fc ff 48 8b 7c 24 10 48 8b 74 24 18 48 8b 54 24 20 48 8b 44 24 08 0f 05 <48> 3d 01 f0 ff ff 76 20 48 c7 44 24 28 ff ff ff ff 48 c7 44 24 30 RSP: 002b:000000c002b65ae8 EFLAGS: 00000212 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 000000c000025000 RCX: 000000000049e0bb RDX: 000000c002b65b64 RSI: 0000000040086602 RDI: 000000000000001b RBP: 000000c002b65b28 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000212 R12: 000000c02b38c840 R13: 0000000000000001 R14: 000000c018a321a0 R15: ffffffffffffffff
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c index d98ac2af8199..a5e1492bbaaa 100644 --- a/fs/ext4/migrate.c +++ b/fs/ext4/migrate.c @@ -663,8 +663,8 @@ int ext4_ind_migrate(struct inode *inode) if (unlikely(ret2 && !ret)) ret = ret2; errout: - ext4_journal_stop(handle); up_write(&EXT4_I(inode)->i_data_sem); + ext4_journal_stop(handle); out_unlock: ext4_writepages_up_write(inode->i_sb, alloc_ctx); return ret;
Fuzzing reports a possible deadlock in jbd2_log_wait_commit. The problem occurs in ext4_ind_migrate due to an incorrect order of unlocking of the journal and write semaphores - the order of unlocking must be the reverse of the order of locking. Found by Linux Verification Center (linuxtesting.org) with syzkaller. Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com> --- fs/ext4/migrate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)