2009-06-17 01:25:11

by J. Bruce Fields

Subject: Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first

On Tue, Jun 16, 2009 at 09:15:32PM -0400, William A. (Andy) Adamson wrote:
> On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<[email protected]> wrote:
> > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
> >> From: Andy Adamson <[email protected]>
> >>
> >> Replay processing needs to preceed other error processing.
> >
> > Why?
> >
>
> Section 18.36.4. (CREATE_SESSION implementation section) states the
> ordering of clientid confirmation processing as
> 1) client id record lookup
> 2) sequence id processing
> 3) client id confirmation
>
> rpc cred processing is done in #3.

I don't see any reason we can't check the cred earlier--let's just leave
this as is and drop this patch.

--b.

>
> ->Andy
>
> > --b.
> >
> >>
> >> Signed-off-by: Andy Adamson <[email protected]>
> >> Signed-off-by: Benny Halevy <[email protected]>
> >> ---
> >> fs/nfsd/nfs4state.c | 12 ++++++------
> >> 1 files changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index bfc808b..5aef525 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> >> }
> >> conf->cl_slot.sl_seqid++;
> >> } else if (unconf) {
> >> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> >> - (ip_addr != unconf->cl_addr)) {
> >> - status = nfserr_clid_inuse;
> >> - goto out_cache;
> >> - }
> >> -
> >> slot = &unconf->cl_slot;
> >> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
> >> if (status) {
> >> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> >> goto out;
> >> }
> >>
> >> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> >> + (ip_addr != unconf->cl_addr)) {
> >> + status = nfserr_clid_inuse;
> >> + goto out_cache;
> >> + }
> >> +
> >> slot->sl_seqid++; /* from 0 to 1 */
> >> move_to_confirmed(unconf);
> >>
> >> --
> >> 1.6.3
> >>
> > _______________________________________________
> > pNFS mailing list
> > [email protected]
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
> >

2009-06-17 01:47:45

by Andy Adamson

Subject: Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first

On Tue, Jun 16, 2009 at 9:25 PM, J. Bruce Fields<[email protected]> wrote:
> On Tue, Jun 16, 2009 at 09:15:32PM -0400, William A. (Andy) Adamson wrote:
>> On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<[email protected]> wrote:
>> > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
>> >> From: Andy Adamson <[email protected]>
>> >>
>> >> Replay processing needs to preceed other error processing.
>> >
>> > Why?
>> >
>>
>> Section 18.36.4. (CREATE_SESSION implementation section) states the
>> ordering of clientid confirmation processing as
>> 1) client id record lookup
>> 2) sequence id processing
>> 3) client id confirmation
>>
>> rpc cred processing is done in #3.
>
> I don't see any reason we can't check the cred earlier--let's just leave
> this as is and drop this patch.

you want to replace the cache with this error? I think at the very
least you need to goto out instead of out_cache.

note that a replay of a CREATE_SESSION on an unconfirmed clientid
returns NFS4ERR_SEQMISORDERED. So it doesn't matter at all what the
rpc_cred is.

The choice is between an NFS4ERR_SEQMISORDERED and NFS4ERR_CLIDINUSE.

I think the patch should stay.

-->Andy
>
> --b.
>
>>
>> ->Andy
>>
>> > --b.
>> >
>> >>
>> >> Signed-off-by: Andy Adamson <[email protected]>
>> >> Signed-off-by: Benny Halevy <[email protected]>
>> >> ---
>> >> fs/nfsd/nfs4state.c | 12 ++++++------
>> >> 1 files changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> >> index bfc808b..5aef525 100644
>> >> --- a/fs/nfsd/nfs4state.c
>> >> +++ b/fs/nfsd/nfs4state.c
>> >> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> >> }
>> >> conf->cl_slot.sl_seqid++;
>> >> } else if (unconf) {
>> >> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> >> - (ip_addr != unconf->cl_addr)) {
>> >> - status = nfserr_clid_inuse;
>> >> - goto out_cache;
>> >> - }
>> >> -
>> >> slot = &unconf->cl_slot;
>> >> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
>> >> if (status) {
>> >> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> >> goto out;
>> >> }
>> >>
>> >> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> >> + (ip_addr != unconf->cl_addr)) {
>> >> + status = nfserr_clid_inuse;
>> >> + goto out_cache;
>> >> + }
>> >> +
>> >> slot->sl_seqid++; /* from 0 to 1 */
>> >> move_to_confirmed(unconf);
>> >>
>> >> --
>> >> 1.6.3
>> >>
>> > _______________________________________________
>> > pNFS mailing list
>> > [email protected]
>> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>> >
>