[PATCH 1/3] ext4: check return value of blkdev_issue_flush() when initializing inode tables

[PATCH 1/3] ext4: check return value of blkdev_issue_flush() when initializing inode tables

[guolingxing]

When lazily zeroing an inode table, ext4_init_inode_table() issues a
cache flush to make sure the zeroed blocks have reached the disk
before setting the EXT4_BG_INODE_ZEROED flag, but the return value is
ignored.  If the cache flush fails, the inode table is still marked as
initialized, and a subsequent crash can leave the table containing
stale data from the previous use of the disk, which may then be
exposed to user space through newly allocated inodes.

Check the return value of blkdev_issue_flush() and leave
EXT4_BG_INODE_ZEROED unset if the cache flush fails, so that the inode
table zeroing is retried on the next mount.

Fixes: a107e5a3a473a ("Merge branch 'next' into upstream-merge")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/ialloc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index a5831fc536db..9abd7fb75b02 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1610,8 +1610,11 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
 	ret = sb_issue_zeroout(sb, blk, num, GFP_NOFS);
 	if (ret < 0)
 		goto err_out;
-	if (barrier)
-		blkdev_issue_flush(sb->s_bdev);
+	if (barrier) {
+		ret = blkdev_issue_flush(sb->s_bdev);
+		if (ret)
+			goto err_out;
+	}
 
 skip_zeroout:
 	ext4_lock_group(sb, group);
-- 
2.34.1

[PATCH 2/3] ext4: check return value of blkdev_issue_flush() when starting a fast commit

[guolingxing]

When the file system device differs from the journal device, a cache
flush is issued on the file system device before fast commit blocks
are written, but the return value is ignored.  If the cache flush
fails, the fast commit blocks are written and the fast commit
completes anyway, even though the updates covered by the preceding
transactions may not be on stable storage yet, so the fast commit may
provide guarantees it cannot keep.

Check the return value of blkdev_issue_flush() and abort the fast
commit if the cache flush fails.  ext4_fc_commit() will then fall back
to a full journal commit, so the caller still gets the expected
durability guarantees.

Fixes: da0c5d2695265 ("ext4: issue fsdev cache flush before starting fast commit")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/fast_commit.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 0cac890cf370..ead8b16f9be0 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -1425,8 +1425,11 @@ static int ext4_fc_perform_commit(journal_t *journal, tid_t commit_tid)
 	 * Step 5: If file system device is different from journal device,
 	 * issue a cache flush before we start writing fast commit blocks.
 	 */
-	if (journal->j_fs_dev != journal->j_dev)
-		blkdev_issue_flush(journal->j_fs_dev);
+	if (journal->j_fs_dev != journal->j_dev) {
+		ret = blkdev_issue_flush(journal->j_fs_dev);
+		if (ret)
+			return ret;
+	}
 
 	blk_start_plug(&plug);
 	alloc_ctx = ext4_fc_lock(sb);
-- 
2.34.1

Re: [PATCH 2/3] ext4: check return value of blkdev_issue_flush() when starting a fast commit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ext4: missing JBD2_BARRIER check for fast commit device flush
--

commit a1cae98067fdb2cd8eec738283b6d33c84b7ca8d
Author: guolingxing <guolingxing@tju.edu.cn>

ext4: check return value of blkdev_issue_flush() when starting a fast commit

When the file system device differs from the journal device, a cache flush
is issued before fast commit blocks are written. The return value of
blkdev_issue_flush() was previously ignored. This patch checks the return
value and aborts the fast commit if the flush fails, falling back to a full
journal commit.

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 0cac890cf3702..ead8b16f9be07 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1425,8 +1425,11 @@ static int ext4_fc_perform_commit(journal_t *journal, tid_t commit_tid)
>  	 * Step 5: If file system device is different from journal device,
>  	 * issue a cache flush before we start writing fast commit blocks.
>  	 */
> -	if (journal->j_fs_dev != journal->j_dev)
> -		blkdev_issue_flush(journal->j_fs_dev);
> +	if (journal->j_fs_dev != journal->j_dev) {
> +		ret = blkdev_issue_flush(journal->j_fs_dev);
> +		if (ret)
> +			return ret;
> +	}

