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

[sashiko-bot@kernel.org]

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