[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

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

[Hongling Zeng]

在 2026年10月06日 12:42, 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?
Hi   Hyunchul
   Good point. Removing the new xattr also loses the old one when it
   replaced an existing ACL.

   v3 saves the previous xattr before the commit and restores it on a
   mode EA failure, removing the new one only when there was no previous
   one. A failed rollback is still recorded with NVolSetErrors().

Thanks!


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