Re: [PATCH v2] ext4: don't append a directory block already mapped in the inode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ext4: ext4_append() misses aliases in multi-block extents
- [High] ext4: ext4_append() forces O(N) iteration over holes leading to soft lockup
- [High] ext4: ext4_append() escalates transient errors to filesystem corruption
--

commit 4e22a7e845c3a3e2ac2696b510b59d9b11640408
Author: Adriano Cordova <adrianox@gmail.com>
ext4: don't append a directory block already mapped in the inode

This commit adds a check in ext4_append to ensure a newly allocated
directory block is not already mapped by the inode. It iterates through
the inode's logical blocks to verify the new physical block does not
alias an existing one.

Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
[ ... ]
> @@ -83,6 +83,25 @@ static struct buffer_head *ext4_append(handle_t *handle,
>  	bh = ext4_bread(handle, inode, *block, EXT4_GET_BLOCKS_CREATE);
>  	if (IS_ERR(bh))
>  		return bh;
> +
> +	for (map.m_lblk = 0; map.m_lblk < *block; map.m_lblk += map.m_len) {
> +		map.m_len = *block - map.m_lblk;
> +		err = ext4_map_blocks(NULL, inode, &map, 0);
> +		if (err < 0)
> +			goto out;

[Severity: High]
Could this cause a regression where transient errors are escalated to
filesystem corruption?

The goto out path calls ext4_std_error(inode->i_sb, err), which marks the
filesystem as corrupted.

If ext4_map_blocks() returns a transient error like -ENOMEM or -EIO, should
it instead propagate the error directly back to the caller, similar to how
the earlier ext4_map_blocks() call handles them before the loop?

> +		if (err == 0) {
> +			map.m_len = 1;
> +			continue;
> +		}

[Severity: High]
Does this force an iteration over large holes, potentially causing a
performance regression?

When ext4_map_blocks() returns 0 for a hole, it populates map.m_len with
the full length of the hole.

Overriding map.m_len to 1 discards the hole size and forces the loop to
iterate block-by-block. For directories with large sparse holes, could this
lead to severe performance degradation and soft lockups?

> +		if (unlikely(map.m_pblk == bh->b_blocknr)) {

[Severity: High]
Can this equality check miss aliases if the mapped extent spans multiple
blocks, allowing a memory corruption regression?

Because ext4_map_blocks() can return a multi-block extent with map.m_len > 1,
the newly allocated block bh->b_blocknr might fall in the middle of a
previously allocated multi-block extent.

If bh->b_blocknr is within the range from map.m_pblk to
map.m_pblk + map.m_len - 1, this exact match check will evaluate to false,
failing to prevent the alias.

Should this check account for the extent length?

> +			EXT4_ERROR_INODE(inode,
> +					 "new block %llu already mapped",
> +					 (unsigned long long)bh->b_blocknr);
> +			err = -EFSCORRUPTED;
> +			goto out;
> +		}
> +	}
> +
>  	inode->i_size += inode->i_sb->s_blocksize;
>  	EXT4_I(inode)->i_disksize = inode->i_size;
>  	err = ext4_mark_inode_dirty(handle, inode);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005003528.92046-1-adrianox@gmail.com?part=1