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

[PATCH v2] 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.

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com>
Signed-off-by: Mikhail Ukhin <mish.uxin2012@yandex.ru>
---
 v2: New addresses have been added and Ritesh Harjani has been noted as a
 reviewer.
 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 b0ea646454ac..59290356aa5b 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:
 	percpu_up_write(&sbi->s_writepages_rwsem);
 	return ret;
-- 
2.25.1

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

[Theodore Ts'o]

On Sat, Apr 06, 2024 at 12:08:03AM +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.
> 
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com>
> Signed-off-by: Mikhail Ukhin <mish.uxin2012@yandex.ru>

Thanks.  This has been addressed by commit 00d873c17e29 ("ext4: avoid
deadlock in fs reclaim with page writeback"), with the same code
change.

						- Ted

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

[Theodore Ts'o]

On Thu, Jun 06, 2024 at 12:14:22AM +0300, миша ухин wrote:
> <div><div>Thank you for the comment.<br />It seems there might be a misunderstanding.<br />The commit 00d873c17e29 ("ext4: avoid deadlock in fs reclaim with page writeback") you mentioned introduces the use of memalloc_nofs_save()/memalloc_nofs_restore() when acquiring the EXT4_SB(sb)-&gt;s_writepages_rwsem lock.<br />On the other hand the patch we proposed corrects the order of locking/unlocking resources with calls to the functions ext4_journal_start()/ext4_journal_stop() and down_write(&amp;EXT4_I(inode)-&gt;i_data_sem)/up_write(&amp;EXT4_I(inode)-&gt;i_data_sem).<br />These patches do not appear to resolve the same issue, and the code changes are different.</div><div> </div><div>- <span style="white-space:pre-wrap">Mikhail Ukhin</span></div></div>

PLEASE do not send HTML messages to the linux-kernel mailing list.  It
looks like garbage when read on a text mail reader.

In any case, you're correct.  I had misremembered the issue with this
patch.  The complaint that I had made with the V1 of the patch has not
been corrected, which is that the assertion made in the commit
description "the order of unlocking must be the reverse of the order
of locking" is errant nonsense.  It is simply is technically
incorrect; the order in which locks are released doesn't matter.  (And
a jbd2 handle is not a lock.)

The syzkaller report which apparntly triggered this failure was
supplied by Artem here[1], and the explanation should include that it
was triggered by an EXT4_IOC_MIGRATE ioctl which was set to require
synchornous update because the file descriptor was opened with O_SYNC,
and this could result in the jbd2_journal_stop() function calling
jbd2_might_wait_for_commit() which could potentially trigger a
deadlock if the EXT4_IOC_MIGRATE call is racing with write(2) system
call.

[1] https://lore.kernel.org/r/1845977.e0hk0VWMCB@cherry

In any case, this is a low priority issue since the only program which
uses EXT4_IOC_MIGRATE is e4defrag, and it doesn't open files with
O_SYNC, so this isn't going to happen in real life.  And so why don't
you use this as an opportunity to practice writing a technically valid
and correct commit description, and how to properlty submit patches
and send valid (non-HTML) messages to the Linux kernel mailing list?

Cheers,

						- Ted