linux-ext4 - Re: [PATCH] jbd2: correct the end of the journal recovery scan range

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230626150538.4n2avgl6hll7lphp@quack3>
Date:   Mon, 26 Jun 2023 17:05:38 +0200
From:   Jan Kara <jack@...e.cz>
To:     Zhang Yi <yi.zhang@...weicloud.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jack@...e.cz,
        harshadshirwadkar@...il.com, yi.zhang@...wei.com,
        yukuai3@...wei.com, chengzhihao1@...wei.com
Subject: Re: [PATCH] jbd2: correct the end of the journal recovery scan range

On Mon 26-06-23 15:33:22, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.com>
> 
> We got a filesystem inconsistency issue below while running generic/475
> I/O failure pressure test with fast_commit feature enabled.
> 
>  Symlink /p3/d3/d1c/d6c/dd6/dce/l101 (inode #132605) is invalid.
> 
> If fast_commit feature is enabled, a special fast_commit journal area is
> appended to the end of the normal journal area. The journal->j_last
> point to the first unused block behind the normal journal area instead
> of the whole log area, and the journal->j_fc_last point to the first
> unused block behind the fast_commit journal area. While doing journal
> recovery, do_one_pass(PASS_SCAN) should first scan the normal journal
> area and turn around to the first block once it meet journal->j_last,
> but the wrap() macro misuse the journal->j_fc_last, so the recovering
> could not read the next magic block (commit block perhaps) and would end
> early mistakenly and missing tN and every transaction after it in the
> following example. Finally, it could lead to filesystem inconsistency.
> 
>  | normal journal area                             | fast commit area |
>  +-------------------------------------------------+------------------+
>  | tN(rere) | tN+1 |~| tN-x |...| tN-1 | tN(front) |       ....       |
>  +-------------------------------------------------+------------------+
>                      /                             /                  /
>                 start               journal->j_last journal->j_fc_last
> 
> This patch fix it by use the correct ending journal->j_last.
> 
> Fixes: 5b849b5f96b4 ("jbd2: fast commit recovery path")
> Reported-by: Theodore Ts'o <tytso@....edu>
> Link: https://lore.kernel.org/linux-ext4/20230613043120.GB1584772@mit.edu/
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>

Ah, great catch! The patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/jbd2/recovery.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 0184931d47f7..c269a7d29a46 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -230,12 +230,8 @@ static int count_tags(journal_t *journal, struct buffer_head *bh)
>  /* Make sure we wrap around the log correctly! */
>  #define wrap(journal, var)						\
>  do {									\
> -	unsigned long _wrap_last =					\
> -		jbd2_has_feature_fast_commit(journal) ?			\
> -			(journal)->j_fc_last : (journal)->j_last;	\
> -									\
> -	if (var >= _wrap_last)						\
> -		var -= (_wrap_last - (journal)->j_first);		\
> +	if (var >= (journal)->j_last)					\
> +		var -= ((journal)->j_last - (journal)->j_first);	\
>  } while (0)
>  
>  static int fc_do_one_pass(journal_t *journal,
> @@ -524,9 +520,7 @@ static int do_one_pass(journal_t *journal,
>  				break;
>  
>  		jbd2_debug(2, "Scanning for sequence ID %u at %lu/%lu\n",
> -			  next_commit_ID, next_log_block,
> -			  jbd2_has_feature_fast_commit(journal) ?
> -			  journal->j_fc_last : journal->j_last);
> +			  next_commit_ID, next_log_block, journal->j_last);
>  
>  		/* Skip over each chunk of the transaction looking
>  		 * either the next descriptor block or the final commit
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR