On 4/21/25 12:23, Guang Yuan Wu wrote: > 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> Sorry for my late reply, had slipped through. I'm kind of ok with adding my Reviewed-by line, but better not in advance. Miklos also didn't add his Reviewed-by. Issue with the patch is that it does not apply # b4 am 62a4a1dd-124a-4576-9138-cdf1d4cbb638@xxxxxxx # bschubert2@imesrv6 linux.git>git am --3way ./v2_20250421_gwu_fs_fuse_fix_race_between_concurrent_setattr_from_multiple_nodes.mbx Applying: fs/fuse: fix race between concurrent setattr from multiple nodes error: corrupt patch at line 11 error: could not build fake ancestor And then looking at the patch, I think all tabs are converted to spaces - doesn't work. > > --- > 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); We could also specific 0 as timeout. fuse_change_attributes_common(inode, &outarg.attr, NULL, invalidate_attr ? 0 : ATTR_TIMEOUT(&outarg), fuse_get_cache_mask(inode), 0); > + > + if (invalidate_attr) > + fuse_invalidate_attr(inode); And then these lines wouldn't be needed. > + > oldsize = inode->i_size; > /* see the comment in fuse_change_attributes() */ > if (!is_wb || is_truncate) > (END) > > Regards > Guang Yuan Wu > Thanks, Bernd
