Hi Bernd,
I think in such case, although outarg.attr (reply from server) is staled, but it is still valid attr (pass above fuse_invalid_attr() check).
set it and then mark it as stale, is for fsnotify_change() consideration after ->setattr() returns, new attr value may be used and could cause potential issue if not set it before ->setattr() returns. Sure, the value may be staled and this should be checked by caller.
Later, iattr data will be revalidated from the next ->getattr() call.
I am unclear why FUSE_I_SIZE_UNSTABLE should be checked here, can you provide more detail ? Thanks.
Regards
Guang Yuan Wu
________________________________________
From: Bernd Schubert
Sent: Thursday, April 10, 2025 6:18 AM
To: Guang Yuan Wu; linux-fsdevel@xxxxxxxxxxxxxxx
Cc: mszeredi@xxxxxxxxxx
Subject: Re: [PATCH] fs/fuse: fix race between concurrent setattr from multiple nodes
On 4/9/25 16:25, Guang Yuan Wu wrote:
> 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 stale after fuse_change_attributes_common().
>
> Signed-off-by: Guang Yuan Wu <gwu@xxxxxxx>
> ---
> fs/fuse/dir.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d58f96d1e9a2..df3a6c995dc6 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1889,6 +1889,8 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> int err;
> bool trust_local_cmtime = is_wb;
> bool fault_blocked = false;
> + bool invalidate_attr = false;
> + u64 attr_version;
>
> if (!fc->default_permissions)
> attr->ia_valid |= ATTR_FORCE;
> @@ -1973,6 +1975,8 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> 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) {
> @@ -1998,9 +2002,17 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> /* FIXME: clear I_DIRTY_SYNC? */
> }
>
> + if ((attr_version != 0 && fi->attr_version > attr_version) ||
> + test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state))
> + invalidate_attr = true;
> +
> fuse_change_attributes_common(inode, &outarg.attr, NULL,
> ATTR_TIMEOUT(&outarg),
> fuse_get_cache_mask(inode), 0);
> +
> + if (invalidate_attr)
> + fuse_invalidate_attr(inode);
Thank you, I think the idea is right. Just some questions.
I wonder if we need to set attributes at all, when just invaliding
them directly after? fuse_change_attributes_i() is just bailing out then?
Also, do we need to test for FUSE_I_SIZE_UNSTABLE here (truncate related,
I think) or is just testing for the attribute version enough.
> +
> oldsize = inode->i_size;
> /* see the comment in fuse_change_attributes() */
> if (!is_wb || is_truncate)
Thanks,
Bernd
