Return-Path: Received: from mail-pd0-f181.google.com ([209.85.192.181]:36780 "EHLO mail-pd0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752590AbbGPDvI (ORCPT ); Wed, 15 Jul 2015 23:51:08 -0400 Received: by pdjr16 with SMTP id r16so36563300pdj.3 for ; Wed, 15 Jul 2015 20:51:07 -0700 (PDT) Message-ID: <55A72A1F.7060507@gmail.com> Date: Thu, 16 Jul 2015 11:50:55 +0800 From: Kinglong Mee MIME-Version: 1.0 To: "J. Bruce Fields" CC: "linux-nfs@vger.kernel.org" , kinglongmee@gmail.com Subject: Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid() References: <55A384B1.8030207@gmail.com> <55A38505.6040405@gmail.com> <20150715204748.GA21669@fieldses.org> <20150715204950.GB21669@fieldses.org> <55A726A0.5090907@gmail.com> In-Reply-To: <55A726A0.5090907@gmail.com> Content-Type: text/plain; charset=windows-1252 Sender: linux-nfs-owner@vger.kernel.org List-ID: 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. 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 >>>> --- >>>> 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 >> >