nfsd: fix change_info in NFSv4 RENAME replies

[PATCH] nfsd: fix change_info in NFSv4 RENAME replies

Posted by Jeff Layton 2 years, 3 months ago

nfsd sends the transposed directory change info in the RENAME reply. The
source directory is in save_fh and the target is in current_fh.

Reported-by: Zhi Li <yieli@redhat.com>
Reported-by: Benjamin Coddington <bcodding@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
This bug predates git, so I can't add a proper Fixes tag. I think this
is probably appropriate for stable series kernels though.

This bug was largely papered over by the fact that we factored in the
ctime when generating a change attribute. Since this commit, however:

    638e3e7d9493 nfsd: use the getattr operation to fetch i_version

We stopped doing that for directory inodes and that caused this bug to
pop up.
---
 fs/nfsd/nfs4proc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5ca748309c26..4199ede0583c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1058,8 +1058,8 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			     rename->rn_tname, rename->rn_tnamelen);
 	if (status)
 		return status;
-	set_change_info(&rename->rn_sinfo, &cstate->current_fh);
-	set_change_info(&rename->rn_tinfo, &cstate->save_fh);
+	set_change_info(&rename->rn_sinfo, &cstate->save_fh);
+	set_change_info(&rename->rn_tinfo, &cstate->current_fh);
 	return nfs_ok;
 }
 

---
base-commit: dd1386dd3c4f4bc55456c88180f9f39697bb95c0
change-id: 20230908-nfsd-fixes-f5bdb87e6035

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix change_info in NFSv4 RENAME replies

Posted by Chuck Lever 2 years, 3 months ago

On Sat, Sep 09, 2023 at 07:12:30AM -0400, Jeff Layton wrote:
> nfsd sends the transposed directory change info in the RENAME reply. The
> source directory is in save_fh and the target is in current_fh.
> 
> Reported-by: Zhi Li <yieli@redhat.com>
> Reported-by: Benjamin Coddington <bcodding@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This bug predates git, so I can't add a proper Fixes tag. I think this
> is probably appropriate for stable series kernels though.
> 
> This bug was largely papered over by the fact that we factored in the
> ctime when generating a change attribute. Since this commit, however:
> 
>     638e3e7d9493 nfsd: use the getattr operation to fetch i_version
> 
> We stopped doing that for directory inodes and that caused this bug to
> pop up.

Applied to nfsd-fixes with a "Cc: stable" tag added.

Is there a publicly-accessible bug report link that can be included?


> ---
>  fs/nfsd/nfs4proc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5ca748309c26..4199ede0583c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1058,8 +1058,8 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			     rename->rn_tname, rename->rn_tnamelen);
>  	if (status)
>  		return status;
> -	set_change_info(&rename->rn_sinfo, &cstate->current_fh);
> -	set_change_info(&rename->rn_tinfo, &cstate->save_fh);
> +	set_change_info(&rename->rn_sinfo, &cstate->save_fh);
> +	set_change_info(&rename->rn_tinfo, &cstate->current_fh);
>  	return nfs_ok;
>  }
>  
> 
> ---
> base-commit: dd1386dd3c4f4bc55456c88180f9f39697bb95c0
> change-id: 20230908-nfsd-fixes-f5bdb87e6035
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever

Re: [PATCH] nfsd: fix change_info in NFSv4 RENAME replies

Posted by Jeff Layton 2 years, 3 months ago

