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

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

Posted by Ojaswin Mujoo 8 months, 3 weeks ago

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

Posted by Baokun Li 8 months, 3 weeks ago

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

Posted by Zhang Yi 8 months, 3 weeks ago

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

Posted by Jan Kara 8 months, 3 weeks ago

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

Posted by Ojaswin Mujoo 8 months, 3 weeks ago

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