linux-ext4 - Re: [PATCH] jbd2: avoid transaction reuse after reformatting

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>] [day] [month] [year] [list]
Date:   Thu, 10 Sep 2020 12:45:34 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     changfengnan <changfengnan@...com>
Cc:     darrick.wong@...cle.com, jack@...e.com, linux-ext4@...r.kernel.org,
        tytso@....edu, Fengnan Chang <changfengnan@...vision.com>
Subject: Re: [PATCH] jbd2: avoid transaction reuse after reformatting

On Sep 10, 2020, at 5:55 AM, changfengnan <changfengnan@...com> wrote:
> 
> From: Fengnan Chang <changfengnan@...vision.com>
> 
> When format ext4 with lazy_journal_init=1, the previous transaction is
> still on disk, it is possible that the previous transaction will be
> used again during jbd2 recovery.Because the seed is changed, the CRC
> check will fail.
> 
> Signed-off-by: Fengnan Chang <changfengnan@...vision.com>

This version of the patch looks OK, with one trivial indentation problem
after the "jbd_debug(1, "JBD2: Invalid checksum" line that could be fixed.

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

> ---
> fs/jbd2/recovery.c | 60 +++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index a4967b27ffb6..8a6bc322a06d 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -33,6 +33,8 @@ struct recovery_info
> 	int		nr_replays;
> 	int		nr_revokes;
> 	int		nr_revoke_hits;
> +	unsigned long  ri_commit_block;
> +	__be64  last_trans_commit_time;
> };
> 
> enum passtype {PASS_SCAN, PASS_REVOKE, PASS_REPLAY};
> @@ -412,7 +414,27 @@ static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
> 	else
> 		return tag->t_checksum == cpu_to_be16(csum32);
> }
> +/*
> + * We check the commit time and compare it with the commit time of
> + * the previous transaction, if it's smaller than previous,
> + * We think it's not belong to same journal.
> + */
> +static bool is_same_journal(journal_t *journal, struct buffer_head *bh, unsigned long blocknr, __u64 last_commit_sec)
> +{
> +	unsigned long commit_block = blocknr + count_tags(journal, bh) + 1;
> +	struct buffer_head *nbh;
> +	struct commit_header *cbh;
> +	__u64	commit_sec;
> +	int err = jread(&nbh, journal, commit_block);
> 
> +	if (err)
> +		return true;
> +
> +	cbh = (struct commit_header *)nbh->b_data;
> +	commit_sec = be64_to_cpu(cbh->h_commit_sec);
> +
> +	return commit_sec >= last_commit_sec;
> +}
> static int do_one_pass(journal_t *journal,
> 			struct recovery_info *info, enum passtype pass)
> {
> @@ -514,18 +536,29 @@ static int do_one_pass(journal_t *journal,
> 		switch(blocktype) {
> 		case JBD2_DESCRIPTOR_BLOCK:
> 			/* Verify checksum first */
> +			if (pass == PASS_SCAN)
> +				info->ri_commit_block = 0;
> +
> 			if (jbd2_journal_has_csum_v2or3(journal))
> 				descr_csum_size =
> 					sizeof(struct jbd2_journal_block_tail);
> 			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",
> +				if (is_same_journal(journal, bh, next_log_block-1, info->last_trans_commit_time)) {
> +					printk(KERN_ERR "JBD2: Invalid checksum recovering block %lu in log\n",
> 				       next_log_block);
> -				err = -EFSBADCRC;
> -				brelse(bh);
> -				goto failed;
> +					err = -EFSBADCRC;
> +					brelse(bh);
> +					goto failed;
> +				} else {
> +					/*it's not belong to same journal, just end this recovery with success*/
> +					jbd_debug(1, "JBD2: Invalid checksum found in block %lu in log, but not same journal %d\n",
> +				       next_log_block, next_commit_ID);

The continued line should be indented one more tab.

> +					err = 0;
> +					brelse(bh);
> +					goto done;
> +				}
> 			}
> 
> 			/* If it is a valid descriptor block, replay it
> @@ -688,6 +721,17 @@ static int do_one_pass(journal_t *journal,
> 			 * are present verify them in PASS_SCAN; else not
> 			 * much to do other than move on to the next sequence
> 			 * number. */
> +			if (pass == PASS_SCAN) {
> +				struct commit_header *cbh =
> +					(struct commit_header *)bh->b_data;
> +				if (info->ri_commit_block) {
> +					jbd_debug(1, "invalid commit block found in %lu, stop here.\n", next_log_block);
> +					brelse(bh);
> +					goto done;
> +				}
> +				info->ri_commit_block = next_log_block;
> +				info->last_trans_commit_time = be64_to_cpu(cbh->h_commit_sec);
> +			}
> 			if (pass == PASS_SCAN &&
> 			    jbd2_has_feature_checksum(journal)) {
> 				int chksum_err, chksum_seen;
> @@ -761,7 +805,11 @@ static int do_one_pass(journal_t *journal,
> 				brelse(bh);
> 				continue;
> 			}
> -
> +			if (pass != PASS_SCAN && info->ri_commit_block) {
> +				jbd_debug(1, "invalid revoke block found in %lu, stop here.\n", next_log_block);
> +				brelse(bh);
> +				goto done;
> +			}
> 			err = scan_revoke_records(journal, bh,
> 						  next_commit_ID, info);
> 			brelse(bh);
> --
> 2.27.0.windows.1
> 
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)