Re: [PATCH V3] fs/fuse: fix race between concurrent setattr from multiple nodes

[Bernd Schubert <bschubert@ddn.com>]

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@ddn.com>
> 
> ---
>  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@ddn.com>