Re: [PATCH] ext4: cache es->s_journal_inum in ext4_sb_info

[Zhang Yi]

On 2025/3/14 19:41, Ojaswin Mujoo wrote:
> Currently, we access journal ino through sbi->s_es->s_journal_inum,
> which directly reads from the ext4 sb buffer head. If someone modifies
> this underneath us then the s_journal_inum field might get corrupted.
> 
> Although direct block device modifications can be expected to cause
> issues in the FS, let's cache s_journal_inum in sbi->s_journal_ino so
> our checks can be more resillient.
> 
> Suggested-by: Baokun Li <libaokun1@xxxxxxxxxx>
> Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> ---
>  fs/ext4/block_validity.c | 23 ++++++++++++++++-------
>  fs/ext4/ext4.h           |  1 +
>  fs/ext4/inode.c          | 19 +++++++++++++++----
>  fs/ext4/super.c          |  5 ++++-
>  4 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 87ee3a17bd29..54e6f3499263 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -247,9 +247,9 @@ int ext4_setup_system_zone(struct super_block *sb)
>  		if (ret)
>  			goto err;
>  	}
> -	if (ext4_has_feature_journal(sb) && sbi->s_es->s_journal_inum) {
> +	if (ext4_has_feature_journal(sb) && sbi->s_journal_ino) {
>  		ret = ext4_protect_reserved_inode(sb, system_blks,
> -				le32_to_cpu(sbi->s_es->s_journal_inum));
> +						  sbi->s_journal_ino);
>  		if (ret)
>  			goto err;
>  	}
> @@ -351,11 +351,20 @@ int ext4_check_blockref(const char *function, unsigned int line,
>  {
>  	__le32 *bref = p;
>  	unsigned int blk;
> -
> -	if (ext4_has_feature_journal(inode->i_sb) &&
> -	    (inode->i_ino ==
> -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> -		return 0;
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +
> +	if (ext4_has_feature_journal(inode->i_sb)) {
> +		/* If we are called from journal init path then
> +		 * sbi->s_journal_ino would be 0
> +		 */
> +		u32 journal_ino = sbi->s_journal_ino ?
> +					  sbi->s_journal_ino :
> +					  sbi->s_es->s_journal_inum;
> +		WARN_ON_ONCE(journal_ino == 0);
> +
> +		if (inode->i_ino == journal_ino)
> +			return 0;
> +	}
>  

Hello Ojaswin,

I'd suggested to assign s_journal_ino in ext4_open_inode_journal(), so
that we can drop these two complex code snippets in ext4_check_blockref()
and __check_block_validity().

@@ -5856,6 +5856,7 @@ static journal_t *ext4_open_inode_journal(struct super_block *sb,
                return ERR_CAST(journal);
        }
        journal->j_private = sb;
+       EXT4_SB(sb)->s_journal_ino = journal_inum;
        journal->j_bmap = ext4_journal_bmap;
        ext4_init_journal_params(sb, journal);
        return journal;

Thanks,
Yi.

>  	while (bref < p+max) {
>  		blk = le32_to_cpu(*bref++);
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 2b7d781bfcad..648b0459e9fd 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1556,6 +1556,7 @@ struct ext4_sb_info {
>  	u32 s_max_batch_time;
>  	u32 s_min_batch_time;
>  	struct file *s_journal_bdev_file;
> +	u32 s_journal_ino;
>  #ifdef CONFIG_QUOTA
>  	/* Names of quota files with journalled quota */
>  	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 365d31004bd0..50961bc0c94d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -384,10 +384,21 @@ static int __check_block_validity(struct inode *inode, const char *func,
>  				unsigned int line,
>  				struct ext4_map_blocks *map)
>  {
> -	if (ext4_has_feature_journal(inode->i_sb) &&
> -	    (inode->i_ino ==
> -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> -		return 0;
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +
> +	if (ext4_has_feature_journal(inode->i_sb)) {
> +		/*
> +		 * If we are called from journal init path then
> +		 * sbi->s_journal_ino would be 0
> +		 */
> +		u32 journal_ino = sbi->s_journal_ino ?
> +					  sbi->s_journal_ino :
> +					  sbi->s_es->s_journal_inum;
> +		WARN_ON_ONCE(journal_ino == 0);
> +
> +		if (inode->i_ino == journal_ino)
> +			return 0;
> +	}
>  	if (!ext4_inode_block_valid(inode, map->m_pblk, map->m_len)) {
>  		ext4_error_inode(inode, func, line, map->m_pblk,
>  				 "lblock %lu mapped to illegal pblock %llu "
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a963ffda692a..44e79db7f12a 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4162,7 +4162,8 @@ int ext4_calculate_overhead(struct super_block *sb)
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct ext4_super_block *es = sbi->s_es;
>  	struct inode *j_inode;
> -	unsigned int j_blocks, j_inum = le32_to_cpu(es->s_journal_inum);
> +	unsigned int j_blocks;
> +	u32 j_inum = sbi->s_journal_ino;
>  	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
>  	ext4_fsblk_t overhead = 0;
>  	char *buf = (char *) get_zeroed_page(GFP_NOFS);
> @@ -6091,6 +6092,8 @@ static int ext4_load_journal(struct super_block *sb,
>  		ext4_commit_super(sb);
>  	}
>  
> +	EXT4_SB(sb)->s_journal_ino = le32_to_cpu(es->s_journal_inum);
> +
>  	return 0;
>  
>  err_out: