Re: [PATCH -next v5 03/32] ext4: simplify error handling in ext4_setattr()

On 8/14/2026 6:12 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] The patch incorrectly routes non-fatal errors from `posix_acl_chmod()` (and `ext4_truncate()`) to `ext4_std_error()`, triggering a spurious filesystem corruption response (DoS).
> --
> 
> --- Patch [3]: [PATCH -next v5 03/32] ext4: simplify error handling in ext4_setattr() ---
> commit f94f03ca7ff7f314c8faf61276a2cc51f78e4ce7
> Author: Zhang Yi <[email protected]>
> 
> ext4: simplify error handling in ext4_setattr()
> 
> Refactor the error handling in ext4_setattr() to propagate errors using
> the existing error variable and jump to the common 'err_out' label. This
> removes the need for the redundant rc variable and unreachable checks.
> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 8654006a57eff..76bf0e944ebee 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
> [ ... ]
>> @@ -6244,15 +6244,19 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>>   		}
>>   
>>   		filemap_invalidate_unlock(inode->i_mapping);
>> +		if (error)
>> +			goto err_out;
>>   	}
>>   
>> -	if (!error) {
>> -		if (inc_ivers)
>> -			inode_inc_iversion(inode);
>> -		setattr_copy(idmap, inode, attr);
>> -		mark_inode_dirty(inode);
>> -	}
>> +	if (inc_ivers)
>> +		inode_inc_iversion(inode);
>> +	setattr_copy(idmap, inode, attr);
>> +	mark_inode_dirty(inode);
>>   
>> +	if (ia_valid & ATTR_MODE)
>> +		error = posix_acl_chmod(idmap, dentry, inode->i_mode);
>> +
>> +err_out:
>>   	/*
>>   	 * If the call to ext4_truncate failed to get a transaction handle at
>>   	 * all, we need to clean up the in-core orphan list manually.
>> @@ -6260,14 +6264,8 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>>   	if (orphan && inode->i_nlink)
>>   		ext4_orphan_del(NULL, inode);
>>   
>> -	if (!error && (ia_valid & ATTR_MODE))
>> -		rc = posix_acl_chmod(idmap, dentry, inode->i_mode);
>> -
>> -err_out:
>> -	if  (error)
>> +	if (error)
>>   		ext4_std_error(inode->i_sb, error);
> 
> [Severity: Critical]
> Does this code now incorrectly route non-fatal errors from posix_acl_chmod()
> (and ext4_truncate()) to ext4_std_error()?
> 
> Passing a non-fatal error code like -ENOSPC or -EDQUOT to ext4_std_error()
> can cause it to treat the condition as a critical filesystem metadata
> corruption, aborting the journal and remounting read-only (or panicking).
> 
> For example, if an unprivileged user exhausts their quota or filesystem
> space, and then executes chmod on a file with a POSIX ACL,
> posix_acl_chmod() attempts to update the ACL, calling ext4_set_acl(), which
> may return -EDQUOT or -ENOSPC. This error is now assigned to the error
> variable and falls through to the err_out label, triggering
> ext4_std_error() and causing a potential local Denial of Service (DoS).

Yeah, if an inode shares its xattrs with another one, updating ACLs
will allocate new space and can return ENOSPC when we're low on space.
I'll drop this patch in the next spin. For later, maybe we can add a
helper similar to ext4_error_is_fatal() to filter ENOSPC, ENOMEM, and
EDQUOT, and only call ext4_std_error() for the real fatal ones. We've
got similar checks in extent.c, so we could probably generalize that
and share the logic.

Thanks,
Yi.

> 
>> -	if (!error)
>> -		error = rc;
>>   	return error;
>>   }
>>
>