Re: [PATCH -next v5 31/32] ext4: partially enable iomap for the buffered I/O path of regular files

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Encrypted and verity inodes on corrupted or downgraded filesystems bypass fscrypt and fsverity, leading to plaintext data writes and unverified reads.
- [Medium] Race condition in background writeback causing spurious WARN_ON_ONCE in ext4_iomap_writepages().
--

--- Patch [31]: [PATCH -next v5 31/32] ext4: partially enable iomap for the buffered I/O path of regular files ---
commit 385d0947c4a6c1e02718f1b42a8ef1dc35bdbf03
Author: Zhang Yi <[email protected]>
Subject: ext4: partially enable iomap for the buffered I/O path of regular files

This commit introduces ext4_enable_buffered_iomap() to determine whether a
regular file inode should use the iomap buffered I/O path. Inodes silently
fall back to the traditional buffer_head path if unsupported features like
encryption, verity, inline data, or data=journal mode are met.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ee15366422a1e..c9ee78fba4d0c 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -4467,6 +4479,10 @@ static int ext4_iomap_writepages(struct address_space *mapping,
>  		.ops = &ext4_writeback_ops,
>  	};
>  
> +	/* inode using the buffer_head buffered I/O path should not go here. */
> +	if (WARN_ON_ONCE(!ext4_inode_buffered_iomap(inode)))
> +		return -EINVAL;
> +

[Severity: Medium]
Can this WARN_ON_ONCE trigger spuriously due to a race with
ext4_change_inode_journal_flag()?

If background writeback via do_writepages() resolves
mapping->a_ops->writepages to ext4_iomap_writepages locklessly, the thread
might be preempted right before this check.

Meanwhile, another thread running ext4_change_inode_journal_flag() could
flush writeback, acquire s_writepages_rwsem exclusively, and clear the
EXT4_STATE_BUFFERED_IOMAP flag.

When the writeback thread resumes, it would evaluate this condition before
acquiring the s_writepages_rwsem read lock, which would trip the warning.
Should this check be moved after acquiring the read lock?

>  	ret = ext4_emergency_state(sb);
>  	if (unlikely(ret)) {

[ ... ]

> @@ -6037,6 +6053,81 @@ static int check_igot_inode(struct inode *inode, ext4_iget_flags flags,
>  	return -EFSCORRUPTED;
>  }
>  
> +/*
> + * Determine whether an inode should use the iomap buffered I/O path.
> + * EXT4_STATE_BUFFERED_IOMAP is generally set at inode initialization

[ ... ]

> +void ext4_enable_buffered_iomap(struct inode *inode)
> +{
> +	struct super_block *sb = inode->i_sb;
> +
> +	if (!S_ISREG(inode->i_mode))
> +		return;
> +	if (ext4_test_inode_flag(inode, EXT4_INODE_EA_INODE))
> +		return;
> +
> +	/* Unsupported Features */
> +	if (ext4_has_feature_inline_data(sb))
> +		return;
> +	if (ext4_has_feature_verity(sb))
> +		return;
> +	if (ext4_has_feature_encrypt(sb))
> +		return;

[Severity: Critical]
Could checking the superblock features instead of the inode flags here lead
to a security bypass if the filesystem is manipulated offline?

If an attacker clears the encryption or verity superblock features offline,
the kernel still sets S_ENCRYPTED or S_VERITY on individual inodes that have
the flags set.

However, these superblock checks would fall through, allowing the encrypted
or verity inodes to be assigned to the iomap buffered I/O path. Since the
ext4 iomap path currently lacks fscrypt and fsverity integration, would this
cause writes to submit plaintext bios and reads to skip cryptographic
verification?

Would it be safer to check IS_ENCRYPTED(inode) and
ext4_test_inode_flag(inode, EXT4_INODE_VERITY) directly?

> +	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA ||
> +	    ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
> +		return;
> +	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
> +		return;
> +
> +	ext4_set_inode_state(inode, EXT4_STATE_BUFFERED_IOMAP);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=31