On Sat, 2023-09-09 at 11:18 -0400, Chuck Lever wrote:
> On Sat, Sep 09, 2023 at 07:12:30AM -0400, Jeff Layton wrote:
> > nfsd sends the transposed directory change info in the RENAME reply. The
> > source directory is in save_fh and the target is in current_fh.
> > 
> > Reported-by: Zhi Li <yieli@redhat.com>
> > Reported-by: Benjamin Coddington <bcodding@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This bug predates git, so I can't add a proper Fixes tag. I think this
> > is probably appropriate for stable series kernels though.
> > 
> > This bug was largely papered over by the fact that we factored in the
> > ctime when generating a change attribute. Since this commit, however:
> > 
> >     638e3e7d9493 nfsd: use the getattr operation to fetch i_version
> > 
> > We stopped doing that for directory inodes and that caused this bug to
> > pop up.
> 
> Applied to nfsd-fixes with a "Cc: stable" tag added.
> 
> Is there a publicly-accessible bug report link that can be included?
> 
> 

Yes. I just made this one public:

    https://bugzilla.redhat.com/show_bug.cgi?id=2218844

It has Ben's reproducer script for this in it too.

> > ---
> >  fs/nfsd/nfs4proc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 5ca748309c26..4199ede0583c 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1058,8 +1058,8 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  			     rename->rn_tname, rename->rn_tnamelen);
> >  	if (status)
> >  		return status;
> > -	set_change_info(&rename->rn_sinfo, &cstate->current_fh);
> > -	set_change_info(&rename->rn_tinfo, &cstate->save_fh);
> > +	set_change_info(&rename->rn_sinfo, &cstate->save_fh);
> > +	set_change_info(&rename->rn_tinfo, &cstate->current_fh);
> >  	return nfs_ok;
> >  }
> >  
> > 
> > ---
> > base-commit: dd1386dd3c4f4bc55456c88180f9f39697bb95c0
> > change-id: 20230908-nfsd-fixes-f5bdb87e6035
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix change_info in NFSv4 RENAME replies

Posted by Chuck Lever III 2 years, 3 months ago


> On Sep 9, 2023, at 1:23 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Sat, 2023-09-09 at 11:18 -0400, Chuck Lever wrote:
>> On Sat, Sep 09, 2023 at 07:12:30AM -0400, Jeff Layton wrote:
>>> nfsd sends the transposed directory change info in the RENAME reply. The
>>> source directory is in save_fh and the target is in current_fh.
>>> 
>>> Reported-by: Zhi Li <yieli@redhat.com>
>>> Reported-by: Benjamin Coddington <bcodding@redhat.com>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> This bug predates git, so I can't add a proper Fixes tag. I think this
>>> is probably appropriate for stable series kernels though.
>>> 
>>> This bug was largely papered over by the fact that we factored in the
>>> ctime when generating a change attribute. Since this commit, however:
>>> 
>>>    638e3e7d9493 nfsd: use the getattr operation to fetch i_version
>>> 
>>> We stopped doing that for directory inodes and that caused this bug to
>>> pop up.
>> 
>> Applied to nfsd-fixes with a "Cc: stable" tag added.
>> 
>> Is there a publicly-accessible bug report link that can be included?
>> 
>> 
> 
> Yes. I just made this one public:
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=2218844
> 
> It has Ben's reproducer script for this in it too.

Thanks, added in my private repo, to be pushed out later.


>>> ---
>>> fs/nfsd/nfs4proc.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 5ca748309c26..4199ede0583c 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1058,8 +1058,8 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>      rename->rn_tname, rename->rn_tnamelen);
>>> if (status)
>>> return status;
>>> - set_change_info(&rename->rn_sinfo, &cstate->current_fh);
>>> - set_change_info(&rename->rn_tinfo, &cstate->save_fh);
>>> + set_change_info(&rename->rn_sinfo, &cstate->save_fh);
>>> + set_change_info(&rename->rn_tinfo, &cstate->current_fh);
>>> return nfs_ok;
>>> }
>>> 
>>> 
>>> ---
>>> base-commit: dd1386dd3c4f4bc55456c88180f9f39697bb95c0
>>> change-id: 20230908-nfsd-fixes-f5bdb87e6035
>>> 
>>> Best regards,
>>> -- 
>>> Jeff Layton <jlayton@kernel.org>
>>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>


--
Chuck Lever