Message ID | BN6PR19MB3187A23CBCF47675F539ADB6BEB42@BN6PR19MB3187.namprd19.prod.outlook.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | fs/fuse: fix race between concurrent setattr from multiple nodes | expand |
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@ddn.com> > --- > 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
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
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@vger.kernel.org > Cc: mszeredi@redhat.com > 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@ddn.com> >> --- >> 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
On 4/15/25 04:27, Guang Yuan Wu wrote: > > > ________________________________________ > From: Bernd Schubert > Sent: Monday, April 14, 2025 10:32 PM > To: Guang Yuan Wu; linux-fsdevel@vger.kernel.org > Cc: mszeredi@redhat.com > Subject: Re: [PATCH] fs/fuse: fix race between concurrent setattr from multiple nodes > > > 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@vger.kernel.org >> Cc: mszeredi@redhat.com >> 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@ddn.com> >>> --- >>> 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; >> > Ack. > >> >>> 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)) > >> 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 could be expanded to FUSE_I_SIZE_UNSTABLE_TRUNC, FUSE_I_SIZE_UNSTABLE_COPY_FILE_RANGE, FUSE_I_SIZE_UNSTABLE_FALLOCATE .... >> and then we can control this much precisely: > > if ((attr_version != 0 && fi->attr_version > attr_version) || > (test_bit(FUSE_I_SIZE_UNSTABLE_COPY_FILE_RANGE, &fi->state)) || > (test_bit(FUSE_I_SIZE_UNSTABLE_FALLOCATE, &fi->state)) ...) Better to have that in a different patch - makes review easier. Could be follow-up or preparation. I tend to follow-up as splitting the flag is an improvement, while the current patch is needed for functionality. Thanks, Bernd
On Tue, 15 Apr 2025 at 04:28, Guang Yuan Wu <gwu@ddn.com> 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, Miklos
On 4/15/2025 9:13 PM, Miklos Szeredi wrote: > [You don't often get email from miklos@szeredi.hu. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Tue, 15 Apr 2025 at 04:28, Guang Yuan Wu <gwu@ddn.com> 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
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); + oldsize = inode->i_size; /* see the comment in fuse_change_attributes() */ if (!is_wb || is_truncate)
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@ddn.com> --- fs/fuse/dir.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)