| Message ID | 1643231618-24342-1-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] nfsd: nfsd4_setclientid_confirm mistakenly expires confirmed client. | expand |
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. > --- > 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
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 > >
> On Jan 27, 2022, at 2:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > 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. May I add a Reviewed-by: Bruce ? > > --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 -- Chuck Lever
On Fri, Jan 28, 2022 at 02:02:57PM +0000, Chuck Lever III wrote: > > > > On Jan 27, 2022, at 2:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > 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. > > May I add a Reviewed-by: Bruce ? Sure.--b.
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;
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> --- fs/nfsd/nfs4state.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)