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