| Message ID | 166260292002.30452.4279820246560338475@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: fix regression with setting ACLs. | expand |
On Thu, 2022-09-08 at 12:08 +1000, NeilBrown wrote: > A recent patch moved ACL setting into nfsd_setattr(). > Unfortunately it didn't work as nfsd_setattr() aborts early if > iap->ia_valid is 0. > > Remove this test, and instead avoid calling notify_change() when > ia_valid is 0. > > This means that nfsd_setattr() will now *always* lock the inode. > Previously it didn't if only a ATTR_MODE change was requested on a > symlink (see Commit 15b7a1b86d66 ("[PATCH] knfsd: fix setattr-on-symlink > error return")). I don't think this change really matters. > > Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/vfs.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 9f486b788ed0..bffb31540df8 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -353,7 +353,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > int accmode = NFSD_MAY_SATTR; > umode_t ftype = 0; > __be32 err; > - int host_err; > + int host_err = 0; > bool get_write_count; > bool size_change = (iap->ia_valid & ATTR_SIZE); > > @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (S_ISLNK(inode->i_mode)) > iap->ia_valid &= ~ATTR_MODE; > > - if (!iap->ia_valid) > - return 0; > - > nfsd_sanitize_attrs(inode, iap); > > if (check_guard && guardtime != inode->i_ctime.tv_sec) > @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto out_unlock; > } > > - iap->ia_valid |= ATTR_CTIME; > - host_err = notify_change(&init_user_ns, dentry, iap, NULL); > + if (iap->ia_valid) { > + iap->ia_valid |= ATTR_CTIME; > + host_err = notify_change(&init_user_ns, dentry, iap, NULL); > + } > > out_unlock: > if (attr->na_seclabel && attr->na_seclabel->len) Looks sane. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On Sep 7, 2022, at 10:08 PM, NeilBrown <neilb@suse.de> wrote: > > > A recent patch moved ACL setting into nfsd_setattr(). > Unfortunately it didn't work as nfsd_setattr() aborts early if > iap->ia_valid is 0. > > Remove this test, and instead avoid calling notify_change() when > ia_valid is 0. > > This means that nfsd_setattr() will now *always* lock the inode. > Previously it didn't if only a ATTR_MODE change was requested on a > symlink (see Commit 15b7a1b86d66 ("[PATCH] knfsd: fix setattr-on-symlink > error return")). I don't think this change really matters. > > Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/vfs.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 9f486b788ed0..bffb31540df8 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -353,7 +353,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > int accmode = NFSD_MAY_SATTR; > umode_t ftype = 0; > __be32 err; > - int host_err; > + int host_err = 0; > bool get_write_count; > bool size_change = (iap->ia_valid & ATTR_SIZE); > > @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (S_ISLNK(inode->i_mode)) > iap->ia_valid &= ~ATTR_MODE; Thank you for the fix, Neil. I've applied this to 6.0-rc. > - if (!iap->ia_valid) > - return 0; > - > nfsd_sanitize_attrs(inode, iap); > > if (check_guard && guardtime != inode->i_ctime.tv_sec) > @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto out_unlock; > } > > - iap->ia_valid |= ATTR_CTIME; > - host_err = notify_change(&init_user_ns, dentry, iap, NULL); > + if (iap->ia_valid) { > + iap->ia_valid |= ATTR_CTIME; > + host_err = notify_change(&init_user_ns, dentry, iap, NULL); > + } > > out_unlock: > if (attr->na_seclabel && attr->na_seclabel->len) > -- > 2.37.1 > -- Chuck Lever
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 9f486b788ed0..bffb31540df8 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -353,7 +353,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, int accmode = NFSD_MAY_SATTR; umode_t ftype = 0; __be32 err; - int host_err; + int host_err = 0; bool get_write_count; bool size_change = (iap->ia_valid & ATTR_SIZE); @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, if (S_ISLNK(inode->i_mode)) iap->ia_valid &= ~ATTR_MODE; - if (!iap->ia_valid) - return 0; - nfsd_sanitize_attrs(inode, iap); if (check_guard && guardtime != inode->i_ctime.tv_sec) @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, goto out_unlock; } - iap->ia_valid |= ATTR_CTIME; - host_err = notify_change(&init_user_ns, dentry, iap, NULL); + if (iap->ia_valid) { + iap->ia_valid |= ATTR_CTIME; + host_err = notify_change(&init_user_ns, dentry, iap, NULL); + } out_unlock: if (attr->na_seclabel && attr->na_seclabel->len)
A recent patch moved ACL setting into nfsd_setattr(). Unfortunately it didn't work as nfsd_setattr() aborts early if iap->ia_valid is 0. Remove this test, and instead avoid calling notify_change() when ia_valid is 0. This means that nfsd_setattr() will now *always* lock the inode. Previously it didn't if only a ATTR_MODE change was requested on a symlink (see Commit 15b7a1b86d66 ("[PATCH] knfsd: fix setattr-on-symlink error return")). I don't think this change really matters. Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs") Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/vfs.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)