Re: [PATCH 2/2] ntfs: roll back the in-memory attributes if the metadata update fails

[liubaolin]

在 2026/9/15 14:27, Hongling Zeng 写道:
> ntfs_setattr() copies the new attributes into the VFS inode with
> setattr_copy() and only then writes them to disk: the POSIX ACL update
> and the ntfs_ea_set_wsl_inode() call that stores the uid, gid and mode
> in the metadata EAs.  When one of those fails, the error is returned
> but the in-memory inode keeps the new, uncommitted attributes, so
> memory and disk diverge until the inode is written back or evicted.
>
> Remember the old mode, uid, gid and the FILE_ATTR_READONLY bit and
> restore them before returning the error.  This is best effort: the
> ctime update is left in place, ntfs_ea_set_wsl_inode() may have
> completed some of the EAs before failing, and a failed ACL update may
> leave the cached ACL divergent.  The error is still reported.
>
> Fixes: 9c87959601e8 ("ntfs: update file operations")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> ---
>   fs/ntfs/file.c | 28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 1417a76788ed..524b33c0e97f 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -325,6 +325,10 @@ int ntfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   	unsigned int ia_valid = attr->ia_valid;
>   	struct ntfs_inode *ni = NTFS_I(vi);
>   	struct ntfs_volume *vol = ni->vol;
> +	umode_t old_mode;
> +	kuid_t old_uid;
> +	kgid_t old_gid;
> +	typeof(ni->flags) old_flags;
>   
>   	if (NVolShutdown(vol))
>   		return -EIO;
> @@ -351,12 +355,17 @@ int ntfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   		ia_valid |= ATTR_MTIME | ATTR_CTIME;
>   	}
>   
> +	old_mode = vi->i_mode;
> +	old_uid = vi->i_uid;
> +	old_gid = vi->i_gid;
> +	old_flags = ni->flags & FILE_ATTR_READONLY;
> +
>   	setattr_copy(idmap, vi, attr);
>   
>   	if (vol->sb->s_flags & SB_POSIXACL && !S_ISLNK(vi->i_mode)) {
>   		err = posix_acl_chmod(idmap, dentry, vi->i_mode);
>   		if (err)
> -			goto out;
> +			goto out_restore;
>   	}
>   
>   	if (0222 & vi->i_mode)
> @@ -378,11 +387,26 @@ int ntfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   		err = ntfs_ea_set_wsl_inode(vi, 0, NULL, flags);
>   		mutex_unlock(&ni->mrec_lock);
>   		if (err)
> -			goto out;
> +			goto out_restore;
>   

Hi Hongling,
   Thanks for the patch.  Restoring the in-memory attributes addresses 
the failure case where no on-disk update has been committed.
  However, ntfs_ea_set_wsl_inode() updates $LXUID, $LXGID and $LXMOD 
independently, so it can fail after partially updating the on-disk 
metadata.  In that case, restoring all VFS attributes to their old 
values can itself create an inconsistency with the disk state.  The ACL 
path has a similar issue if the ACL update succeeds before the later EA 
update fails.

  This requires transactional updates of the related EAs with atomic 
failure handling.  Maintainer Namjae is currently working on the NTFS 
journal support, which should provide the necessary transactional 
infrastructure.
  Could this patch be deferred until that journal work is available, so 
that the attribute updates and their rollback can be handled atomically?

Thanks,
Baolin.

>   	}
>   
>   	mark_inode_dirty(vi);
> +	goto out;
> +
> +out_restore:
> +	/*
> +	 * The on-disk metadata update failed: put the in-memory
> +	 * attributes back so that the inode does not keep the new,
> +	 * uncommitted values.  This is best effort: the ctime update is
> +	 * left in place, ntfs_ea_set_wsl_inode() may have completed some
> +	 * of the EAs before failing, and a failed ACL update may leave
> +	 * the cached ACL divergent.
> +	 */
> +	vi->i_mode = old_mode;
> +	vi->i_uid = old_uid;
> +	vi->i_gid = old_gid;
> +	ni->flags = (ni->flags & ~FILE_ATTR_READONLY) | old_flags;
>   out:
>   	return err;
>   }