On 4/10/25 10:21, Guang Yuan Wu wrote:
>
> 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.
<moved comments inline>
> 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.
Good point about fsnotify_change(), would you mind to add a comment about
that?
+ if ((attr_version != 0 && fi->attr_version > attr_version) ||
+ test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) {
+ /* Applying attributes, for example for fsnotify_change() */
+ invalidate_attr = true;
>
> I am unclear why FUSE_I_SIZE_UNSTABLE should be checked here, can you
> provide more detail ? Thanks.
The function itself is setting it on truncate - we can trust attributes
in that case. I think if we want to test for FUSE_I_SIZE_UNSTABLE, it
should check for
if ((attr_version != 0 && fi->attr_version > attr_version) ||
(test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) && !truncate))
Thanks,
Bernd
