* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox