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:
