[PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[Ojaswin Mujoo]

Block validity checks need to be skipped in case they are called
for journal blocks since they are part of system's protected
zone.

Currently, this is done by checking inode->ino against
sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
buffer head. If someone modifies this underneath us then the
s_journal_inum field might get corrupted. To prevent against this,
change the check to directly compare the inode with journal->j_inode.

**Slight change in behavior**: During journal init path,
check_block_validity etc might be called for journal inode when
sbi->s_journal is not set yet. In this case we now proceed with
ext4_inode_block_valid() instead of returning early. Since systems zones
have not been set yet, it is okay to proceed so we can perform basic
checks on the blocks.

Suggested-by: Baokun Li <libaokun1@huawei.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---

** Changes since v1 [1] **

- instead of using an sbi field direction check against jorunal->j_inode
- let block validity perform basic checks on journal blocks as well
	during init path
- kvm-xfstests quick tests are passing

[1] https://lore.kernel.org/linux-ext4/d1a9328a41029f6210a1924b192a59afcd3c5cee.1741952406.git.ojaswin@linux.ibm.com/

 fs/ext4/block_validity.c | 5 ++---
 fs/ext4/inode.c          | 9 +++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 87ee3a17bd29..e8c5525afc67 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
 {
 	__le32 *bref = p;
 	unsigned int blk;
+	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
 
-	if (ext4_has_feature_journal(inode->i_sb) &&
-	    (inode->i_ino ==
-	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
+	if (journal && inode == journal->j_inode)
 		return 0;
 
 	while (bref < p+max) {
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 365d31004bd0..8b048be14008 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -384,10 +384,11 @@ 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;
+	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
+
+	if (journal && inode == journal->j_inode)
+			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 "
-- 
2.48.1

Re: [PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[Jan Kara]

On Thu 27-03-25 23:18:09, Ojaswin Mujoo wrote:
> Block validity checks need to be skipped in case they are called
> for journal blocks since they are part of system's protected
> zone.
> 
> Currently, this is done by checking inode->ino against
> sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
> buffer head. If someone modifies this underneath us then the
> s_journal_inum field might get corrupted. To prevent against this,
> change the check to directly compare the inode with journal->j_inode.
> 
> **Slight change in behavior**: During journal init path,
> check_block_validity etc might be called for journal inode when
> sbi->s_journal is not set yet. In this case we now proceed with
> ext4_inode_block_valid() instead of returning early. Since systems zones
> have not been set yet, it is okay to proceed so we can perform basic
> checks on the blocks.
> 
> Suggested-by: Baokun Li <libaokun1@huawei.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Looks good. Feel free to add:

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

One style nit below:

> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 87ee3a17bd29..e8c5525afc67 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
>  {
>  	__le32 *bref = p;
>  	unsigned int blk;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
>  
> -	if (ext4_has_feature_journal(inode->i_sb) &&
> -	    (inode->i_ino ==
> -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> +	if (journal && inode == journal->j_inode)
>  		return 0;
>  
>  	while (bref < p+max) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 365d31004bd0..8b048be14008 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -384,10 +384,11 @@ 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;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> +
> +	if (journal && inode == journal->j_inode)
> +			return 0;

Bogus indentation here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[Zhang Yi]

On 2025/3/28 1:48, Ojaswin Mujoo wrote:
> Block validity checks need to be skipped in case they are called
> for journal blocks since they are part of system's protected
> zone.
> 
> Currently, this is done by checking inode->ino against
> sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
> buffer head. If someone modifies this underneath us then the
> s_journal_inum field might get corrupted. To prevent against this,
> change the check to directly compare the inode with journal->j_inode.
> 
> **Slight change in behavior**: During journal init path,
> check_block_validity etc might be called for journal inode when
> sbi->s_journal is not set yet. In this case we now proceed with
> ext4_inode_block_valid() instead of returning early. Since systems zones
> have not been set yet, it is okay to proceed so we can perform basic
> checks on the blocks.
> 
> Suggested-by: Baokun Li <libaokun1@huawei.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Thanks for the inprovement! Besides the indentation that Jan pointed
out, it looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
> 
> ** Changes since v1 [1] **
> 
> - instead of using an sbi field direction check against jorunal->j_inode
> - let block validity perform basic checks on journal blocks as well
> 	during init path
> - kvm-xfstests quick tests are passing
> 
> [1] https://lore.kernel.org/linux-ext4/d1a9328a41029f6210a1924b192a59afcd3c5cee.1741952406.git.ojaswin@linux.ibm.com/
> 
>  fs/ext4/block_validity.c | 5 ++---
>  fs/ext4/inode.c          | 9 +++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 87ee3a17bd29..e8c5525afc67 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
>  {
>  	__le32 *bref = p;
>  	unsigned int blk;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
>  
> -	if (ext4_has_feature_journal(inode->i_sb) &&
> -	    (inode->i_ino ==
> -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> +	if (journal && inode == journal->j_inode)
>  		return 0;
>  
>  	while (bref < p+max) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 365d31004bd0..8b048be14008 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -384,10 +384,11 @@ 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;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> +
> +	if (journal && inode == journal->j_inode)
> +			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 "

Re: [PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[Baokun Li]

On 2025/3/28 1:48, Ojaswin Mujoo wrote:
> Block validity checks need to be skipped in case they are called
> for journal blocks since they are part of system's protected
> zone.
>
> Currently, this is done by checking inode->ino against
> sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
> buffer head. If someone modifies this underneath us then the
> s_journal_inum field might get corrupted. To prevent against this,
> change the check to directly compare the inode with journal->j_inode.
>
> **Slight change in behavior**: During journal init path,
> check_block_validity etc might be called for journal inode when
> sbi->s_journal is not set yet. In this case we now proceed with
> ext4_inode_block_valid() instead of returning early. Since systems zones
> have not been set yet, it is okay to proceed so we can perform basic
> checks on the blocks.
>
> Suggested-by: Baokun Li <libaokun1@huawei.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Thanks for the patch！Looks good to me, except for the extra indentation
pointed out by Honza. With that fixed feel free to add:

Reviewed-by: Baokun Li <libaokun1@huawei.com>


Cheers,
Baokun
> ---
>
> ** Changes since v1 [1] **
>
> - instead of using an sbi field direction check against jorunal->j_inode
> - let block validity perform basic checks on journal blocks as well
> 	during init path
> - kvm-xfstests quick tests are passing
>
> [1] https://lore.kernel.org/linux-ext4/d1a9328a41029f6210a1924b192a59afcd3c5cee.1741952406.git.ojaswin@linux.ibm.com/
>
>   fs/ext4/block_validity.c | 5 ++---
>   fs/ext4/inode.c          | 9 +++++----
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 87ee3a17bd29..e8c5525afc67 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
>   {
>   	__le32 *bref = p;
>   	unsigned int blk;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
>   
> -	if (ext4_has_feature_journal(inode->i_sb) &&
> -	    (inode->i_ino ==
> -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> +	if (journal && inode == journal->j_inode)
>   		return 0;
>   
>   	while (bref < p+max) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 365d31004bd0..8b048be14008 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -384,10 +384,11 @@ 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;
> +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> +
> +	if (journal && inode == journal->j_inode)
> +			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 "

Re: [PATCH v2] ext4: Make block validity check resistent to sb bh corruption

[Ojaswin Mujoo]

On Thu, Mar 27, 2025 at 07:25:30PM +0100, Jan Kara wrote:
> On Thu 27-03-25 23:18:09, Ojaswin Mujoo wrote:
> > Block validity checks need to be skipped in case they are called
> > for journal blocks since they are part of system's protected
> > zone.
> > 
> > Currently, this is done by checking inode->ino against
> > sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
> > buffer head. If someone modifies this underneath us then the
> > s_journal_inum field might get corrupted. To prevent against this,
> > change the check to directly compare the inode with journal->j_inode.
> > 
> > **Slight change in behavior**: During journal init path,
> > check_block_validity etc might be called for journal inode when
> > sbi->s_journal is not set yet. In this case we now proceed with
> > ext4_inode_block_valid() instead of returning early. Since systems zones
> > have not been set yet, it is okay to proceed so we can perform basic
> > checks on the blocks.
> > 
> > Suggested-by: Baokun Li <libaokun1@huawei.com>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> One style nit below:
> 
> > diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> > index 87ee3a17bd29..e8c5525afc67 100644
> > --- a/fs/ext4/block_validity.c
> > +++ b/fs/ext4/block_validity.c
> > @@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
> >  {
> >  	__le32 *bref = p;
> >  	unsigned int blk;
> > +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> >  
> > -	if (ext4_has_feature_journal(inode->i_sb) &&
> > -	    (inode->i_ino ==
> > -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> > +	if (journal && inode == journal->j_inode)
> >  		return 0;
> >  
> >  	while (bref < p+max) {
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 365d31004bd0..8b048be14008 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -384,10 +384,11 @@ 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;
> > +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> > +
> > +	if (journal && inode == journal->j_inode)
> > +			return 0;
> 
> Bogus indentation here.

Thanks for the review Jan and for catching this. My bad, i missed
running checkpatch on this and somehow this looked okay in vim.

I'll fix this and quickly send a v3.

Regards,
ojaswin
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR