[PATCH 4.9-4.19] jbd2: fix use-after-free of transaction_t race

[PATCH 4.9-4.19] jbd2: fix use-after-free of transaction_t race

[Samuel Mendoza-Jonas]

From: Ritesh Harjani <riteshh@linux.ibm.com>

commit cc16eecae687912238ee6efbff71ad31e2bc414e upstream.

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.

Link: https://lore.kernel.org/r/948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com
Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
Cc: stable@kernel.org
Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
[backport to 4.9-4.19 in original jbd2_journal_commit_transaction()
 location before the refactor in
 4f9818684870 "jbd2: refactor wait logic for transaction updates into a
 common function"]
Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
Fixes: 1da177e4c3f41524
Cc: stable@kernel.org # 4.9.x - 4.19.x
---
While marked for 5.17 stable, it looks like this fix also applies to the
original location in jbd2_journal_commit_transaction() before it was
refactored to use jbd2_journal_wait_updates(). This applies the same
change there.

 fs/jbd2/commit.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 97760cb9bcd7..66e776eb5ea7 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -382,6 +382,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int csum_size = 0;
 	LIST_HEAD(io_bufs);
 	LIST_HEAD(log_bufs);
+	DEFINE_WAIT(wait);
 
 	if (jbd2_journal_has_csum_v2or3(journal))
 		csum_size = sizeof(struct jbd2_journal_block_tail);
@@ -434,22 +435,25 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
 					      stats.run.rs_locked);
 
-	spin_lock(&commit_transaction->t_handle_lock);
-	while (atomic_read(&commit_transaction->t_updates)) {
-		DEFINE_WAIT(wait);
+	while (1) {
+		commit_transaction = journal->j_running_transaction;
+		if (!commit_transaction)
+			break;
 
+		spin_lock(&commit_transaction->t_handle_lock);
 		prepare_to_wait(&journal->j_wait_updates, &wait,
 					TASK_UNINTERRUPTIBLE);
-		if (atomic_read(&commit_transaction->t_updates)) {
+		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);
+			finish_wait(&journal->j_wait_updates, &wait);
+			break;
 		}
+		spin_unlock(&commit_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);
 
 	J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
 			journal->j_max_transaction_buffers);
-- 
2.25.1

[PATCH 5.4-5.16] jbd2: fix use-after-free of transaction_t race

[Samuel Mendoza-Jonas]

From: Ritesh Harjani <riteshh@linux.ibm.com>

commit cc16eecae687912238ee6efbff71ad31e2bc414e upstream.

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.

Link: https://lore.kernel.org/r/948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com
Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
Cc: stable@kernel.org
Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
[backport to 5.4-5.16 in original jbd2_journal_commit_transaction()
 location before the refactor in
 4f9818684870 "jbd2: refactor wait logic for transaction updates into a
 common function"]
Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
Fixes: 1da177e4c3f41524
Cc: stable@kernel.org # 5.4.x - 5.16.x
---
While marked for 5.17 stable, it looks like this fix also applies to the
original location in jbd2_journal_commit_transaction() before it was
refactored to use jbd2_journal_wait_updates(). This applies the same
change there.

 fs/jbd2/commit.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index d188fa913a07..e9ed8a71ff25 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -408,6 +408,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int csum_size = 0;
 	LIST_HEAD(io_bufs);
 	LIST_HEAD(log_bufs);
+	DEFINE_WAIT(wait);
 
 	if (jbd2_journal_has_csum_v2or3(journal))
 		csum_size = sizeof(struct jbd2_journal_block_tail);
@@ -484,22 +485,25 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
 					      stats.run.rs_locked);
 
-	spin_lock(&commit_transaction->t_handle_lock);
-	while (atomic_read(&commit_transaction->t_updates)) {
-		DEFINE_WAIT(wait);
+	while (1) {
+		commit_transaction = journal->j_running_transaction;
+		if (!commit_transaction)
+			break;
 
+		spin_lock(&commit_transaction->t_handle_lock);
 		prepare_to_wait(&journal->j_wait_updates, &wait,
 					TASK_UNINTERRUPTIBLE);
-		if (atomic_read(&commit_transaction->t_updates)) {
+		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);
+			finish_wait(&journal->j_wait_updates, &wait);
+			break;
 		}
+		spin_unlock(&commit_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);
 	commit_transaction->t_state = T_SWITCH;
 	write_unlock(&journal->j_state_lock);
 
-- 
2.25.1

Re: [PATCH 4.9-4.19] jbd2: fix use-after-free of transaction_t race

[Samuel Mendoza-Jonas]

On Tue, Apr 26, 2022 at 11:27:01AM -0700, Samuel Mendoza-Jonas wrote:
> From: Ritesh Harjani <riteshh@linux.ibm.com>
> 
> commit cc16eecae687912238ee6efbff71ad31e2bc414e upstream.
> 
> 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.
> 
> Link: https://lore.kernel.org/r/948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com
> Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> Cc: stable@kernel.org
> Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> [backport to 4.9-4.19 in original jbd2_journal_commit_transaction()
>  location before the refactor in
>  4f9818684870 "jbd2: refactor wait logic for transaction updates into a
>  common function"]
> Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
> Fixes: 1da177e4c3f41524
> Cc: stable@kernel.org # 4.9.x - 4.19.x
> ---
> While marked for 5.17 stable, it looks like this fix also applies to the
> original location in jbd2_journal_commit_transaction() before it was
> refactored to use jbd2_journal_wait_updates(). This applies the same
> change there.

Jan kindly pointed out this was a false alarm:
https://lore.kernel.org/all/20220427111726.3wdyxbqoxs7skdzf@quack3.lan/

So the existing patch is fine and these can be ignored!

Cheers,
Sam

> 
>  fs/jbd2/commit.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 97760cb9bcd7..66e776eb5ea7 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -382,6 +382,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	int csum_size = 0;
>  	LIST_HEAD(io_bufs);
>  	LIST_HEAD(log_bufs);
> +	DEFINE_WAIT(wait);
>  
>  	if (jbd2_journal_has_csum_v2or3(journal))
>  		csum_size = sizeof(struct jbd2_journal_block_tail);
> @@ -434,22 +435,25 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
>  					      stats.run.rs_locked);
>  
> -	spin_lock(&commit_transaction->t_handle_lock);
> -	while (atomic_read(&commit_transaction->t_updates)) {
> -		DEFINE_WAIT(wait);
> +	while (1) {
> +		commit_transaction = journal->j_running_transaction;
> +		if (!commit_transaction)
> +			break;
>  
> +		spin_lock(&commit_transaction->t_handle_lock);
>  		prepare_to_wait(&journal->j_wait_updates, &wait,
>  					TASK_UNINTERRUPTIBLE);
> -		if (atomic_read(&commit_transaction->t_updates)) {
> +		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);
> +			finish_wait(&journal->j_wait_updates, &wait);
> +			break;
>  		}
> +		spin_unlock(&commit_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);
>  
>  	J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
>  			journal->j_max_transaction_buffers);
> -- 
> 2.25.1
> 

Re: [PATCH 4.9-4.19] jbd2: fix use-after-free of transaction_t race

[Greg KH]

On Wed, Apr 27, 2022 at 09:31:50AM -0700, Samuel Mendoza-Jonas wrote:
> On Tue, Apr 26, 2022 at 11:27:01AM -0700, Samuel Mendoza-Jonas wrote:
> > From: Ritesh Harjani <riteshh@linux.ibm.com>
> > 
> > commit cc16eecae687912238ee6efbff71ad31e2bc414e upstream.
> > 
> > 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.
> > 
> > Link: https://lore.kernel.org/r/948c2fed518ae739db6a8f7f83f1d58b504f87d0.1644497105.git.ritesh.list@gmail.com
> > Fixes: 4f98186848707f53 ("jbd2: refactor wait logic for transaction updates into a common function")
> > Cc: stable@kernel.org
> > Reported-and-tested-by: syzbot+afa2ca5171d93e44b348@syzkaller.appspotmail.com
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > [backport to 4.9-4.19 in original jbd2_journal_commit_transaction()
> >  location before the refactor in
> >  4f9818684870 "jbd2: refactor wait logic for transaction updates into a
> >  common function"]
> > Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
> > Fixes: 1da177e4c3f41524
> > Cc: stable@kernel.org # 4.9.x - 4.19.x
> > ---
> > While marked for 5.17 stable, it looks like this fix also applies to the
> > original location in jbd2_journal_commit_transaction() before it was
> > refactored to use jbd2_journal_wait_updates(). This applies the same
> > change there.
> 
> Jan kindly pointed out this was a false alarm:
> https://lore.kernel.org/all/20220427111726.3wdyxbqoxs7skdzf@quack3.lan/
> 
> So the existing patch is fine and these can be ignored!

Thanks for the update, now all dropped from my queue.

greg k-h