[PATCH v4] ntfs: roll back the ACL xattr commit when the WSL mode update fails

[PATCH v4] ntfs: roll back the ACL xattr commit when the WSL mode update fails

[Hongling Zeng]

ntfs_set_acl_ex() commits the ACL xattr before it rewrites the WSL
mode EA, and removes it again when the rewrite fails.  That only
undoes a set that created the xattr: when the update replaced or
removed an existing one, the old ACL is discarded along with the
failed update.  The undo result is discarded and the EA paths record
no error state, so when it 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, a chmod 0640 -> 0777 that reported failure leaves
the file accessible to the relaxed ACL while stat() still reports
0640.

Save the previous xattr before committing the update, and restore it
when the mode EA rewrite fails.  A removed xattr is restored by
creating it again: the removal dropped the entry, and XATTR_REPLACE
fails with -ENODATA on a missing entry.  Remove the new xattr only
when there was no previous one, and skip the undo when the commit
changed nothing, e.g. removing an xattr that never existed.  Fail
the update when the previous xattr cannot be read back.  A failed
rollback is still recorded: 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")
Suggested-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
Changes in v3:
- Save the previous ACL xattr before committing the new one and roll
  back to it when the update replaced an existing one, instead of
  removing the new one (suggested by Hyunchul Lee)
- Fail the update when the previous xattr cannot be read back, and
  restore a zero-length previous xattr without XATTR_REPLACE
Changes in v4:
- Undo a removal too: restore the previous xattr by creating it
  again, since the removal dropped the entry
- Skip the undo when the commit changed nothing, e.g. removing an
  xattr that never existed
---
 fs/ntfs/ea.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 4 deletions(-)

diff --git a/fs/ntfs/ea.c b/fs/ntfs/ea.c
index 1104fd72cd2a..0577a2adc365 100644
--- a/fs/ntfs/ea.c
+++ b/fs/ntfs/ea.c
@@ -1038,8 +1038,10 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
 {
 	const char *name;
 	size_t size, name_len;
-	void *value;
-	int err;
+	void *value, *old_value = NULL;
+	int err, old_len = -ENODATA;
+	bool old_exists = false;
+	bool acl_committed = false;
 	int flags;
 	umode_t mode;
 
@@ -1083,7 +1085,40 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
 	}
 
 	mutex_lock(&NTFS_I(inode)->mrec_lock);
+	if (!init_acl) {
+		/*
+		 * Save the previous xattr so the update can be
+		 * rolled back to it when the mode EA rewrite fails.
+		 * A new inode cannot have one yet, and one that
+		 * cannot be read back fails the update.
+		 */
+		old_len = ntfs_get_ea(inode, name, name_len, NULL, 0);
+		if (old_len >= 0) {
+			old_exists = true;
+			/* kmalloc(0) returns the non-NULL ZERO_SIZE_PTR. */
+			old_value = kmalloc(old_len, GFP_NOFS);
+			if (!old_value) {
+				err = -ENOMEM;
+				goto out_unlock;
+			}
+			err = ntfs_get_ea(inode, name, name_len,
+					  old_value, old_len);
+			if (err != old_len) {
+				kfree(old_value);
+				old_value = NULL;
+				if (err >= 0)
+					err = -EIO;
+				goto out_unlock;
+			}
+		} else if (old_len != -ENODATA) {
+			err = old_len;
+			goto out_unlock;
+		}
+	}
 	err = ntfs_set_ea(inode, name, name_len, value, size, flags, NULL);
+	if (!err)
+		acl_committed = true;
+out_unlock:
 	mutex_unlock(&NTFS_I(inode)->mrec_lock);
 	if (err == -ENODATA && !size)
 		err = 0; /* Removing non existed xattr. */
