| Message ID | 20240328143301.6432-1-mish.uxin2012@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: fix semaphore unlocking order | expand |
Mikhail Ukhin <mish.uxin2012@yandex.ru> writes: > 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. > 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 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. I am not sure if there is an easy reproducer. If yes, we should consider adding it in fstests. > > Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com> Thanks for fixing it. With above changes to subject and CC tag, this looks good to me. Feel free to add - Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@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
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
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.
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(-)