* [PATCH] jbd2: Fix use-after-free of transaction_t race
@ 2022-02-10 15:37 Ritesh Harjani
2022-02-10 19:15 ` Jan Kara
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ritesh Harjani @ 2022-02-10 15:37 UTC (permalink / raw)
To: linux-ext4; +Cc: Jan Kara, Ritesh Harjani, syzbot+afa2ca5171d93e44b348
jbd2_journal_wait_updates() is called with j_state_lock held. But if
there is a commit in progress, then this transaction might get committed
and freed via jbd2_journal_commit_transaction() ->
jbd2_journal_free_transaction(), when we release j_state_lock.
So check for journal->j_running_transaction everytime we release and
acquire j_state_lock to avoid use-after-free issue.
Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
fs/jbd2/transaction.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 8e2f8275a253..259e00046a8b 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -842,27 +842,38 @@ EXPORT_SYMBOL(jbd2_journal_restart);
*/
void jbd2_journal_wait_updates(journal_t *journal)
{
- transaction_t *commit_transaction = journal->j_running_transaction;
+ DEFINE_WAIT(wait);
- if (!commit_transaction)
- return;
+ while (1) {
+ /*
+ * Note that the running transaction can get freed under us if
+ * this transaction is getting committed in
+ * jbd2_journal_commit_transaction() ->
+ * jbd2_journal_free_transaction(). This can only happen when we
+ * release j_state_lock -> schedule() -> acquire j_state_lock.
+ * Hence we should everytime retrieve new j_running_transaction
+ * value (after j_state_lock release acquire cycle), else it may
+ * lead to use-after-free of old freed transaction.
+ */
+ transaction_t *transaction = journal->j_running_transaction;
- spin_lock(&commit_transaction->t_handle_lock);
- while (atomic_read(&commit_transaction->t_updates)) {
- DEFINE_WAIT(wait);
+ if (!transaction)
+ break;
+ spin_lock(&transaction->t_handle_lock);
prepare_to_wait(&journal->j_wait_updates, &wait,
- TASK_UNINTERRUPTIBLE);
- if (atomic_read(&commit_transaction->t_updates)) {
- spin_unlock(&commit_transaction->t_handle_lock);
- write_unlock(&journal->j_state_lock);
- schedule();
- write_lock(&journal->j_state_lock);
- spin_lock(&commit_transaction->t_handle_lock);
+ TASK_UNINTERRUPTIBLE);
+ if (!atomic_read(&transaction->t_updates)) {
+ spin_unlock(&transaction->t_handle_lock);
+ finish_wait(&journal->j_wait_updates, &wait);
+ break;
}
+ spin_unlock(&transaction->t_handle_lock);
+ write_unlock(&journal->j_state_lock);
+ schedule();
finish_wait(&journal->j_wait_updates, &wait);
+ write_lock(&journal->j_state_lock);
}
- spin_unlock(&commit_transaction->t_handle_lock);
}
/**
@@ -877,8 +888,6 @@ void jbd2_journal_wait_updates(journal_t *journal)
*/
void jbd2_journal_lock_updates(journal_t *journal)
{
- DEFINE_WAIT(wait);
-
jbd2_might_wait_for_commit(journal);
write_lock(&journal->j_state_lock);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
2022-02-10 15:37 [PATCH] jbd2: Fix use-after-free of transaction_t race Ritesh Harjani
@ 2022-02-10 19:15 ` Jan Kara
2022-03-03 0:41 ` Theodore Ts'o
2022-04-26 18:31 ` Samuel Mendoza-Jonas
2 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2022-02-10 19:15 UTC (permalink / raw)
To: Ritesh Harjani; +Cc: linux-ext4, Jan Kara, syzbot+afa2ca5171d93e44b348
On Thu 10-02-22 21:07:11, Ritesh Harjani wrote:
> jbd2_journal_wait_updates() is called with j_state_lock held. But if
> there is a commit in progress, then this transaction might get committed
> and freed via jbd2_journal_commit_transaction() ->
> jbd2_journal_free_transaction(), when we release j_state_lock.
> So check for journal->j_running_transaction everytime we release and
> acquire j_state_lock to avoid use-after-free issue.
>
> Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/jbd2/transaction.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 8e2f8275a253..259e00046a8b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -842,27 +842,38 @@ EXPORT_SYMBOL(jbd2_journal_restart);
> */
> void jbd2_journal_wait_updates(journal_t *journal)
> {
> - transaction_t *commit_transaction = journal->j_running_transaction;
> + DEFINE_WAIT(wait);
>
> - if (!commit_transaction)
> - return;
> + while (1) {
> + /*
> + * Note that the running transaction can get freed under us if
> + * this transaction is getting committed in
> + * jbd2_journal_commit_transaction() ->
> + * jbd2_journal_free_transaction(). This can only happen when we
> + * release j_state_lock -> schedule() -> acquire j_state_lock.
> + * Hence we should everytime retrieve new j_running_transaction
> + * value (after j_state_lock release acquire cycle), else it may
> + * lead to use-after-free of old freed transaction.
> + */
> + transaction_t *transaction = journal->j_running_transaction;
>
> - spin_lock(&commit_transaction->t_handle_lock);
> - while (atomic_read(&commit_transaction->t_updates)) {
> - DEFINE_WAIT(wait);
> + if (!transaction)
> + break;
>
> + spin_lock(&transaction->t_handle_lock);
> prepare_to_wait(&journal->j_wait_updates, &wait,
> - TASK_UNINTERRUPTIBLE);
> - if (atomic_read(&commit_transaction->t_updates)) {
> - spin_unlock(&commit_transaction->t_handle_lock);
> - write_unlock(&journal->j_state_lock);
> - schedule();
> - write_lock(&journal->j_state_lock);
> - spin_lock(&commit_transaction->t_handle_lock);
> + TASK_UNINTERRUPTIBLE);
> + if (!atomic_read(&transaction->t_updates)) {
> + spin_unlock(&transaction->t_handle_lock);
> + finish_wait(&journal->j_wait_updates, &wait);
> + break;
> }
> + spin_unlock(&transaction->t_handle_lock);
> + write_unlock(&journal->j_state_lock);
> + schedule();
> finish_wait(&journal->j_wait_updates, &wait);
> + write_lock(&journal->j_state_lock);
> }
> - spin_unlock(&commit_transaction->t_handle_lock);
> }
>
> /**
> @@ -877,8 +888,6 @@ void jbd2_journal_wait_updates(journal_t *journal)
> */
> void jbd2_journal_lock_updates(journal_t *journal)
> {
> - DEFINE_WAIT(wait);
> -
> jbd2_might_wait_for_commit(journal);
>
> write_lock(&journal->j_state_lock);
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
2022-02-10 15:37 [PATCH] jbd2: Fix use-after-free of transaction_t race Ritesh Harjani
2022-02-10 19:15 ` Jan Kara
@ 2022-03-03 0:41 ` Theodore Ts'o
2022-04-26 18:31 ` Samuel Mendoza-Jonas
2 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2022-03-03 0:41 UTC (permalink / raw)
To: Ritesh Harjani, linux-ext4
Cc: Theodore Ts'o, syzbot+afa2ca5171d93e44b348, Jan Kara
On Thu, 10 Feb 2022 21:07:11 +0530, Ritesh Harjani wrote:
> jbd2_journal_wait_updates() is called with j_state_lock held. But if
> there is a commit in progress, then this transaction might get committed
> and freed via jbd2_journal_commit_transaction() ->
> jbd2_journal_free_transaction(), when we release j_state_lock.
> So check for journal->j_running_transaction everytime we release and
> acquire j_state_lock to avoid use-after-free issue.
>
> [...]
Applied, thanks!
[1/1] jbd2: Fix use-after-free of transaction_t race
commit: cc16eecae687912238ee6efbff71ad31e2bc414e
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
2022-02-10 15:37 [PATCH] jbd2: Fix use-after-free of transaction_t race Ritesh Harjani
2022-02-10 19:15 ` Jan Kara
2022-03-03 0:41 ` Theodore Ts'o
@ 2022-04-26 18:31 ` Samuel Mendoza-Jonas
2022-04-27 11:17 ` Jan Kara
2 siblings, 1 reply; 6+ messages in thread
From: Samuel Mendoza-Jonas @ 2022-04-26 18:31 UTC (permalink / raw)
To: Ritesh Harjani; +Cc: linux-ext4, Jan Kara, syzbot+afa2ca5171d93e44b348
On Thu, Feb 10, 2022 at 09:07:11PM +0530, Ritesh Harjani wrote:
> jbd2_journal_wait_updates() is called with j_state_lock held. But if
> there is a commit in progress, then this transaction might get committed
> and freed via jbd2_journal_commit_transaction() ->
> jbd2_journal_free_transaction(), when we release j_state_lock.
> So check for journal->j_running_transaction everytime we release and
> acquire j_state_lock to avoid use-after-free issue.
>
> Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Hi Ritesh,
Looking at the refactor in the commit this fixes, I believe the same
issue is present prior to the refactor, so this would apply before 5.17
as well.
I've posted a backport for 4.9-4.19 and 5.4-5.16 to stable here:
https://lore.kernel.org/stable/20220426182702.716304-1-samjonas@amazon.com/T/#t
Please have a look and let me know if you agree.
Cheers,
Sam Mendoza-Jonas
> ---
> fs/jbd2/transaction.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 8e2f8275a253..259e00046a8b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -842,27 +842,38 @@ EXPORT_SYMBOL(jbd2_journal_restart);
> */
> void jbd2_journal_wait_updates(journal_t *journal)
> {
> - transaction_t *commit_transaction = journal->j_running_transaction;
> + DEFINE_WAIT(wait);
>
> - if (!commit_transaction)
> - return;
> + while (1) {
> + /*
> + * Note that the running transaction can get freed under us if
> + * this transaction is getting committed in
> + * jbd2_journal_commit_transaction() ->
> + * jbd2_journal_free_transaction(). This can only happen when we
> + * release j_state_lock -> schedule() -> acquire j_state_lock.
> + * Hence we should everytime retrieve new j_running_transaction
> + * value (after j_state_lock release acquire cycle), else it may
> + * lead to use-after-free of old freed transaction.
> + */
> + transaction_t *transaction = journal->j_running_transaction;
>
> - spin_lock(&commit_transaction->t_handle_lock);
> - while (atomic_read(&commit_transaction->t_updates)) {
> - DEFINE_WAIT(wait);
> + if (!transaction)
> + break;
>
> + spin_lock(&transaction->t_handle_lock);
> prepare_to_wait(&journal->j_wait_updates, &wait,
> - TASK_UNINTERRUPTIBLE);
> - if (atomic_read(&commit_transaction->t_updates)) {
> - spin_unlock(&commit_transaction->t_handle_lock);
> - write_unlock(&journal->j_state_lock);
> - schedule();
> - write_lock(&journal->j_state_lock);
> - spin_lock(&commit_transaction->t_handle_lock);
> + TASK_UNINTERRUPTIBLE);
> + if (!atomic_read(&transaction->t_updates)) {
> + spin_unlock(&transaction->t_handle_lock);
> + finish_wait(&journal->j_wait_updates, &wait);
> + break;
> }
> + spin_unlock(&transaction->t_handle_lock);
> + write_unlock(&journal->j_state_lock);
> + schedule();
> finish_wait(&journal->j_wait_updates, &wait);
> + write_lock(&journal->j_state_lock);
> }
> - spin_unlock(&commit_transaction->t_handle_lock);
> }
>
> /**
> @@ -877,8 +888,6 @@ void jbd2_journal_wait_updates(journal_t *journal)
> */
> void jbd2_journal_lock_updates(journal_t *journal)
> {
> - DEFINE_WAIT(wait);
> -
> jbd2_might_wait_for_commit(journal);
>
> write_lock(&journal->j_state_lock);
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
2022-04-26 18:31 ` Samuel Mendoza-Jonas
@ 2022-04-27 11:17 ` Jan Kara
2022-04-27 16:21 ` Samuel Mendoza-Jonas
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2022-04-27 11:17 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: Ritesh Harjani, linux-ext4, Jan Kara, syzbot+afa2ca5171d93e44b348
On Tue 26-04-22 11:31:24, Samuel Mendoza-Jonas wrote:
> On Thu, Feb 10, 2022 at 09:07:11PM +0530, Ritesh Harjani wrote:
> > jbd2_journal_wait_updates() is called with j_state_lock held. But if
> > there is a commit in progress, then this transaction might get committed
> > and freed via jbd2_journal_commit_transaction() ->
> > jbd2_journal_free_transaction(), when we release j_state_lock.
> > So check for journal->j_running_transaction everytime we release and
> > acquire j_state_lock to avoid use-after-free issue.
> >
> > Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> > Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>
> Hi Ritesh,
>
> Looking at the refactor in the commit this fixes, I believe the same
> issue is present prior to the refactor, so this would apply before 5.17
> as well.
> I've posted a backport for 4.9-4.19 and 5.4-5.16 to stable here:
> https://lore.kernel.org/stable/20220426182702.716304-1-samjonas@amazon.com/T/#t
>
> Please have a look and let me know if you agree.
Actually the refactor was indeed the cause for use-after-free. The original
code in jbd2_journal_lock_updates() was like:
/* Wait until there are no running updates */
while (1) {
transaction_t *transaction = journal->j_running_transaction;
if (!transaction)
break;
spin_lock(&transaction->t_handle_lock);
prepare_to_wait(&journal->j_wait_updates, &wait,
TASK_UNINTERRUPTIBLE);
if (!atomic_read(&transaction->t_updates)) {
spin_unlock(&transaction->t_handle_lock);
finish_wait(&journal->j_wait_updates, &wait);
break;
}
spin_unlock(&transaction->t_handle_lock);
write_unlock(&journal->j_state_lock);
schedule();
finish_wait(&journal->j_wait_updates, &wait);
write_lock(&journal->j_state_lock);
}
So you can see the code was indeed careful enough to not touch
t_handle_lock after sleeping. The code in jbd2_journal_commit_transaction()
did touch t_handle_lock but there it didn't matter because nobody else
besides the task running jbd2_journal_commit_transaction() can free the
transaction...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: Fix use-after-free of transaction_t race
2022-04-27 11:17 ` Jan Kara
@ 2022-04-27 16:21 ` Samuel Mendoza-Jonas
0 siblings, 0 replies; 6+ messages in thread
From: Samuel Mendoza-Jonas @ 2022-04-27 16:21 UTC (permalink / raw)
To: Jan Kara; +Cc: Ritesh Harjani, linux-ext4, syzbot+afa2ca5171d93e44b348
On Wed, Apr 27, 2022 at 01:17:26PM +0200, Jan Kara wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue 26-04-22 11:31:24, Samuel Mendoza-Jonas wrote:
> > On Thu, Feb 10, 2022 at 09:07:11PM +0530, Ritesh Harjani wrote:
> > > jbd2_journal_wait_updates() is called with j_state_lock held. But if
> > > there is a commit in progress, then this transaction might get committed
> > > and freed via jbd2_journal_commit_transaction() ->
> > > jbd2_journal_free_transaction(), when we release j_state_lock.
> > > So check for journal->j_running_transaction everytime we release and
> > > acquire j_state_lock to avoid use-after-free issue.
> > >
> > > Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> > > Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > Hi Ritesh,
> >
> > Looking at the refactor in the commit this fixes, I believe the same
> > issue is present prior to the refactor, so this would apply before 5.17
> > as well.
> > I've posted a backport for 4.9-4.19 and 5.4-5.16 to stable here:
> > https://lore.kernel.org/stable/20220426182702.716304-1-samjonas@amazon.com/T/#t
> >
> > Please have a look and let me know if you agree.
>
> Actually the refactor was indeed the cause for use-after-free. The original
> code in jbd2_journal_lock_updates() was like:
>
> /* Wait until there are no running updates */
> while (1) {
> transaction_t *transaction = journal->j_running_transaction;
>
> if (!transaction)
> break;
> spin_lock(&transaction->t_handle_lock);
> prepare_to_wait(&journal->j_wait_updates, &wait,
> TASK_UNINTERRUPTIBLE);
> if (!atomic_read(&transaction->t_updates)) {
> spin_unlock(&transaction->t_handle_lock);
> finish_wait(&journal->j_wait_updates, &wait);
> break;
> }
> spin_unlock(&transaction->t_handle_lock);
> write_unlock(&journal->j_state_lock);
> schedule();
> finish_wait(&journal->j_wait_updates, &wait);
> write_lock(&journal->j_state_lock);
> }
>
> So you can see the code was indeed careful enough to not touch
> t_handle_lock after sleeping. The code in jbd2_journal_commit_transaction()
> did touch t_handle_lock but there it didn't matter because nobody else
> besides the task running jbd2_journal_commit_transaction() can free the
> transaction...
>
Right you are, I misinterpreted the interaction with
jbd2_journal_commit_transaction(). Thanks for verifying, I'll follow up
stable to disregard those backports.
Cheers,
Sam
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-27 16:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10 15:37 [PATCH] jbd2: Fix use-after-free of transaction_t race Ritesh Harjani
2022-02-10 19:15 ` Jan Kara
2022-03-03 0:41 ` Theodore Ts'o
2022-04-26 18:31 ` Samuel Mendoza-Jonas
2022-04-27 11:17 ` Jan Kara
2022-04-27 16:21 ` Samuel Mendoza-Jonas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.