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? --------------------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--)
