Re: [PATCH] nfsd: make a copy of struct iattr before calling notify_change

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote:
> 
> > On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@kernel.org> 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@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > 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.

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;
	...

> > +
> > + 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@kernel.org>