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

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

[Hongling Zeng]

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@kylinos.cn>
---
 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);
+
+				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

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@kylinos.cn>
> ---
>  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