Skip to contentSkip to conversation

jbd2: avoid transaction reuse after reformatting

PATCH Applied Jan Kara 3 participants 4 messages Oct 7, 2020 – Oct 12, 2020 1 message not in the archive Journalling layer for block devices (JBD2)

v4 of 6 Applied Latest is v6 →

lore
Share & tools
lore

Jan Kara

author
#0 lore raw

PATCH v4 1/1 jbd2: avoid transaction reuse after reformatting

From: changfengnan <[email protected]>

When ext4 is formatted with lazy_journal_init=1 and transactions from the previous filesystem are still on disk, it is possible that they are considered during a recovery after a crash. Because the checksum seed has changed, the CRC check will fail, and the journal recovery fails with checksum error although the journal is otherwise perfectly valid. Fix the problem by checking commit block time stamps to determine whether the data in the journal block is just stale or whether it is indeed corrupt.

---
 fs/jbd2/recovery.c | 77 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 12 deletions(-)

Changes since v3 (https://lore.kernel.org/linux-ext4/[email protected]):
* fixed sparse warnings

fs/jbd2/recovery.c +65 -12
index faa97d748474..acca8463ab16 100644
@@ -428,6 +428,8 @@ static int do_one_pass(journal_t *journal,
 	__u32			crc32_sum = ~0; /* Transactional Checksums */
 	int			descr_csum_size = 0;
 	int			block_error = 0;
+	bool			need_check_commit_time = false;
+	__u64			last_trans_commit_time = 0;
 
 	/*
 	 * First thing is to establish what we expect to find in the log
@@ -520,12 +522,22 @@ static int do_one_pass(journal_t *journal,
 			if (descr_csum_size > 0 &&
 			    !jbd2_descriptor_block_csum_verify(journal,
 							       bh->b_data)) {
-				printk(KERN_ERR "JBD2: Invalid checksum "
-				       "recovering block %lu in log\n",
-				       next_log_block);
-				err = -EFSBADCRC;
-				brelse(bh);
-				goto failed;
+				/*
+				 * PASS_SCAN can see stale blocks due to lazy
+ 				 * journal init. Don't error out on those yet.
+				 */
+				if (pass != PASS_SCAN) {
+					pr_err("JBD2: Invalid checksum "
+					       "recovering block %lu in log\n",
+					       next_log_block);
+					err = -EFSBADCRC;
+					brelse(bh);
+					goto failed;
+				}
+				need_check_commit_time = true;
+				jbd_debug(1,
+					"invalid descriptor block found in %lu\n",
+					next_log_block);
 			}
 
 			/* If it is a valid descriptor block, replay it
@@ -535,6 +547,7 @@ static int do_one_pass(journal_t *journal,
 			if (pass != PASS_REPLAY) {
 				if (pass == PASS_SCAN &&
 				    jbd2_has_feature_checksum(journal) &&
+				    !need_check_commit_time &&
 				    !info->end_transaction) {
 					if (calc_chksums(journal, bh,
 							&next_log_block,
@@ -684,16 +697,20 @@ static int do_one_pass(journal_t *journal,
 			 *	 "Interrupted Commit".)
 			 */
 
-			/* Found an expected commit block: if checksums
-			 * are present verify them in PASS_SCAN; else not
+			/*
+			 * Found an expected commit block: if checksums
+			 * are present, verify them in PASS_SCAN; else not
 			 * much to do other than move on to the next sequence
-			 * number. */
+			 * number.
+			 */
 			if (pass == PASS_SCAN &&
 			    jbd2_has_feature_checksum(journal)) {
 				struct commit_header *cbh =
 					(struct commit_header *)bh->b_data;
 				unsigned found_chksum =
 					be32_to_cpu(cbh->h_chksum[0]);
+				__u64 commit_time =
+					be64_to_cpu(cbh->h_commit_sec);
 
 				if (info->end_transaction) {
 					journal->j_failed_commit =
@@ -702,6 +719,33 @@ static int do_one_pass(journal_t *journal,
 					break;
 				}
 
+				/*
+				 * If need_check_commit_time is set, it means
+				 * csum verify failed before, if commit_time is
+				 * increasing, it's same journal, otherwise it
+				 * is stale journal block, just end this
+				 * recovery.
+				 */
+				if (need_check_commit_time) {
+					if (commit_time >= last_trans_commit_time) {
+						pr_err("JBD2: Invalid checksum found in transaction %u\n",
+						       next_commit_ID);
+						err = -EFSBADCRC;
+						brelse(bh);
+						goto failed;
+					}
+					/*
+					 * It likely does not belong to same
+					 * journal, just end this recovery with
+					 * success.
+					 */
+					jbd_debug(1, "JBD2: Invalid checksum ignored in transaction %u, likely stale data\n",
+						  next_commit_ID);
+					err = 0;
+					brelse(bh);
+					goto done;
+				}
+
 				/* Neither checksum match nor unused? */
 				if (!((crc32_sum == found_chksum &&
 				       cbh->h_chksum_type ==
@@ -714,6 +758,7 @@ static int do_one_pass(journal_t *journal,
 					goto chksum_error;
 
 				crc32_sum = ~0;
+				last_trans_commit_time = commit_time;
 			}
 			if (pass == PASS_SCAN &&
 			    !jbd2_commit_block_csum_verify(journal,
@@ -733,6 +778,17 @@ static int do_one_pass(journal_t *journal,
 			continue;
 
 		case JBD2_REVOKE_BLOCK:
+			/*
+			 * Check revoke block crc in pass_scan, if csum verify
+			 * failed, check commit block time later.
+			 */
+			if (pass == PASS_SCAN &&
+			    !jbd2_descriptor_block_csum_verify(journal,
+							       bh->b_data)) {
+				jbd_debug(1, "JBD2: invalid revoke block found in %lu\n",
+					  next_log_block);
+				need_check_commit_time = true;
+			}
 			/* If we aren't in the REVOKE pass, then we can
 			 * just skip over this block. */
 			if (pass != PASS_REVOKE) {
@@ -800,9 +856,6 @@ static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
 	offset = sizeof(jbd2_journal_revoke_header_t);
 	rcount = be32_to_cpu(header->r_count);
 
-	if (!jbd2_descriptor_block_csum_verify(journal, header))
-		return -EFSBADCRC;
-
 	if (jbd2_journal_has_csum_v2or3(journal))
 		csum_size = sizeof(struct jbd2_journal_block_tail);
 	if (rcount > journal->j_blocksize - csum_size)
Signature · 1 line
2.16.4
Reviewed-by on v4 1/1
#1 lore raw
Quoted 12 lines · Jan Kara

From: changfengnan <[email protected]>

When ext4 is formatted with lazy_journal_init=1 and transactions from the previous filesystem are still on disk, it is possible that they are considered during a recovery after a crash. Because the checksum seed has changed, the CRC check will fail, and the journal recovery fails with checksum error although the journal is otherwise perfectly valid. Fix the problem by checking commit block time stamps to determine whether the data in the journal block is just stale or whether it is indeed corrupt.

Quoted 3 lines

NB: one trivial formatting cleanup if patch is refreshed

Quoted 7 lines
fs/jbd2/recovery.c +0 -3
@@ -520,12 +522,22 @@ static int do_one_pass(journal_t *journal,
 			if (descr_csum_size > 0 &&
 			    !jbd2_descriptor_block_csum_verify(journal,
 							       bh->b_data)) {
-				printk(KERN_ERR "JBD2: Invalid checksum "
-				       "recovering block %lu in log\n",
-				       next_log_block);

fs/jbd2/recovery.c static int do_one_pass(journal_t *journal, · line 531

Quoted 10 lines
fs/jbd2/recovery.c +7 -3
-				err = -EFSBADCRC;
-				brelse(bh);
-				goto failed;
+				/*
+				 * PASS_SCAN can see stale blocks due to lazy
+ 				 * journal init. Don't error out on those yet.
+				 */
+				if (pass != PASS_SCAN) {
+					pr_err("JBD2: Invalid checksum "
+					       "recovering block %lu in log\n",

(style) should keep console message strings on a single line

Cheers, Andreas

maintainer Applied
#2 lore raw
Quoted 13 lines · Andreas Dilger
Quoted 12 lines · Jan Kara

From: changfengnan <[email protected]>

When ext4 is formatted with lazy_journal_init=1 and transactions from the previous filesystem are still on disk, it is possible that they are considered during a recovery after a crash. Because the checksum seed has changed, the CRC check will fail, and the journal recovery fails with checksum error although the journal is otherwise perfectly valid. Fix the problem by checking commit block time stamps to determine whether the data in the journal block is just stale or whether it is indeed corrupt.

Quoted 8 lines
Quoted 3 lines

NB: one trivial formatting cleanup if patch is refreshed

Applied, thanks. I fixed the trivial format cleanup you pointed out, plus a whitespace fix pointed out by checkpatch.

- Ted

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 11 lines · 常凤楠

Hi Jan:
Thank you for your suggestions,I tested the new version of the patch,I think there still have some prblems. 1. Looks like you think jbd2_has_feature_checksum can determine that CRC is enabled,but this is different from jbd2_journal_has_csum_v2or3. so when csum is v2 or v3, this is still have problem. 2. This patch looks fixed the situations of descriptor and revoke block, commit block is not considered. Maybe it’s because my previous modification was problematic,I have a new idea, how about check crc first and compare timestap,if check crc is failed, then compare timestap, this way the risk will be much smaller. What do you think?

Hum, you're right that commit block checking will not work with v2/v3 checksums. Thanks for catching that! I like the order of checks you propose to fix the problem, I'll update the patch. Thanks!

Honza

Quoted 36 lines

------------------&nbsp;Original&nbsp;------------------
From: "Theodore Y. Ts'o" <[email protected]&gt;;
Date:&nbsp;Fri, Oct 9, 2020 10:16 AM
To:&nbsp;"Andreas Dilger"<[email protected]&gt;;
Cc:&nbsp;"Jan Kara"<[email protected]&gt;;"linux-ext4"<[email protected]&gt;;"Fengnan Chang"<[email protected]&gt;;"常凤楠"<[email protected]&gt;; Subject:&nbsp;Re: [PATCH v4] jbd2: avoid transaction reuse after reformatting

On Thu, Oct 08, 2020 at 02:13:02PM -0600, Andreas Dilger wrote:
&gt; On Oct 7, 2020, at 2:13 AM, Jan Kara <[email protected]&gt; wrote:
&gt; &gt;
&gt; &gt; From: changfengnan <[email protected]&gt;
&gt; &gt;
&gt; &gt; When ext4 is formatted with lazy_journal_init=1 and transactions from &gt; &gt; the previous filesystem are still on disk, it is possible that they are &gt; &gt; considered during a recovery after a crash. Because the checksum seed &gt; &gt; has changed, the CRC check will fail, and the journal recovery fails &gt; &gt; with checksum error although the journal is otherwise perfectly valid. &gt; &gt; Fix the problem by checking commit block time stamps to determine &gt; &gt; whether the data in the journal block is just stale or whether it is &gt; &gt; indeed corrupt.
&gt; &gt;
&gt; &gt; Reported-by: kernel test robot <[email protected]&gt;
&gt; &gt; Signed-off-by: Fengnan Chang <[email protected]&gt; &gt; &gt; Signed-off-by: Jan Kara <[email protected]&gt;
&gt;
&gt; Reviewed-by: Andreas Dilger <[email protected]&gt;
&gt;
&gt; NB: one trivial formatting cleanup if patch is refreshed
&gt;

Applied, thanks.&nbsp; I fixed the trivial format cleanup you pointed out, plus a whitespace fix pointed out by checkpatch.

&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp;&nbsp; - Ted

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR