Hi all,
On 4/16/2025 6:18 PM, Guang Yuan Wu wrote:
On 4/15/2025 9:13 PM, Miklos Szeredi wrote:
[You don't often get email from miklos@xxxxxxxxxx. Learn why this is
important at https://aka.ms/LearnAboutSenderIdentification ]
On Tue, 15 Apr 2025 at 04:28, Guang Yuan Wu <gwu@xxxxxxx> wrote:
I though about this ...
Actually, FUSE_I_SIZE_UNSTABLE can be set concurrently, by truncate
and other flow, and if the bit is ONLY set from truncate case, we can
trust attributes, but other flow may set it as well.
FUSE_I_SIZE_UNSTABLE is set with the inode lock held exclusive. If
this wasn't the case, the FUSE_I_SIZE_UNSTABLE state could become
corrupted (i.e it doesn't nest).
Thanks.
for truncate, inode lock is acquired in do_truncate(). (not in i_op -
>setattr())
others (for example, fallocate), inode lock is acquired in f_op-
>fallocate()
So, I think FUSE_I_SIZE_UNSTABLE check can be removed from:
if ((attr_version != 0 && fi->attr_version > attr_version) ||
test_bit(REDFS_I_SIZE_UNSTABLE, &fi->state))
/* Applying attributes, for example for fsnotify_change() */
invalidate_attr = true;
Thanks,
Miklos
Regards
Guang Yuan Wu
After address Bernd's and Miklos's comment (Removal of
FUSE_I_SIZE_UNSTABLE check and add some comment), update the patch as below:
V2:
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>
Reviewed-by: Bernd Schubert <bschubert@xxxxxxx>
Reviewed-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
---
fs/fuse/dir.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 83ac192e7fdd..0cc5a07a42e6 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 invalidate_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,9 +2059,17 @@ 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() */
+ 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);
+
oldsize = inode->i_size;
/* see the comment in fuse_change_attributes() */
if (!is_wb || is_truncate)
(END)
Regards
Guang Yuan Wu
