> > + if (ip_addr != conf->cl_addr &&
> > + !(exid->flags & EXCHGID4_FLAG_UPD_CONFIRMED_REC_A))
> {
> > + /* Client collision. 18.35.4 case 3 */
> > + status = nfserr_clid_inuse;
> > + goto out;
> > + }
> > + /*
> > + * Set bit when the owner id and verifier map to an
> already
> > + * confirmed client id (18.35.3).
> > + */
> > + exid->flags |= EXCHGID4_FLAG_CONFIRMED_R;
> > +
> > + /*
> > + * Falling into 18.35.4 case 2, possible router replay.
>
I wrote:
> Checking the spec: case 2 says: "If the server has the following
> confirmed record, and the request does not have
> EXCHGID4_FLAG_UPD_CONFIRMED_REC_A set,..."
>
> But that flag *is* set when we get to this code.
>
On Tue, Mar 31, 2009 at 04:59:38PM -0400, Sager, Mike wrote:
> Isn't this case 6?
>
> I don't think EXCHGID4_FLAG_UPD_CONFIRMED_REC_A is necessarily set when
> we get here. If the verifier, credentials, and ip addresses match, we
> can get here without the flag being set. The comment should be updated,
> but I think it actually covers both cases 2 and 6 here with the code
> snippet under out_copy. It doesn't seem to make sense to essentially
> ignore the value of the flag, but I think the end result is the
> same...at least based on my interpretation of the spec :)
OK, maybe--though in that case the ip_addr/exid->flags conditional
should probably just be removed entirely?
In any case someone needs to take a careful look at this function.
--b.
Yes, I guess it should be removed. It's in there in an attempt to
detect a client collision (case 3: same client owner, different ip, flag
not set), but as you and Benny have pointed out, basing it on an ip
check is flawed anyways...
Mike
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
Sent: Wednesday, April 01, 2009 11:02 AM
To: Sager, Mike
Cc: Benny Halevy; [email protected]; [email protected]
Subject: Re: [pnfs] [PATCH v2 15/47] nfsd41: exchange_id operation
> > + if (ip_addr != conf->cl_addr &&
> > + !(exid->flags & EXCHGID4_FLAG_UPD_CONFIRMED_REC_A))
> {
> > + /* Client collision. 18.35.4 case 3 */
> > + status = nfserr_clid_inuse;
> > + goto out;
> > + }
> > + /*
> > + * Set bit when the owner id and verifier map to an
> already
> > + * confirmed client id (18.35.3).
> > + */
> > + exid->flags |= EXCHGID4_FLAG_CONFIRMED_R;
> > +
> > + /*
> > + * Falling into 18.35.4 case 2, possible router replay.
>
I wrote:
> Checking the spec: case 2 says: "If the server has the following
> confirmed record, and the request does not have
> EXCHGID4_FLAG_UPD_CONFIRMED_REC_A set,..."
>
> But that flag *is* set when we get to this code.
>
On Tue, Mar 31, 2009 at 04:59:38PM -0400, Sager, Mike wrote:
> Isn't this case 6?
>
> I don't think EXCHGID4_FLAG_UPD_CONFIRMED_REC_A is necessarily set
> when we get here. If the verifier, credentials, and ip addresses
> match, we can get here without the flag being set. The comment
> should be updated, but I think it actually covers both cases 2 and 6
> here with the code snippet under out_copy. It doesn't seem to make
> sense to essentially ignore the value of the flag, but I think the end
> result is the same...at least based on my interpretation of the spec
> :)
OK, maybe--though in that case the ip_addr/exid->flags conditional
should probably just be removed entirely?
In any case someone needs to take a careful look at this function.
--b.