On 4/30/25 10:49, Guang Yuan Wu wrote:
> Hi, all,
> Here is the updated V3 patch to address Bernd's comments:
> - fix format issue (keep original tab/space style)
> - remove "Reviewed-by:..." lines
> - invalidate attr by timeout of i_time, instead of inval_mask
>
>
> V3:
>
> fuse: fix race between concurrent setattrs from multiple nodes
>
> When mounting a user-space filesystem on multiple clients, after
> concurrent ->setattr() calls from different node, stale inode
> attributes may be cached in some node.
>
> This is caused by fuse_setattr() racing with
> fuse_reverse_inval_inode().
>
> When filesystem server receives setattr request, the client node
> with valid iattr cached will be required to update the fuse_inode's
> attr_version and invalidate the cache by fuse_reverse_inval_inode(),
> and at the next call to ->getattr() they will be fetched from user
> space.
>
> The race scenario is:
> 1. client-1 sends setattr (iattr-1) request to server
> 2. client-1 receives the reply from server
> 3. before client-1 updates iattr-1 to the cached attributes by
> fuse_change_attributes_common(), server receives another setattr
> (iattr-2) request from client-2
> 4. server requests client-1 to update the inode attr_version and
> invalidate the cached iattr, and iattr-1 becomes staled
> 5. client-2 receives the reply from server, and caches iattr-2
> 6. continue with step 2, client-1 invokes
> fuse_change_attributes_common(), and caches iattr-1
>
> The issue has been observed from concurrent of chmod, chown, or
> truncate, which all invoke ->setattr() call.
>
> The solution is to use fuse_inode's attr_version to check whether
> the attributes have been modified during the setattr request's
> lifetime. If so, mark the attributes as invalid in the function
> fuse_change_attributes_common().
>
> Signed-off-by: Guang Yuan Wu <gwu@xxxxxxx>
>
> ---
> fs/fuse/dir.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 83ac192e7fdd..a961c3ed7b26 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1946,6 +1946,8 @@ int fuse_do_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> int err;
> bool trust_local_cmtime = is_wb;
> bool fault_blocked = false;
> + bool invalid_attr = false;
> + u64 attr_version;
>
> if (!fc->default_permissions)
> attr->ia_valid |= ATTR_FORCE;
> @@ -2030,6 +2032,8 @@ int fuse_do_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> if (fc->handle_killpriv_v2 && !capable(CAP_FSETID))
> inarg.valid |= FATTR_KILL_SUIDGID;
> }
> +
> + attr_version = fuse_get_attr_version(fm->fc);
> fuse_setattr_fill(fc, &args, inode, &inarg, &outarg);
> err = fuse_simple_request(fm, &args);
> if (err) {
> @@ -2055,8 +2059,14 @@ int fuse_do_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> /* FIXME: clear I_DIRTY_SYNC? */
> }
>
> + if (attr_version != 0 && fi->attr_version > attr_version)
> + /* Applying attributes, for example for fsnotify_change(), and
> + * set i_time with 0 as attributes timeout value.
> + */
> + invalid_attr = true;
> +
> fuse_change_attributes_common(inode, &outarg.attr, NULL,
> - ATTR_TIMEOUT(&outarg),
> + invalid_attr ? 0 : ATTR_TIMEOUT(&outarg),
> fuse_get_cache_mask(inode), 0);
> oldsize = inode->i_size;
> /* see the comment in fuse_change_attributes() */
>
Formatting of the commit message is still off a bit - either Miklos
needs to edit it or we need a v4 version.
For the change itself:
Reviewed-by: Bernd Schubert <bschubert@xxxxxxx>
