* [PATCH 07/44] nfsd41: create_session check replay first @ 2009-06-16 1:19 Benny Halevy 2009-06-16 20:47 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Benny Halevy @ 2009-06-16 1:19 UTC (permalink / raw) To: bfields; +Cc: pnfs, linux-nfs From: Andy Adamson <andros@netapp.com> Replay processing needs to preceed other error processing. Signed-off-by: Andy Adamson <andros@netapp.com> Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 07/44] nfsd41: create_session check replay first 2009-06-16 1:19 [PATCH 07/44] nfsd41: create_session check replay first Benny Halevy @ 2009-06-16 20:47 ` J. Bruce Fields 2009-06-16 21:16 ` J. Bruce Fields 2009-06-17 1:15 ` William A. (Andy) Adamson 0 siblings, 2 replies; 7+ messages in thread From: J. Bruce Fields @ 2009-06-16 20:47 UTC (permalink / raw) To: Benny Halevy; +Cc: pnfs, linux-nfs, andros On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: > From: Andy Adamson <andros@netapp.com> > > Replay processing needs to preceed other error processing. Why? --b. > > Signed-off-by: Andy Adamson <andros@netapp.com> > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > --- > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 07/44] nfsd41: create_session check replay first 2009-06-16 20:47 ` J. Bruce Fields @ 2009-06-16 21:16 ` J. Bruce Fields 2009-06-17 1:22 ` [pnfs] " William A. (Andy) Adamson 2009-06-17 1:15 ` William A. (Andy) Adamson 1 sibling, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2009-06-16 21:16 UTC (permalink / raw) To: Benny Halevy; +Cc: pnfs, linux-nfs, andros On Tue, Jun 16, 2009 at 04:47:34PM -0400, bfields wrote: > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: > > From: Andy Adamson <andros@netapp.com> > > > > Replay processing needs to preceed other error processing. > > Why? Note: there's a slight leak of information here, which I don't think is really important, but still by default would rather avoid: if a replay is sent by someone other than the sender of the original create_session, then sending back the cached result means they get to see a create_session result that otherwise would have required them to sniff the network (or decrypt a packet sent with the original user's creds, if krb5p was in effect). I doubt that's a big deal: I don't see anything that looks security-critical in the create_session results. But absent any real reason to do otherwise, I'd rather check the creds before checking for replay. (Note: this is more critical in the case of the sessions DRC: e.g. if the cached result includes read data, then we could end up exposing filesystem data to someone who wouldn't otherwise be able to see it.) --b. > > --b. > > > > > Signed-off-by: Andy Adamson <andros@netapp.com> > > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first 2009-06-16 21:16 ` J. Bruce Fields @ 2009-06-17 1:22 ` William A. (Andy) Adamson 0 siblings, 0 replies; 7+ messages in thread From: William A. (Andy) Adamson @ 2009-06-17 1:22 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs On Tue, Jun 16, 2009 at 5:16 PM, J. Bruce Fields<bfields@fieldses.org> wrote: > On Tue, Jun 16, 2009 at 04:47:34PM -0400, bfields wrote: >> On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: >> > From: Andy Adamson <andros@netapp.com> >> > >> > Replay processing needs to preceed other error processing. >> >> Why? > > Note: there's a slight leak of information here, which I don't think is > really important, but still by default would rather avoid: if a replay > is sent by someone other than the sender of the original create_session, > then sending back the cached result means they get to see a > create_session result that otherwise would have required them to sniff > the network (or decrypt a packet sent with the original user's creds, if > krb5p was in effect). > > I doubt that's a big deal: I don't see anything that looks > security-critical in the create_session results. But absent any real > reason to do otherwise, I'd rather check the creds before checking for > replay. > > (Note: this is more critical in the case of the sessions DRC: e.g. if > the cached result includes read data, then we could end up exposing > filesystem data to someone who wouldn't otherwise be able to see it.) We should figure out credential checking for both create session and sequence drc... -->Andy > > --b. > >> >> --b. >> >> > >> > Signed-off-by: Andy Adamson <andros@netapp.com> >> > Signed-off-by: Benny Halevy <bhalevy@panasas.com> >> > --- >> > 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 > pNFS@linux-nfs.org > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first 2009-06-16 20:47 ` J. Bruce Fields 2009-06-16 21:16 ` J. Bruce Fields @ 2009-06-17 1:15 ` William A. (Andy) Adamson [not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 7+ messages in thread From: William A. (Andy) Adamson @ 2009-06-17 1:15 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<bfields@fieldses.org> wrote: > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: >> From: Andy Adamson <andros@netapp.com> >> >> 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. ->Andy > --b. > >> >> Signed-off-by: Andy Adamson <andros@netapp.com> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >> --- >> 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 > pNFS@linux-nfs.org > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs > ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first [not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2009-06-17 1:25 ` J. Bruce Fields 2009-06-17 1:39 ` William A. (Andy) Adamson 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2009-06-17 1:25 UTC (permalink / raw) To: William A. (Andy) Adamson; +Cc: Benny Halevy, linux-nfs, pnfs 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<bfields@fieldses.org> wrote: > > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: > >> From: Andy Adamson <andros@netapp.com> > >> > >> 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 <andros@netapp.com> > >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> > >> --- > >> 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 > > pNFS@linux-nfs.org > > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first 2009-06-17 1:25 ` J. Bruce Fields @ 2009-06-17 1:39 ` William A. (Andy) Adamson 0 siblings, 0 replies; 7+ messages in thread From: William A. (Andy) Adamson @ 2009-06-17 1:39 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs On Tue, Jun 16, 2009 at 9:25 PM, J. Bruce Fields<bfields@fieldses.org> 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<bfields@fieldses.org> wrote: >> > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote: >> >> From: Andy Adamson <andros@netapp.com> >> >> >> >> 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 <andros@netapp.com> >> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >> >> --- >> >> 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 >> > pNFS@linux-nfs.org >> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-06-17 1:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-16 1:19 [PATCH 07/44] nfsd41: create_session check replay first Benny Halevy
2009-06-16 20:47 ` J. Bruce Fields
2009-06-16 21:16 ` J. Bruce Fields
2009-06-17 1:22 ` [pnfs] " William A. (Andy) Adamson
2009-06-17 1:15 ` William A. (Andy) Adamson
[not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-06-17 1:25 ` J. Bruce Fields
2009-06-17 1:39 ` William A. (Andy) Adamson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.