[PATCH v3] ntfs: roll back the ACL xattr commit when the WSL mode update 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.  Removing the
new xattr is only the right undo when no ACL xattr existed before the
update: when the update replaced an existing one, the old ACL is
discarded along with the failed update.  The undo is best-effort as
well: its 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, 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.

Save the previous xattr before committing the new one, and roll back
to it when the mode EA rewrite fails.  Remove the new xattr only when
there was no previous one.  When the previous xattr cannot be read
back, fail the update instead of replacing a value that could not be
rolled back to.  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
---
 fs/ntfs/ea.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/fs/ntfs/ea.c b/fs/ntfs/ea.c
index 1104fd72cd2a..319c2c09632f 100644
--- a/fs/ntfs/ea.c
+++ b/fs/ntfs/ea.c
@@ -1038,8 +1038,9 @@ 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;
 	int flags;
 	umode_t mode;
 
@@ -1083,7 +1084,39 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
 	}
 
 	mutex_lock(&NTFS_I(inode)->mrec_lock);
+	if (acl && !init_acl) {
+		/*
+		 * Save the previous xattr so a committed set can be
+		 * rolled back to it.  A new inode cannot have one yet,
+		 * and one that cannot be read fails the update instead
+		 * of being replaced: what was not saved cannot be
+		 * rolled back to.
+		 */
+		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);
+out_unlock:
 	mutex_unlock(&NTFS_I(inode)->mrec_lock);
 	if (err == -ENODATA && !size)
 		err = 0; /* Removing non existed xattr. */
@@ -1095,8 +1128,52 @@ 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) {
+				int undo_err;
+
+				/*
+				 * 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 (old_exists) {
+					/*
+					 * A zero-length xattr cannot be
+					 * restored with XATTR_REPLACE,
+					 * which would remove it again.
+					 */
+					int undo_flags = old_len ?
+						XATTR_REPLACE : 0;
+
+					undo_err = ntfs_set_ea(inode, name,
+							name_len, old_value,
+							old_len, undo_flags,
+							NULL);
+				} else {
+					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 rollback
+					 * 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;
@@ -1109,6 +1186,7 @@ static noinline int ntfs_set_acl_ex(struct mnt_idmap *idmap,
 	}
 
 out:
+	kfree(old_value);
 	kfree(value);
 
 	return err;
-- 
2.25.1