On Tue, May 23, 2023 at 09:41:14AM -0400, Jeff Layton wrote: > On Mon, 2023-05-22 at 12:45 +1000, NeilBrown wrote: > > On Thu, 18 May 2023, Jeff Layton 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; > > > + > > > + host_err = __nfsd_setattr(dentry, &attrs); > > > > I think this needs something to ensure a well meaning by-passer doesn't > > try to "optimise" it back to the way it was. > > Maybe make "iap" const? Or add a comment? Or both? > > > > We can't make iap const, as we have to call nfsd_sanitize_attrs on it, > and that will change things in it. I think we'll probably have to settle > for a comment. Chuck, can we fold the patch below in to this one? Done, and pushed to nfsd-fixes. > --------------------8<------------------ > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2a3687cdf926..817effd63730 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -538,6 +538,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > for (retries = 1;;) { > struct iattr attrs; > > + /* > + * notify_change can alter the iattr in ways that make it > + * unsuitable for submission multiple times. Make a copy > + * for every loop. > + */ > attrs = *iap; > host_err = __nfsd_setattr(dentry, &attrs); > if (host_err != -EAGAIN || !retries--) >