[Severity: Medium]
This problem wasn't introduced by this patch, but does this flush need to
validate journal->j_flags & JBD2_BARRIER before proceeding?

In ext4_fc_perform_commit(), blkdev_issue_flush() is called unconditionally
when the filesystem device differs from the journal device. If the filesystem
is mounted with the "nobarrier" option, this appears to ignore the mount
option and issue the cache flush anyway, unlike the JBD2 full commit path and
fast commit tail block writers.

Could this bypass the user's mount options and cause unnecessary I/O
overhead during fast commits?

>  
>  	blk_start_plug(&plug);
>  	alloc_ctx = ext4_fc_lock(sb);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013062618.1741670-1-guolingxing@tju.edu.cn?part=2

[PATCH 3/3] ext4: check return value of blkdev_issue_flush() when replaying fast commits

[guolingxing]

ext4_fc_replay_inode() writes the replayed inode to disk and then
issues a cache flush, but the return value is ignored.  If the cache
flush fails, the fast commit replay continues and the mount may
succeed, even though the replayed inode contents are not guaranteed to
be on stable storage yet.

Check the return value of blkdev_issue_flush() and abort the fast
commit replay if the cache flush fails, like any other error detected
while replaying the inode.  This causes the mount to fail instead of
silently succeeding with potentially unreplayed data.

Fixes: 8016e29f4362e ("ext4: fast commit recovery path")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/fast_commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index ead8b16f9be0..f554405ebabf 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -2019,7 +2019,7 @@ static int ext4_fc_replay_inode(struct super_block *sb,
 out:
 	iput(inode);
 	if (!ret)
-		blkdev_issue_flush(sb->s_bdev);
+		ret = blkdev_issue_flush(sb->s_bdev);
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH 3/3] ext4: check return value of blkdev_issue_flush() when replaying fast commits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ext4: regression causing mount failures on devices lacking flush support
- [Medium] ext4: severe mount delay due to O(N) per-inode fast commit cache flush
--

commit 1d9ac3ed6ced15861bdc5e6964915fbce6a1b1e6
Author: guolingxing <guolingxing@tju.edu.cn>

ext4: check return value of blkdev_issue_flush() when replaying fast commits

This patch updates ext4_fc_replay_inode() to check and return the error
code from blkdev_issue_flush(). It intends to abort the fast commit replay
and fail the mount if the cache flush fails.

Cc: stable@vger.kernel.org

> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index ead8b16f9be07..f554405ebabfe 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -2019,7 +2019,7 @@ static int ext4_fc_replay_inode(struct super_block *sb,
>  out:
>  	iput(inode);
>  	if (!ret)
> -		blkdev_issue_flush(sb->s_bdev);
> +		ret = blkdev_issue_flush(sb->s_bdev);

[Severity: High]
Does this code introduce a regression for devices that do not support cache
flushes?

If the underlying block device (such as certain loop devices, ramdisks,
or older flash media) does not support flushes, blkdev_issue_flush() will
return -EOPNOTSUPP. Ext4 usually ignores -EOPNOTSUPP for flushes, but this
change unconditionally captures and returns the error code to the fast
commit replay mechanism.

As a result, fast commit replay will abort and the entire filesystem mount
will fail on these devices.

[Severity: Medium]
Can this flush cause severe mount delays during fast commit recovery?

Because ext4_fc_replay_inode() is called for every inode in the fast commit
journal, this will trigger an O(N) number of full device cache flushes.
This can cause significant performance degradation when processing a journal
with many EXT4_FC_TAG_INODE tags.

Additionally, is this flush structurally redundant? JBD2 already guarantees
a final blkdev_issue_flush() on the filesystem device (journal->j_fs_dev)
in jbd2_journal_recover() after all recovery passes (including fast commit
replay) are complete:

fs/jbd2/recovery.c:jbd2_journal_recover() {
    ...
    /* Make sure all replayed data is on permanent storage */
    if (journal->j_flags & JBD2_BARRIER) {
        err2 = blkdev_issue_flush(journal->j_fs_dev);
        if (!err)
            err = err2;
    }
    ...
}

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013062618.1741670-1-guolingxing@tju.edu.cn?part=3

Re: [PATCH 1/3] ext4: check return value of blkdev_issue_flush() when initializing inode tables

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ext4: lazyinit thread ignores nobarrier mount option and aborts on flush error
--

commit 06e54e3a165a1e8b6269f6dbdf6487defdbf7915
Author: guolingxing <guolingxing@tju.edu.cn>

ext4: check return value of blkdev_issue_flush() when initializing inode tables

This commit checks the return value of blkdev_issue_flush() when lazily
zeroing inode tables to prevent exposing stale data if the flush fails.
By leaving the EXT4_BG_INODE_ZEROED flag unset on failure, it ensures
the zeroing is retried on the next mount.

> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index f97a2f4904ebb..843a78fc6e51a 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c

[ ... ]

> @@ -1611,8 +1611,11 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
>  	ret = sb_issue_zeroout(sb, blk, num, GFP_NOFS);
>  	if (ret < 0)
>  		goto err_out;
> -	if (barrier)
> -		blkdev_issue_flush(sb->s_bdev);
> +	if (barrier) {
> +		ret = blkdev_issue_flush(sb->s_bdev);
> +		if (ret)
> +			goto err_out;
> +	}

[Severity: Medium]
Does this change break filesystem initialization for users who mount with
the nobarrier option on devices with broken write caches?

In ext4_run_li_request() in fs/ext4/super.c, the lazyinit thread calls
ext4_init_inode_table() with the barrier argument set unconditionally
to 1 for the first block group, ignoring the mount options:

    ret = ext4_init_inode_table(sb, group,
                                elr->lr_timeout ? 0 : 1);

If a user mounts with -o nobarrier because their block device returns errors
for flush requests, this timing measurement flush will fail. Before this
patch, the flush error was safely ignored.

Now that the return value is checked, will this cause the lazy inode table
initialization thread to permanently abort and leave the inode tables
un-zeroed across mounts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013062618.1741670-1-guolingxing@tju.edu.cn?part=1

[PATCH v2 0/2] ext4: check return value of blkdev_issue_flush() in unchecked call sites

[guolingxing]

This series adds return value checks to the blkdev_issue_flush() call
sites in ext4 which ignore the return value: lazy inode table
initialization and the file system device flush issued before
starting a fast commit.  Both call sites also issue the flush without
checking the barrier mount option; the series makes them honor the
option, consistent with the other cache flushes in ext4 and jbd2.

Changes since v1:
- Drop the fast commit replay patch.  JBD2 already calls
  sync_blockdev() on the file system device, checks the device write
  error, and issues a final barrier-gated cache flush after journal
  and fast commit replay, and it propagates all of these errors to
  the mount.  The error check added by that patch was therefore
  redundant; the pre-existing per-inode flush in the replay path is
  left as is for now.
- Gate the remaining two cache flushes on the barrier mount option
  (JBD2_BARRIER for the fast commit path).  Checking the return value
  makes a cache flush failure fatal for lazy inode table
  initialization and fast commits; without the gating, this would
  regress file systems mounted with nobarrier on devices with failing
  cache flushes.
- Update the patch descriptions to describe the gating.

The patches are otherwise unchanged since v1.

guolingxing (2):
  ext4: check return value of blkdev_issue_flush() when initializing
    inode tables
  ext4: check return value of blkdev_issue_flush() when starting a fast
    commit

 fs/ext4/fast_commit.c | 8 ++++++--
 fs/ext4/ialloc.c      | 7 +++++--
 fs/ext4/super.c       | 3 ++-
 3 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] ext4: check return value of blkdev_issue_flush() when initializing inode tables

[guolingxing]

When lazily zeroing an inode table, ext4_init_inode_table() issues a
cache flush to make sure the zeroed blocks have reached the disk
before setting the EXT4_BG_INODE_ZEROED flag, but the return value is
ignored.  If the cache flush fails, the inode table is still marked as
initialized, and a subsequent crash can leave the table containing
stale data from the previous use of the disk, which may then be
exposed to user space through newly allocated inodes.

Check the return value of blkdev_issue_flush() and leave
EXT4_BG_INODE_ZEROED unset if the cache flush fails, so that the inode
table zeroing is retried on the next mount.

