* [PATCH v2] ext4: fix i_data_sem unlock order in ext4_ind_migrate()
@ 2024-04-05 21:08 Mikhail Ukhin
2024-05-09 14:51 ` Theodore Ts'o
0 siblings, 1 reply; 3+ messages in thread
From: Mikhail Ukhin @ 2024-04-05 21:08 UTC (permalink / raw
To: Theodore Ts'o
Cc: Mikhail Ukhin, stable, Andreas Dilger, linux-ext4, linux-kernel,
Michail Ivanov, Pavel Koshutin, lvc-project, Ritesh Harjani,
Artem Sadovnikov
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ext4: fix i_data_sem unlock order in ext4_ind_migrate()
2024-04-05 21:08 [PATCH v2] ext4: fix i_data_sem unlock order in ext4_ind_migrate() Mikhail Ukhin
@ 2024-05-09 14:51 ` Theodore Ts'o
[not found] ` <3159311717621748@mail.yandex.ru>
0 siblings, 1 reply; 3+ messages in thread
From: Theodore Ts'o @ 2024-05-09 14:51 UTC (permalink / raw
To: Mikhail Ukhin
Cc: stable, Andreas Dilger, linux-ext4, linux-kernel, Michail Ivanov,
Pavel Koshutin, lvc-project, Ritesh Harjani, Artem Sadovnikov
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ext4: fix i_data_sem unlock order in ext4_ind_migrate()
[not found] ` <3159311717621748@mail.yandex.ru>
@ 2024-06-06 21:07 ` Theodore Ts'o
0 siblings, 0 replies; 3+ messages in thread
From: Theodore Ts'o @ 2024-06-06 21:07 UTC (permalink / raw
To: миша ухин
Cc: stable@vger.kernel.org, Andreas Dilger,
linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
Michail Ivanov, Pavel Koshutin, lvc-project@linuxtesting.org,
Ritesh Harjani, Artem Sadovnikov
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)->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(&EXT4_I(inode)->i_data_sem)/up_write(&EXT4_I(inode)->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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-06 21:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-05 21:08 [PATCH v2] ext4: fix i_data_sem unlock order in ext4_ind_migrate() Mikhail Ukhin
2024-05-09 14:51 ` Theodore Ts'o
[not found] ` <3159311717621748@mail.yandex.ru>
2024-06-06 21:07 ` Theodore Ts'o
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).