> On May 17, 2023, at 3:05 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote: >> >>> On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >>> >>> notify_change can modify the iattr structure. In particular it can can >>> end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a >>> BUG() if the same iattr is passed to notify_change more than once. >>> >>> Make a copy of the struct iattr before calling notify_change. >>> >>> Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY >>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969 >>> Reported-by: Zhi Li <yieli@xxxxxxxxxx> >>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> >>> --- >>> fs/nfsd/vfs.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>> index c4ef24c5ffd0..ad0c5cd900b1 100644 >>> --- a/fs/nfsd/vfs.c >>> +++ b/fs/nfsd/vfs.c >>> @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> >>> inode_lock(inode); >>> for (retries = 1;;) { >>> - host_err = __nfsd_setattr(dentry, iap); >>> + struct iattr attrs = *iap; >> >> This construct always makes me queazy. I'm never sure if an >> initializer inside a loop is "only once" or "every time". I >> fixed a bug like this once. >> >> But if you've tested it and it addresses the BUG, then let's >> go with this. I can apply it to nfsd-fixes. >> > > > I've done some light testing with this kernel, but this was found by Zhi > while testing with the lustre racer test, so it involves some raciness. > I've never hit this myself. Has Zhi tested this fix? > I'm pretty sure though that this has to be initialized every time. The > assignment is inside the loop, after all. I'm ok with moving the > assignment to a different line if you like though: > > struct iattr attrs; > > attrs = *iap; > ... Yeah I could do that. I find that easier to read when a loop is involved; it's unambiguous then what is going on. >>> + >>> + host_err = __nfsd_setattr(dentry, &attrs); >>> if (host_err != -EAGAIN || !retries--) >>> break; >>> if (!nfsd_wait_for_delegreturn(rqstp, inode)) >>> -- >>> 2.40.1 >>> >> >> -- >> Chuck Lever >> >> > > -- > Jeff Layton <jlayton@xxxxxxxxxx> -- Chuck Lever
