| Message ID | 20201007081319.16341-1-jack@suse.cz |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [v4] jbd2: avoid transaction reuse after reformatting | expand |
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
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
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 > ------------------ Original ------------------ > From: "Theodore Y. Ts'o" <tytso@mit.edu>; > Date: Fri, Oct 9, 2020 10:16 AM > To: "Andreas Dilger"<adilger@dilger.ca>; > Cc: "Jan Kara"<jack@suse.cz>;"linux-ext4"<linux-ext4@vger.kernel.org>;"Fengnan Chang"<changfengnan@hikvision.com>;"常凤楠"<fengnanchang@foxmail.com>; > Subject: Re: [PATCH v4] jbd2: avoid transaction reuse after reformatting > > > > 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
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)