Since a cache flush failure now aborts the lazy inode table
initialization, also pass the barrier mount option down to
ext4_init_inode_table(), consistent with the other cache flushes in
ext4 and jbd2 which are issued only when the barrier option is
enabled.  This keeps file systems mounted with nobarrier working on
devices with failing cache flushes.

Fixes: a107e5a3a473a ("Merge branch 'next' into upstream-merge")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/ialloc.c | 7 +++++--
 fs/ext4/super.c  | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index a5831fc536db..9abd7fb75b02 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1610,8 +1610,11 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
 	ret = sb_issue_zeroout(sb, blk, num, GFP_NOFS);
 	if (ret < 0)
 		goto err_out;
-	if (barrier)
-		blkdev_issue_flush(sb->s_bdev);
+	if (barrier) {
+		ret = blkdev_issue_flush(sb->s_bdev);
+		if (ret)
+			goto err_out;
+	}
 
 skip_zeroout:
 	ext4_lock_group(sb, group);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d636b89090a2..941af6fd050c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3793,7 +3793,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
 	if (!ret) {
 		start_time = ktime_get_ns();
 		ret = ext4_init_inode_table(sb, group,
-					    elr->lr_timeout ? 0 : 1);
+					    !elr->lr_timeout &&
+					    test_opt(sb, BARRIER));
 		trace_ext4_lazy_itable_init(sb, group);
 		if (elr->lr_timeout == 0) {
 			elr->lr_timeout = nsecs_to_jiffies((ktime_get_ns() - start_time) *
-- 
2.34.1

Re: [PATCH v2 1/2] ext4: check return value of blkdev_issue_flush() when initializing inode tables

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ext4: information leak via uninitialized inode tables when lazy init aborts
- [High] ext4: missing cache flush in no-journal mode exposes stale data during lazy init
--

commit 38a8b479f27a31839f98ff44865f5323825a1303
Author: guolingxing <guolingxing@tju.edu.cn>

ext4: check return value of blkdev_issue_flush() when initializing inode tables

This commit introduces error checking for the cache flush in
ext4_init_inode_table(). If the flush fails, the initialization is aborted,
and it updates ext4_run_li_request() to check the BARRIER mount option.

Cc: stable@vger.kernel.org

> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index f97a2f4904ebb..843a78fc6e51a 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
[ ... ]
> @@ -1611,8 +1611,11 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
>  	ret = sb_issue_zeroout(sb, blk, num, GFP_NOFS);
>  	if (ret < 0)
>  		goto err_out;
> -	if (barrier)
> -		blkdev_issue_flush(sb->s_bdev);
> +	if (barrier) {
> +		ret = blkdev_issue_flush(sb->s_bdev);
> +		if (ret)
> +			goto err_out;
> +	}

[Severity: High]
If blkdev_issue_flush() fails with an error like -EOPNOTSUPP, this will now
unconditionally abort the initialization via goto err_out.

When ext4_init_inode_table() returns this error back up to
ext4_lazyinit_thread(), the thread permanently removes the lazy init request:

fs/ext4/super.c:ext4_lazyinit_thread() {
    ...
    if (err) {
        ext4_remove_li_request(elr);
        continue;
    }
    ...
}

This leaves all remaining groups uninitialized. Could an unprivileged user
then force an uninitialized inode table block into the page cache (e.g., by
attempting to read a free inode via a stale file handle)? 

If so, __ext4_get_inode_loc() would see the block is already in the cache and
bypass the on-demand zeroing fast path:

fs/ext4/inode.c:__ext4_get_inode_loc() {
    ...
    bh = sb_getblk(sb, block);
    if (unlikely(!bh))
        return -ENOMEM;
    if (ext4_buffer_uptodate(bh))
        goto has_buffer;
    ...
}

Would this allow an attacker to allocate a new file in that block and read
the uninitialized fields (like extended attribute space), exposing stale
disk contents?

>  
>  skip_zeroout:
>  	ext4_lock_group(sb, group);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7d8aedfbbb87f..7d882981bfabe 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
[ ... ]
> @@ -3827,7 +3827,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
>  	if (!ret) {
>  		start_time = ktime_get_ns();
>  		ret = ext4_init_inode_table(sb, group,
> -					    elr->lr_timeout ? 0 : 1);
> +					    !elr->lr_timeout &&
> +					    test_opt(sb, BARRIER));

[Severity: High]
For all block groups after the first, elr->lr_timeout becomes non-zero,
meaning !elr->lr_timeout evaluates to false and barrier is passed as 0 to
ext4_init_inode_table().

Without the barrier, ext4_init_inode_table() skips blkdev_issue_flush(),
while sb_issue_zeroout() writes zeroes to the disk cache and the
EXT4_BG_INODE_ZEROED flag is set on the group descriptor.

In no-journal mode, without an explicit flush, could the disk reorder the
group descriptor write before the zeroes? If a crash occurs at the right
time, might this expose stale data to new users since the allocator assumes
the blocks with EXT4_BG_INODE_ZEROED set are already zeroed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013082331.2089623-1-guolingxing@tju.edu.cn?part=1

[PATCH v2 2/2] ext4: check return value of blkdev_issue_flush() when starting a fast commit

[guolingxing]

When the file system device differs from the journal device, a cache
flush is issued on the file system device before fast commit blocks
are written, but the return value is ignored.  If the cache flush
fails, the fast commit blocks are written and the fast commit
completes anyway, even though the updates covered by the preceding
transactions may not be on stable storage yet, so the fast commit may
provide guarantees it cannot keep.

Check the return value of blkdev_issue_flush() and abort the fast
commit if the cache flush fails.  ext4_fc_commit() will then fall back
to a full journal commit, so the caller still gets the expected
durability guarantees.

Since a cache flush failure now aborts the fast commit, also issue
the flush only when the journal barrier option is enabled, consistent
with the file system device flush issued during a full journal commit
and with the fast commit tail block flush.  This keeps file systems
mounted with nobarrier working on devices with failing cache flushes.

Fixes: da0c5d2695265 ("ext4: issue fsdev cache flush before starting fast commit")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/fast_commit.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 0cac890cf370..c669f4cd7d85 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -1425,8 +1425,12 @@ static int ext4_fc_perform_commit(journal_t *journal, tid_t commit_tid)
 	 * Step 5: If file system device is different from journal device,
 	 * issue a cache flush before we start writing fast commit blocks.
 	 */
-	if (journal->j_fs_dev != journal->j_dev)
-		blkdev_issue_flush(journal->j_fs_dev);
+	if (journal->j_fs_dev != journal->j_dev &&
+	    journal->j_flags & JBD2_BARRIER) {
+		ret = blkdev_issue_flush(journal->j_fs_dev);
+		if (ret)
+			return ret;
+	}
 
 	blk_start_plug(&plug);
 	alloc_ctx = ext4_fc_lock(sb);
-- 
2.34.1

Re: [PATCH v2 2/2] ext4: check return value of blkdev_issue_flush() when starting a fast commit

[sashiko-bot]

> When the file system device differs from the journal device, a cache
> flush is issued on the file system device before fast commit blocks
> are written, but the return value is ignored.  If the cache flush
> fails, the fast commit blocks are written and the fast commit
> completes anyway, even though the updates covered by the preceding
> transactions may not be on stable storage yet, so the fast commit may
> provide guarantees it cannot keep.
> 
> Check the return value of blkdev_issue_flush() and abort the fast
> commit if the cache flush fails.  ext4_fc_commit() will then fall back
> to a full journal commit, so the caller still gets the expected
> durability guarantees.
> 
> Since a cache flush failure now aborts the fast commit, also issue
> the flush only when the journal barrier option is enabled, consistent
> with the file system device flush issued during a full journal commit
> and with the fast commit tail block flush.  This keeps file systems
> mounted with nobarrier working on devices with failing cache flushes.
> 
> Fixes: da0c5d2695265 ("ext4: issue fsdev cache flush before starting fast commit")
> Cc: stable@vger.kernel.org
> Signed-off-by: guolingxing <guolingxing@tju.edu.cn>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013082331.2089623-1-guolingxing@tju.edu.cn?part=2