@@ -1095,8 +1130,54 @@ 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);
+			if (acl_committed) {
+				int undo_err;
+
+				if (old_exists) {
+					/*
+					 * Restore the previous xattr.
+					 * A removal dropped the entry, and
+					 * XATTR_REPLACE fails with
+					 * -ENODATA on a missing entry, so
+					 * create it.  A zero-length xattr
+					 * cannot be restored with
+					 * XATTR_REPLACE, which would
+					 * remove it again.
+					 */
+					int undo_flags = acl && old_len ?
+						XATTR_REPLACE : 0;
+
+					undo_err = ntfs_set_ea(inode, name,
+							name_len, old_value,
+							old_len, undo_flags,
+							NULL);
+				} else {
+					/*
+					 * No xattr existed before the
+					 * update, so remove the newly
+					 * committed one.
+					 */
+					undo_err = ntfs_set_ea(inode, name,
+							name_len, NULL, 0,
+							XATTR_REPLACE, NULL);
+				}
+				if (undo_err) {
+					/*
+					 * The xattr change is committed
+					 * already, so a failed rollback
+					 * leaves it 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;
@@ -1109,6 +1190,7 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
 	}
 
 out:
+	kfree(old_value);
 	kfree(value);
 
 	return err;
-- 
2.25.1

Re: [PATCH v4] ntfs: roll back the ACL xattr commit when the WSL mode update fails

[Hyunchul Lee]

Hi Hongling,

On Mon, Oct 12, 2026 at 03:34:52PM +0800, Hongling Zeng wrote:
> ntfs_set_acl_ex() commits the ACL xattr before it rewrites the WSL
> mode EA, and removes it again when the rewrite fails.  That only
> undoes a set that created the xattr: when the update replaced or
> removed an existing one, the old ACL is discarded along with the
> failed update.  The undo result is discarded and the EA paths record
> no error state, so when it 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, a chmod 0640 -> 0777 that reported failure leaves
> the file accessible to the relaxed ACL while stat() still reports
> 0640.
> 
> Save the previous xattr before committing the update, and restore it
> when the mode EA rewrite fails.  A removed xattr is restored by
> creating it again: the removal dropped the entry, and XATTR_REPLACE
> fails with -ENODATA on a missing entry.  Remove the new xattr only
> when there was no previous one, and skip the undo when the commit
> changed nothing, e.g. removing an xattr that never existed.  Fail
> the update when the previous xattr cannot be read back.  A failed
> rollback is still recorded: 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")
> Suggested-by: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> Changes in v3:
> - Save the previous ACL xattr before committing the new one and roll
>   back to it when the update replaced an existing one, instead of
>   removing the new one (suggested by Hyunchul Lee)
> - Fail the update when the previous xattr cannot be read back, and
>   restore a zero-length previous xattr without XATTR_REPLACE
> Changes in v4:
> - Undo a removal too: restore the previous xattr by creating it
>   again, since the removal dropped the entry
> - Skip the undo when the commit changed nothing, e.g. removing an
>   xattr that never existed
> ---
>  fs/ntfs/ea.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ntfs/ea.c b/fs/ntfs/ea.c
> index 1104fd72cd2a..0577a2adc365 100644
> --- a/fs/ntfs/ea.c
> +++ b/fs/ntfs/ea.c
> @@ -1038,8 +1038,10 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
>  {
>  	const char *name;
>  	size_t size, name_len;
> -	void *value;
> -	int err;
> +	void *value, *old_value = NULL;
> +	int err, old_len = -ENODATA;
> +	bool old_exists = false;
> +	bool acl_committed = false;
>  	int flags;
>  	umode_t mode;
>  
> @@ -1083,7 +1085,40 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
>  	}
>  
>  	mutex_lock(&NTFS_I(inode)->mrec_lock);
> +	if (!init_acl) {
> +		/*
> +		 * Save the previous xattr so the update can be
> +		 * rolled back to it when the mode EA rewrite fails.
> +		 * A new inode cannot have one yet, and one that
> +		 * cannot be read back fails the update.
> +		 */

Could you shorten this comment? I think that the explanations are already
clear from the code.

> +		old_len = ntfs_get_ea(inode, name, name_len, NULL, 0);
> +		if (old_len >= 0) {
> +			old_exists = true;

Could you remove the old_exists? we can use old_len >= 0.

> +			/* kmalloc(0) returns the non-NULL ZERO_SIZE_PTR. */
> +			old_value = kmalloc(old_len, GFP_NOFS);
> +			if (!old_value) {
> +				err = -ENOMEM;
> +				goto out_unlock;
> +			}
> +			err = ntfs_get_ea(inode, name, name_len,
> +					  old_value, old_len);
> +			if (err != old_len) {
> +				kfree(old_value);
> +				old_value = NULL;
> +				if (err >= 0)
> +					err = -EIO;
> +				goto out_unlock;
> +			}
> +		} else if (old_len != -ENODATA) {
> +			err = old_len;
> +			goto out_unlock;
> +		}
> +	}
>  	err = ntfs_set_ea(inode, name, name_len, value, size, flags, NULL);
> +	if (!err)
> +		acl_committed = true;
> +out_unlock:
>  	mutex_unlock(&NTFS_I(inode)->mrec_lock);
>  	if (err == -ENODATA && !size)
>  		err = 0; /* Removing non existed xattr. */
> @@ -1095,8 +1130,54 @@ 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);
> +			if (acl_committed) {
> +				int undo_err;
> +
> +				if (old_exists) {
> +					/*
> +					 * Restore the previous xattr.
> +					 * A removal dropped the entry, and
> +					 * XATTR_REPLACE fails with
> +					 * -ENODATA on a missing entry, so
> +					 * create it.  A zero-length xattr
> +					 * cannot be restored with
> +					 * XATTR_REPLACE, which would
> +					 * remove it again.
> +					 */

And I think this comment can be reduced.

> +					int undo_flags = acl && old_len ?
> +						XATTR_REPLACE : 0;
> +
> +					undo_err = ntfs_set_ea(inode, name,
> +							name_len, old_value,
> +							old_len, undo_flags,
> +							NULL);
> +				} else {
> +					/*
> +					 * No xattr existed before the
> +					 * update, so remove the newly
> +					 * committed one.
> +					 */

This branch is self-explanatory from `!old_exists`. 

> +					undo_err = ntfs_set_ea(inode, name,
> +							name_len, NULL, 0,
> +							XATTR_REPLACE, NULL);
> +				}
> +				if (undo_err) {
> +					/*
> +					 * The xattr change is committed
> +					 * already, so a failed rollback
> +					 * leaves it 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.
> +					 */

Same here.

> +					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;
> @@ -1109,6 +1190,7 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
>  	}
>  
>  out:
> +	kfree(old_value);
>  	kfree(value);
>  
>  	return err;
> -- 
> 2.25.1
> 

-- 
Thanks,
Hyunchul