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

[Hongling Zeng <zhongling0719@126.com>]

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