Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] jbd2: avoid transaction reuse after reformatting
@ 2020-10-07  8:13 Jan Kara
  2020-10-08 20:13 ` Andreas Dilger
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2020-10-07  8:13 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, Fengnan Chang, changfengnan, Jan Kara

From: changfengnan <fengnanchang@foxmail.com>

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.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Fengnan Chang <changfengnan@hikvision.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/recovery.c | 77 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 12 deletions(-)

Changes since v3 (https://lore.kernel.org/linux-ext4/20201006103154.7130-1-jack@suse.cz):
* fixed sparse warnings

diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index faa97d748474..acca8463ab16 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -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)
-- 
2.16.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] jbd2: avoid transaction reuse after reformatting
  2020-10-07  8:13 [PATCH v4] jbd2: avoid transaction reuse after reformatting Jan Kara
@ 2020-10-08 20:13 ` Andreas Dilger
  2020-10-09  2:16   ` Theodore Y. Ts'o
  0 siblings, 1 reply; 4+ messages in thread
From: Andreas Dilger @ 2020-10-08 20:13 UTC (permalink / raw)
  To: Jan Kara; +Cc: Ted Tso, linux-ext4, Fengnan Chang, changfengnan


[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Oct 7, 2020, at 2:13 AM, Jan Kara <jack@suse.cz> wrote:
> 
> From: changfengnan <fengnanchang@foxmail.com>
> 
> 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.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Fengnan Chang <changfengnan@hikvision.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

NB: one trivial formatting cleanup if patch is refreshed

> @@ -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",

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


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] jbd2: avoid transaction reuse after reformatting
  2020-10-08 20:13 ` Andreas Dilger
@ 2020-10-09  2:16   ` Theodore Y. Ts'o
       [not found]     ` <tencent_909C05B896D07CD8BC84F2EC74B03A1A4007@qq.com>
  0 siblings, 1 reply; 4+ messages in thread
From: Theodore Y. Ts'o @ 2020-10-09  2:16 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Jan Kara, linux-ext4, Fengnan Chang, changfengnan

On Thu, Oct 08, 2020 at 02:13:02PM -0600, Andreas Dilger wrote:
> On Oct 7, 2020, at 2:13 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > From: changfengnan <fengnanchang@foxmail.com>
> > 
> > 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.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Fengnan Chang <changfengnan@hikvision.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] jbd2: avoid transaction reuse after reformatting
       [not found]     ` <tencent_909C05B896D07CD8BC84F2EC74B03A1A4007@qq.com>
@ 2020-10-12 15:29       ` Jan Kara
  0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2020-10-12 15:29 UTC (permalink / raw)
  To: 常凤楠
  Cc: Theodore Y. Ts'o, Andreas Dilger, Jan Kara, linux-ext4,
	Fengnan Chang

On Fri 09-10-20 19:06:41, 常凤楠 wrote:
> 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

> ------------------&nbsp;Original&nbsp;------------------
> From:                                                                                                                        "Theodore Y. Ts'o"                                                                                    <tytso@mit.edu&gt;;
> Date:&nbsp;Fri, Oct 9, 2020 10:16 AM
> To:&nbsp;"Andreas Dilger"<adilger@dilger.ca&gt;;
> Cc:&nbsp;"Jan Kara"<jack@suse.cz&gt;;"linux-ext4"<linux-ext4@vger.kernel.org&gt;;"Fengnan Chang"<changfengnan@hikvision.com&gt;;"常凤楠"<fengnanchang@foxmail.com&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 <jack@suse.cz&gt; wrote:
> &gt; &gt; 
> &gt; &gt; From: changfengnan <fengnanchang@foxmail.com&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 <lkp@intel.com&gt;
> &gt; &gt; Signed-off-by: Fengnan Chang <changfengnan@hikvision.com&gt;
> &gt; &gt; Signed-off-by: Jan Kara <jack@suse.cz&gt;
> &gt; 
> &gt; Reviewed-by: Andreas Dilger <adilger@dilger.ca&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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07  8:13 [PATCH v4] jbd2: avoid transaction reuse after reformatting Jan Kara
2020-10-08 20:13 ` Andreas Dilger
2020-10-09  2:16   ` Theodore Y. Ts'o
     [not found]     ` <tencent_909C05B896D07CD8BC84F2EC74B03A1A4007@qq.com>
2020-10-12 15:29       ` Jan Kara

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git