Re: [PATCH v2] ntfs: record an error when the ACL xattr undo fails

[Hyunchul Lee]

Hi Hongling,

On Sun, Oct 04, 2026 at 10:26:22AM +0800, Hongling Zeng wrote:
> ntfs_set_acl_ex() commits the new ACL xattr before it rewrites the WSL
> mode EA, and removes it again when that rewrite fails.  The removal is
> best-effort: its result is discarded and the EA paths record no error
> state, so when the removal fails too, the new, typically more
> permissive, ACL is left on disk while i_mode, the mode EA and the
> cached ACL keep the old values.  Once the stale xattr is loaded again -
> after a remount, or when the cached ACL is discarded - a chmod 0640 ->
> 0777 that reported failure leaves the file accessible to the relaxed
> ACL while stat() still reports 0640.
> 
> Check the removal and record the failure: NVolSetErrors(), which the
> next persistence point persists as VOLUME_IS_DIRTY, and a log message
> the errors= policy acts on.  The flag cannot be written to disk here
> directly: the target inode's mrec_lock is held and the $Volume
> mrec_lock cannot be nested under it.
> 
> Fixes: fc053f05ca28 ("ntfs: add reparse and ea operations")
> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> ---
>  Change in v2:
>  -undo only a committed set: undoing a removal returned -ENODATA for an
>   already-removed xattr and was recorded as a volume error, or re-added
>   an empty xattr once the removal had dropped the last EA
> ---
>  fs/ntfs/ea.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs/ea.c b/fs/ntfs/ea.c
> index b4fcfbe2da4c..b3e079e2dc03 100644
> --- a/fs/ntfs/ea.c
> +++ b/fs/ntfs/ea.c
> @@ -1084,8 +1084,34 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
>  		mutex_lock(&NTFS_I(inode)->mrec_lock);
>  		err = ntfs_ea_set_wsl_inode(inode, 0, &ea_size, NTFS_EA_MODE);
>  		if (err) {
> -			ntfs_set_ea(inode, name, name_len, NULL, 0,
> -				    XATTR_REPLACE, NULL);
> +			/*
> +			 * Only a set needs undoing: a removal leaves no
> +			 * new ACL behind, and repeating it would either
> +			 * return -ENODATA again or, once it dropped the
> +			 * last EA, re-create an empty xattr.
> +			 */
> +			if (acl) {
> +				int undo_err = ntfs_set_ea(inode, name, name_len,
> +						NULL, 0, XATTR_REPLACE, NULL);

Could we restore the previous ACL xattr instead of unconditionally
remove it?

> +
> +				if (undo_err) {
> +					/*
> +					 * The ACL xattr is committed
> +					 * already, so a failed removal
> +					 * leaves the new, typically more
> +					 * permissive, ACL on disk while
> +					 * i_mode, the mode EA and the
> +					 * cached ACL keep the old values.
> +					 * Record the error: the next
> +					 * persistence point persists it as
> +					 * VOLUME_IS_DIRTY.  Run chkdsk.
> +					 */
> +					NVolSetErrors(NTFS_I(inode)->vol);
> +					ntfs_error(inode->i_sb,
> +						   "Failed to undo the ACL xattr: %d",
> +						   undo_err);
> +				}
> +			}
>  			mutex_unlock(&NTFS_I(inode)->mrec_lock);
>  			inode->i_mode = old_mode;
>  			goto out;
> -- 
> 2.25.1
> 

-- 
Thanks,
Hyunchul