[PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[Mikhail Ukhin]

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(-)

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

Re: [PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[Greg KH]

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>

Re: [PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[Theodore Ts'o]

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

Re: [PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[Artem Sadovnikov]

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

Re: [PATCH] ext4: fix semaphore unlocking order

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Mikhail Ukhin <mish.uxin2012@yandex.ru> writes:
>
>> Fuzzing reports a possible deadlock in jbd2_log_wait_commit.

I think I agree with what Jan hinted to me in the call, that
how can an unlock order mismatch be a deadlock.

But yes, a wrong unlock order can increase the locking times of
thread-2 waiting on lock B; for e.g. if a premption happens between
unlock of lock A & B by thread-1.

So it is always good to fix the unlock order too.

>>
>> 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.
>>
>
> Maybe we should update the subject msg to:
>       "ext4: "fix i_data_sem unlock order in ext4_ind_migrate()"
>
> and also should add:
>     CC: stable@vger.kernel.org

In that case, I am not really sure, if this requires a cc'd stable.
So, I will leave this upto Ted.

>
>
> I think this should have been fixed in patch [1], but looks like it
> forgot to fix the unlock order.
>
> [1]: https://lore.kernel.org/all/1364801462-13120-1-git-send-email-dmonakhov@openvz.org/
>
>
>> Found by Linux Verification Center (linuxtesting.org) with syzkaller.

It will be good to know what was the test which identified this though?

-ritesh

Re: [PATCH] ext4: fix i_data_sem unlock order in ext4_ind_migrate()

[Mikhail Ukhin]

On 04.04.2024 19:02, Ritesh Harjani (IBM) wrote:
> It will be good to know what was the test which identified this though?
>
> -ritesh


Unfortunately syzkaller was not able to generate a reproducer for this 
issue.


- Ukhin Mikhail.