[PATCH] ext4: return an error when a journal block is not mapped

[PATCH] ext4: return an error when a journal block is not mapped

[Karl Mehltretter]

If the internal journal inode contains a hole, ext4_map_blocks() returns
0. ext4_journal_bmap() logs "journal bmap failed" and aborts the
journal, but also returns 0 and leaves *block unchanged.

jbd2_journal_bmap() takes that as success and uses the unchanged logical
journal block number as a physical filesystem block number. Journal I/O
can then overwrite filesystem blocks outside the journal despite the
abort.

Return -EFSCORRUPTED for a hole, matching the error passed to
jbd2_journal_abort(). Keep propagating negative mapping errors.

Fixes: 62913ae96de7 ("ext4, jbd2: add an optimized bmap for the journal inode")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
Found with an experimental clang that warns when a function returns 0
right after reporting a failure.

Recorded QEMU A/B testing used x86_64 kernels and a 1 KiB-block image
with a hole in the journal inode starting at logical block 40. The
unpatched kernel overwrote filesystem block 40, a reserved GDT block.
With the patch, none of the marker blocks changed. There were three
runs per kernel.

Both kernels logged the mapping failure and journal abort:

  EXT4-fs (vda): journal bmap failed: block 40 ret 0
  Aborting journal on device vda-8.

 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index bca0dc87d0b7..3e7dabe45153 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5967,7 +5967,7 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
 			 "journal bmap failed: block %llu ret %d\n",
 			 *block, ret);
 		jbd2_journal_abort(journal, ret ? ret : -EFSCORRUPTED);
-		return ret;
+		return ret ? ret : -EFSCORRUPTED;
 	}
 	*block = map.m_pblk;
 	return 0;

base-commit: e767a4ea70a3992c37ed604157d32f0dfbf9b1e3
-- 
2.53.0

Re: [PATCH] ext4: return an error when a journal block is not mapped

[Jan Kara]

On Sun 04-10-26 20:06:29, Karl Mehltretter wrote:
> If the internal journal inode contains a hole, ext4_map_blocks() returns
> 0. ext4_journal_bmap() logs "journal bmap failed" and aborts the
> journal, but also returns 0 and leaves *block unchanged.
> 
> jbd2_journal_bmap() takes that as success and uses the unchanged logical
> journal block number as a physical filesystem block number. Journal I/O
> can then overwrite filesystem blocks outside the journal despite the
> abort.
> 
> Return -EFSCORRUPTED for a hole, matching the error passed to
> jbd2_journal_abort(). Keep propagating negative mapping errors.
> 
> Fixes: 62913ae96de7 ("ext4, jbd2: add an optimized bmap for the journal inode")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Makes sense. Feel free to add:

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

								Honza

> ---
> Found with an experimental clang that warns when a function returns 0
> right after reporting a failure.
> 
> Recorded QEMU A/B testing used x86_64 kernels and a 1 KiB-block image
> with a hole in the journal inode starting at logical block 40. The
> unpatched kernel overwrote filesystem block 40, a reserved GDT block.
> With the patch, none of the marker blocks changed. There were three
> runs per kernel.
> 
> Both kernels logged the mapping failure and journal abort:
> 
>   EXT4-fs (vda): journal bmap failed: block 40 ret 0
>   Aborting journal on device vda-8.
> 
>  fs/ext4/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index bca0dc87d0b7..3e7dabe45153 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5967,7 +5967,7 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
>  			 "journal bmap failed: block %llu ret %d\n",
>  			 *block, ret);
>  		jbd2_journal_abort(journal, ret ? ret : -EFSCORRUPTED);
> -		return ret;
> +		return ret ? ret : -EFSCORRUPTED;
>  	}
>  	*block = map.m_pblk;
>  	return 0;
> 
> base-commit: e767a4ea70a3992c37ed604157d32f0dfbf9b1e3
> -- 
> 2.53.0
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: return an error when a journal block is not mapped

[Zhang Yi]

On 10/5/2026 2:06 AM, Karl Mehltretter wrote:
> If the internal journal inode contains a hole, ext4_map_blocks() returns
> 0. ext4_journal_bmap() logs "journal bmap failed" and aborts the
> journal, but also returns 0 and leaves *block unchanged.
> 
> jbd2_journal_bmap() takes that as success and uses the unchanged logical
> journal block number as a physical filesystem block number. Journal I/O
> can then overwrite filesystem blocks outside the journal despite the
> abort.
> 
> Return -EFSCORRUPTED for a hole, matching the error passed to
> jbd2_journal_abort(). Keep propagating negative mapping errors.
> 
> Fixes: 62913ae96de7 ("ext4, jbd2: add an optimized bmap for the journal inode")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Looks good to me.

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

> ---
> Found with an experimental clang that warns when a function returns 0
> right after reporting a failure.
> 
> Recorded QEMU A/B testing used x86_64 kernels and a 1 KiB-block image
> with a hole in the journal inode starting at logical block 40. The
> unpatched kernel overwrote filesystem block 40, a reserved GDT block.
> With the patch, none of the marker blocks changed. There were three
> runs per kernel.
> 
> Both kernels logged the mapping failure and journal abort:
> 
>    EXT4-fs (vda): journal bmap failed: block 40 ret 0
>    Aborting journal on device vda-8.
> 
>   fs/ext4/super.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index bca0dc87d0b7..3e7dabe45153 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5967,7 +5967,7 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
>   			 "journal bmap failed: block %llu ret %d\n",
>   			 *block, ret);
>   		jbd2_journal_abort(journal, ret ? ret : -EFSCORRUPTED);
> -		return ret;
> +		return ret ? ret : -EFSCORRUPTED;
>   	}
>   	*block = map.m_pblk;
>   	return 0;
> 
> base-commit: e767a4ea70a3992c37ed604157d32f0dfbf9b1e3

Re: [PATCH] ext4: return an error when a journal block is not mapped

[Markus Elfring]

…
> +++ b/fs/ext4/super.c
> @@ -5967,7 +5967,7 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
>  			 "journal bmap failed: block %llu ret %d\n",
>  			 *block, ret);
>  		jbd2_journal_abort(journal, ret ? ret : -EFSCORRUPTED);
> -		return ret;
> +		return ret ? ret : -EFSCORRUPTED;
>  	}
>  	*block = map.m_pblk;
>  	return 0;

Would you expect that an optimiser will avoid a duplicate expression here?
How do you think about to adjust this implementation detail another bit?

Regards,
Markus

Re: [PATCH] ext4: return an error when a journal block is not mapped

[Jan Kara]

On Thu 08-10-26 10:32:06, Markus Elfring wrote:
> …
> > +++ b/fs/ext4/super.c
> > @@ -5967,7 +5967,7 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
> >  			 "journal bmap failed: block %llu ret %d\n",
> >  			 *block, ret);
> >  		jbd2_journal_abort(journal, ret ? ret : -EFSCORRUPTED);
> > -		return ret;
> > +		return ret ? ret : -EFSCORRUPTED;
> >  	}
> >  	*block = map.m_pblk;
> >  	return 0;
> 
> Would you expect that an optimiser will avoid a duplicate expression here?
> How do you think about to adjust this implementation detail another bit?

I don't think we care about a duplicate expression in this path but yes,
perhaps doing:
	if (!ret)
		ret = -EFSCORRUPTED;
before calling jbd2_journal_abort() would be a tad bit cleaner.

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