2022-01-27 01:12:58

by Dai Ngo

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

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 <[email protected]>
---
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

2022-01-28 08:37:21

by Chuck Lever

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

Hi Dai-

> On Jan 26, 2022, at 4:13 PM, Dai Ngo <[email protected]> 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 <[email protected]>

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

2022-01-28 16:39:29

by J. Bruce Fields

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

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 <[email protected]> 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 <[email protected]>
>
> 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
>
>

2022-01-31 11:05:13

by Chuck Lever

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

> On Jan 27, 2022, at 2:42 PM, J. Bruce Fields <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>
>> 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

2022-01-31 11:11:14

by J. Bruce Fields

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

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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>
> >> 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.