linux-kernel - [PATCH] [RFC] Fix data missing when reusing bh which is ready to be checkpointed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221220150551.653925-1-chengzhihao1@huawei.com>
Date:   Tue, 20 Dec 2022 23:05:51 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     <tytso@....edu>, <jack@...e.com>
CC:     <linux-ext4@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <chengzhihao1@...wei.com>, <yi.zhang@...wei.com>,
        <libaokun1@...wei.com>, <zhanchengbin1@...wei.com>,
        <yangerkun@...wei.com>
Subject: [PATCH] [RFC] Fix data missing when reusing bh which is ready to be checkpointed

From: zhanchengbin <zhanchengbin1@...wei.com>

Following process will make data lost and could lead to a filesystem
corrupted problem:

1. jh(bh) is inserted into T1->t_checkpoint_list, bh is dirty, and
   jh->b_transaction = NULL
2. T1 is added into journal->j_checkpoint_transactions.
3. Get bh prepare to write while doing checkpoing:
           PA				    PB
   do_get_write_access             jbd2_log_do_checkpoint
    spin_lock(&jh->b_state_lock)
     if (buffer_dirty(bh))
      clear_buffer_dirty(bh)   // clear buffer dirty
       set_buffer_jbddirty(bh)
				    transaction =
				    journal->j_checkpoint_transactions
				    jh = transaction->t_checkpoint_list
				    if (!buffer_dirty(bh))
		                      __jbd2_journal_remove_checkpoint(jh)
				      // bh won't be flushed
		                    jbd2_cleanup_journal_tail
    __jbd2_journal_file_buffer(jh, transaction, BJ_Reserved)
4. Aborting journal/Power-cut before writing latest bh on journal area.

In this way we get a corrupted filesystem with bh'data lost.

Fix it by wrapping clear_buffer_dirty(bh) and jh->b_transaction setting
into journal->j_list_lock, so that jbd2_log_do_checkpoint() will wait
until jh's new transaction fininshed even bh is currently not dirty.

Cc: <stable@...nel.org>
Signed-off-by: zhanchengbin <zhanchengbin1@...wei.com>
Signed-off-by: Baokun Li <libaokun1@...wei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
---
 This is a quick fix, I need some suggestions about this patch, whether
 it will import new problems if this patch is applied.
 Yi suggests that the formal solution could be splitting
 journal->j_list_lock into two locks: one protects checkpoint list and
 the other one for other lists. Besides, jh->b_state_lock should be
 held while traversing transaction->t_checkpoint_list in
 jbd2_log_do_checkpoint()/journal_shrink_one_cp_list().

 fs/jbd2/transaction.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 6a404ac1c178..d22460001d6b 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -990,6 +990,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
  	start_lock = jiffies;
 	lock_buffer(bh);
 	spin_lock(&jh->b_state_lock);
+	spin_lock(&journal->j_list_lock);
 
 	/* If it takes too long to lock the buffer, trace it */
 	time_lock = jbd2_time_diff(start_lock, jiffies);
@@ -1039,6 +1040,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 
 	error = -EROFS;
 	if (is_handle_aborted(handle)) {
+		spin_unlock(&journal->j_list_lock);
 		spin_unlock(&jh->b_state_lock);
 		goto out;
 	}
@@ -1049,8 +1051,10 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 	 * b_next_transaction points to it
 	 */
 	if (jh->b_transaction == transaction ||
-	    jh->b_next_transaction == transaction)
+	    jh->b_next_transaction == transaction) {
+		spin_unlock(&journal->j_list_lock);
 		goto done;
+	}
 
 	/*
 	 * this is the first time this transaction is touching this buffer,
@@ -1073,11 +1077,11 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 		 * Paired with barrier in jbd2_write_access_granted()
 		 */
 		smp_wmb();
-		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
 		spin_unlock(&journal->j_list_lock);
 		goto done;
 	}
+	spin_unlock(&journal->j_list_lock);
 	/*
 	 * If there is already a copy-out version of this buffer, then we don't
 	 * need to make another one
-- 
2.31.1