2023-07-14 03:06:59

by Zhang Yi

Subject: [PATCH 2/3] jbd2: Check 'jh->b_transaction' before remove it from checkpoint

From: Zhihao Cheng <[email protected]>

Following process will corrupt ext4 image:
Step 1:
jbd2_journal_commit_transaction
__jbd2_journal_insert_checkpoint(jh, commit_transaction)
// Put jh into trans1->t_checkpoint_list
journal->j_checkpoint_transactions = commit_transaction
// Put trans1 into journal->j_checkpoint_transactions

Step 2:
do_get_write_access
test_clear_buffer_dirty(bh) // clear buffer dirty，set jbd dirty
__jbd2_journal_file_buffer(jh, transaction) // jh belongs to trans2

Step 3:
drop_cache
journal_shrink_one_cp_list
jbd2_journal_try_remove_checkpoint
if (!trylock_buffer(bh)) // lock bh, true
if (buffer_dirty(bh)) // buffer is not dirty
__jbd2_journal_remove_checkpoint(jh)
// remove jh from trans1->t_checkpoint_list

Step 4:
jbd2_log_do_checkpoint
trans1 = journal->j_checkpoint_transactions
// jh is not in trans1->t_checkpoint_list
jbd2_cleanup_journal_tail(journal) // trans1 is done

Step 5: Power cut, trans2 is not committed, jh is lost in next mounting.

Fix it by checking 'jh->b_transaction' before remove it from checkpoint.

Fixes: 46f881b5b175 ("jbd2: fix a race when checking checkpoint buffer busy")
Signed-off-by: Zhihao Cheng <[email protected]>
Signed-off-by: Zhang Yi <[email protected]>
---
fs/jbd2/checkpoint.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 936c6d758a65..f033ac807013 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -639,6 +639,8 @@ int jbd2_journal_try_remove_checkpoint(struct journal_head *jh)
{
struct buffer_head *bh = jh2bh(jh);

+ if (jh->b_transaction)
+ return -EBUSY;
if (!trylock_buffer(bh))
return -EBUSY;
if (buffer_dirty(bh)) {
--
2.39.2

2023-07-31 16:40:27

by Jan Kara

Subject: Re: [PATCH 2/3] jbd2: Check 'jh->b_transaction' before remove it from checkpoint

On Fri 14-07-23 10:55:27, Zhang Yi wrote:
> From: Zhihao Cheng <[email protected]>
>
> Following process will corrupt ext4 image:
> Step 1:
> jbd2_journal_commit_transaction
> __jbd2_journal_insert_checkpoint(jh, commit_transaction)
> // Put jh into trans1->t_checkpoint_list
> journal->j_checkpoint_transactions = commit_transaction
> // Put trans1 into journal->j_checkpoint_transactions
>
> Step 2:
> do_get_write_access
> test_clear_buffer_dirty(bh) // clear buffer dirty，set jbd dirty
> __jbd2_journal_file_buffer(jh, transaction) // jh belongs to trans2
>
> Step 3:
> drop_cache
> journal_shrink_one_cp_list
> jbd2_journal_try_remove_checkpoint
> if (!trylock_buffer(bh)) // lock bh, true
> if (buffer_dirty(bh)) // buffer is not dirty
> __jbd2_journal_remove_checkpoint(jh)
> // remove jh from trans1->t_checkpoint_list
>
> Step 4:
> jbd2_log_do_checkpoint
> trans1 = journal->j_checkpoint_transactions
> // jh is not in trans1->t_checkpoint_list
> jbd2_cleanup_journal_tail(journal) // trans1 is done
>
> Step 5: Power cut, trans2 is not committed, jh is lost in next mounting.
>
> Fix it by checking 'jh->b_transaction' before remove it from checkpoint.
>
> Fixes: 46f881b5b175 ("jbd2: fix a race when checking checkpoint buffer busy")
> Signed-off-by: Zhihao Cheng <[email protected]>
> Signed-off-by: Zhang Yi <[email protected]>

Indeed! I've missed this difference between __cp_buffer_busy() and
jbd2_journal_try_remove_checkpoint() during my review of 46f881b5b175. The
fix looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/jbd2/checkpoint.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 936c6d758a65..f033ac807013 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -639,6 +639,8 @@ int jbd2_journal_try_remove_checkpoint(struct journal_head *jh)
> {
> struct buffer_head *bh = jh2bh(jh);
>
> + if (jh->b_transaction)
> + return -EBUSY;
> if (!trylock_buffer(bh))
> return -EBUSY;
> if (buffer_dirty(bh)) {
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR