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

On 8/14/2026 6:39 PM, [email protected] wrote:
> 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?
> 

This is right, and the issue is worse than a spurious WARN, proceeding
with mismatched a_ops in that window can corrupt buffer_head-backed
folios. Sequence:

writeback         change journal flag      new write
---------         -------------------      ---------
do_writepages()
   a_ops->writepages resolved
   = ext4_iomap_writepages
   WARN check passes
   (BUFFERED_IOMAP still set)
   <preempted>
                   filemap_invalidate_lock()
                   filemap_write_and_wait()
                   truncate_pagecache(inode, 0)
                   ext4_writepages_down_write()
                   ext4_disable_buffered_iomap()
                     -> clear BUFFERED_IOMAP
                   ext4_set_aops()
                     -> a_ops = ext4_journalled_aops
                   release write lock + invalidate
                                            write() syscall
                                            ext4_journalled_aops
                                            create_empty_buffers()
                                            folio->private = buffer_head
resumed
ext4_writepages_down_read()
iomap_writepages()
   ifs_alloc(folio) sees folio->private != NULL
   -> returns it as iomap_folio_state
   -> actually a buffer_head pointer
   ifs->state, ifs->state_lock accessed
   -> memory/data corruption

So we should move the BUFFERED_IOMAP flag check under the
s_writepages_rwsem in ext4_iomap_writepages() and drop both
WARN_ON_ONCE() during writeback.

>>   	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?
> 

This is a false positive. I don't think we need to consider
offline filesystem corruption here.

Thanks,
Yi.

>> +	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);
>