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

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

[Jeff Layton]

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;
+
+		host_err = __nfsd_setattr(dentry, &attrs);
 		if (host_err != -EAGAIN || !retries--)
 			break;
 		if (!nfsd_wait_for_delegreturn(rqstp, inode))
-- 
2.40.1

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

[Chuck Lever III]

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


> +
> + host_err = __nfsd_setattr(dentry, &attrs);
> if (host_err != -EAGAIN || !retries--)
> break;
> if (!nfsd_wait_for_delegreturn(rqstp, inode))
> -- 
> 2.40.1
> 

--
Chuck Lever

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

[Jeff Layton]

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>

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

[Chuck Lever III]

> On May 17, 2023, at 3:05 PM, Jeff Layton <jlayton@kernel.org> 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@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.

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


--
Chuck Lever

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

[Jeff Layton]

On Wed, 2023-05-17 at 19:13 +0000, Chuck Lever III wrote:
> 
> > On May 17, 2023, at 3:05 PM, Jeff Layton <jlayton@kernel.org> 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@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.
> 
> Has Zhi tested this fix?
> 

Not yet. I just cooked it up this morning. I built a test kernel but
testing it will take some time since it depends on load.

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

Your call. I'm fairly certain that the patch does the right thing as-is,
but if you think it makes it more readable, then OK.
-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

On Wed, 2023-05-17 at 12:26 -0400, 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@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;
> +
> +		host_err = __nfsd_setattr(dentry, &attrs);
>  		if (host_err != -EAGAIN || !retries--)
>  			break;
>  		if (!nfsd_wait_for_delegreturn(rqstp, inode))

Zhi Li tested the test kernel for this today and this seems to have
fixed the issue. I think you can add:

Tested-by: Zhi Li <yieli@redhat.com>

Cheers,
Jeff

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

[Chuck Lever III]

> On May 19, 2023, at 6:10 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2023-05-17 at 12:26 -0400, 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@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;
>> +
>> + host_err = __nfsd_setattr(dentry, &attrs);
>> if (host_err != -EAGAIN || !retries--)
>> break;
>> if (!nfsd_wait_for_delegreturn(rqstp, inode))
> 
> Zhi Li tested the test kernel for this today and this seems to have
> fixed the issue. I think you can add:
> 
> Tested-by: Zhi Li <yieli@redhat.com>

Thanks to you both. Applied to nfsd-fixes for the next 6.4-rc PR.

--
Chuck Lever

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

[NeilBrown]

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@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;
> +
> +		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?

NeilBrown


>  		if (host_err != -EAGAIN || !retries--)
>  			break;
>  		if (!nfsd_wait_for_delegreturn(rqstp, inode))
> -- 
> 2.40.1
> 
> 

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

[Jeff Layton]

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@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;
> > +
> > +		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--)

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

[Chuck Lever]

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@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;
> > > +
> > > +		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--)
> 

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

[NeilBrown]

On Tue, 23 May 2023, 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@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;
> > > +
> > > +		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?

Thanks :-)

NeilBrown


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