diff mbox series

nfsd: make a copy of struct iattr before calling notify_change

Message ID 20230517162645.254512-1-jlayton@kernel.org
State New
Headers
Series nfsd: make a copy of struct iattr before calling notify_change |

Commit Message

Jeff Layton May 17, 2023, 4:26 p.m. UTC 
  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(-)

Comments

Chuck Lever May 17, 2023, 5:47 p.m. UTC | #1 
> 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
Jeff Layton May 17, 2023, 7:05 p.m. UTC | #2 
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
> 
>
Chuck Lever May 17, 2023, 7:13 p.m. UTC | #3 
> 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
Jeff Layton May 17, 2023, 9:37 p.m. UTC | #4 
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 May 19, 2023, 10:10 a.m. UTC | #5 
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
Chuck Lever May 19, 2023, 1:35 p.m. UTC | #6 
> 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
NeilBrown May 22, 2023, 2:45 a.m. UTC | #7 
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
> 
>
Jeff Layton May 23, 2023, 1:41 p.m. UTC | #8 
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--)
Chuck Lever May 23, 2023, 1:50 p.m. UTC | #9 
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--)
>
NeilBrown May 23, 2023, 9:57 p.m. UTC | #10 
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--)
> 
>
diff mbox series

Patch

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