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

From: Hongling Zeng

Date: Mon Oct 12 2026 - 21:19:02 EST

Hi Hyunchul,

Thanks for the review.
I will trim the comments and drop old_exists in favour of
old_len >= 0. Will post v5.

Thanks!
在 2026年10月12日 17:49, 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@xxxxxxxxx>
Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
---
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