Re: [PATCH 1/1] nfsd: nfsd4_setclientid_confirm mistakenly expires confirmed client.

[bfields@fieldses.org (J. Bruce Fields)]

On Thu, Jan 27, 2022 at 03:51:54PM +0000, Chuck Lever III wrote:
> Hi Dai-
> 
> > On Jan 26, 2022, at 4:13 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> > 
> > From RFC 7530 Section 16.34.5:
> > 
> > o  The server has not recorded an unconfirmed { v, x, c, *, * } and
> >   has recorded a confirmed { v, x, c, *, s }.  If the principals of
> >   the record and of SETCLIENTID_CONFIRM do not match, the server
> >   returns NFS4ERR_CLID_INUSE without removing any relevant leased
> >   client state, and without changing recorded callback and
> >   callback_ident values for client { x }.
> > 
> > The current code intents to do what the spec describes above but
> > it forgot to set 'old' to NULL resulting to the confirmed client
> > to be expired.
> > 
> > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> 
> On it's face, this seems like the correct thing to do.
> 
> I believe the issue was introduced in commit 2b63482185e6 ("nfsd:
> fix clid_inuse on mount with security change") in 2015. I can
> add a Fixes: tag and apply this for 5.17-rc.

Looks right to me too--thanks, Dai.

--b.

> > ---
> > fs/nfsd/nfs4state.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 72900b89cf84..32063733443d 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4130,8 +4130,10 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> > 			status = nfserr_clid_inuse;
> > 			if (client_has_state(old)
> > 					&& !same_creds(&unconf->cl_cred,
> > -							&old->cl_cred))
> > +							&old->cl_cred)) {
> > +				old = NULL;
> > 				goto out;
> > +			}
> > 			status = mark_client_expired_locked(old);
> > 			if (status) {
> > 				old = NULL;
> > -- 
> > 2.9.5
> > 
> 
> --
> Chuck Lever
> 
>