On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote:
> On 7/16/2015 11:36, Kinglong Mee wrote:
> > On 7/16/2015 04:49, J. Bruce Fields wrote:
> >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> >>>> have moved gen_confirm() to gen_clid().
> >>>
> >>> This means the statement in that earlier commit is wrong:
> >>>
> >>>
> >>> With this, there's no need to keep two counters as they'd always
> >>> be in sync anyway, so just use the clientid_counter for both.
> >>>
> >>> Looks to me like this may need a separate counter to eliminate the
> >>> possibibility of returning the same confirm twice for a one clientid?
> >
> > Yes, nfsd will generate same confirm for one clientid in one second.
> >
> > verf[0] = (__force __be32)jiffies;
> > verf[1] = (__force __be32)nn->clientid_counter;
> >
> > for case 1: probable callback update, the new unconf client needs
> > a different confirm.
>
> Ignore this patch, and just revert commit 294ac32e99
> "nfsd: protect clid and verifier generation with client_lock"
> is a better solve.
We can't revert that completely, it does fix a real locking bug at
least, I think.
I'd agree to reinstating a separate counter for the verifier. That
verifier probably also needs to be per-network namespace to make the
per-network-namespace locking correct.
--b.
>
> thanks,
> Kinglong Mee
>
> >
> > Rereading rfc7530,
> > x be the value of the client.id subfield of the SETCLIENTID4args
> > structure.
> >
> > v be the value of the client.verifier subfield of the
> > SETCLIENTID4args structure.
> >
> > c be the value of the client ID field returned in the
> > SETCLIENTID4resok structure.
> >
> > k represent the value combination of the callback and callback_ident
> > fields of the SETCLIENTID4args structure.
> >
> > s be the setclientid_confirm value returned in the SETCLIENTID4resok
> > structure.
> >
> > { v, x, c, k, s } be a quintuple for a client record. A client
> > record is confirmed if there has been a SETCLIENTID_CONFIRM
> > operation to confirm it. Otherwise, it is unconfirmed. An
> > unconfirmed record is established by a SETCLIENTID call.
> >
> > ... /* case 1: probable callback update */ ...
> >
> > o The server checks if it has recorded a confirmed record for { v,
> > x, c, l, s }, where l may or may not equal k. If so, and since
> > the id verifier v of the request matches that which is confirmed
> > and recorded, the server treats this as a probable callback
> > information update and records an unconfirmed { v, x, c, k, t }
> > and leaves the confirmed { v, x, c, l, s } in place, such that
> > t != s. It does not matter whether k equals l or not. Any
> > pre-existing unconfirmed { v, x, c, *, * } is removed.
> >
> > The server returns { c, t }. It is indeed returning the old
> > clientid4 value c, because the client apparently only wants to
> > update callback value k to value l. It's possible this request is
> > one from the Byzantine router that has stale callback information,
> > but this is not a problem. The callback information update is
> > only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> >
> > The server awaits confirmation of k via SETCLIENTID_CONFIRM
> > { c, t }.
> >
> > The server does NOT remove client (lock/share/delegation) state
> > for x.
> >
> >>
> >> (but frankly I can never completely review changes to the
> >> setclientid/setclientid_confirm behavior without rereading RFC 7530
> >> 16.33.5 every time, which is a slog. Might help to contrive a pynfs
> >> test derived from that text which tests for this particular behavior.)
> >>
> >
> > Make sense.
> > I will make it later.
> >
> > thanks,
> > Kinglong Mee
> >
> >
> >>>
> >>> --b.
> >>>
> >>>>
> >>>> After it, setclientid will return a bad reply with all zero confirms
> >>>> after copy_clid().
> >>>>
> >>>> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
> >>>> ---
> >>>> fs/nfsd/nfs4state.c | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>>> index e0a4556..b1f84fc 100644
> >>>> --- a/fs/nfsd/nfs4state.c
> >>>> +++ b/fs/nfsd/nfs4state.c
> >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>> unconf = find_unconfirmed_client_by_name(&clname, nn);
> >>>> if (unconf)
> >>>> unhash_client_locked(unconf);
> >>>> - if (conf && same_verf(&conf->cl_verifier, &clverifier))
> >>>> + if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> >>>> /* case 1: probable callback update */
> >>>> copy_clid(new, conf);
> >>>> - else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>> + gen_confirm(new, nn);
> >>>> + } else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>> gen_clid(new, nn);
> >>>> new->cl_minorversion = 0;
> >>>> gen_callback(new, setclid, rqstp);
> >>>> --
> >>>> 2.4.3
> >>
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
