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

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).

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

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