mainly setclientid/confirm cleanup

PATCH New J. Bruce Fields 1 participant 11 messages May 23, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2012-05-23 21:40 UTC #0 lore raw

While working on some locking fixes I ran into trouble understanding the setclientid and setclientid_confirm code. So, here's some cleanup and minor bugfixes.

The result is a lot simpler.

--b.

J. Bruce Fields

author

2012-05-23 21:40 UTC #1 lore raw

PATCH 1/10 nfsd4: setclientid remove unnecessary terms from a logical expression

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

fs/nfsd/nfs4state.c +2 -3

index 21266c7..3dc0289 100644
@@ -2344,9 +2344,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			nfsd4_probe_callback(conf);
 			status = nfs_ok;
 		}
-	} else if ((!conf || (conf && !same_verf(&conf->cl_confirm, &confirm)))
-	    && (!unconf || (unconf && !same_verf(&unconf->cl_confirm,
-				    				&confirm)))) {
+	} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
+	    && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
 		/*
 		 * RFC 3530 14.2.34 CASE 4:
 		 * Client probably hasn't noticed that we rebooted yet.

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #2 lore raw

PATCH 2/10 nfsd4: setclientid/confirm comment cleanup

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Be a little more concise.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   67 +++++++++------------------------------------------
 1 file changed, 11 insertions(+), 56 deletions(-)

fs/nfsd/nfs4state.c +11 ~~-56~~

index 3dc0289..4ce1045 100644
@@ -2169,17 +2169,13 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		return status;
 
-	/* 
-	 * XXX The Duplicate Request Cache (DRC) has been checked (??)
-	 * We get here on a DRC miss.
-	 */
-
 	strhashval = clientstr_hashval(dname);
 
+	/* Cases below refer to rfc 3530 section 14.2.33: */
 	nfs4_lock_state();
 	conf = find_confirmed_client_by_str(dname, strhashval);
 	if (conf) {
-		/* RFC 3530 14.2.33 CASE 0: */
+		/* case 0: */
 		status = nfserr_clid_inuse;
 		if (clp_used_exchangeid(conf))
 			goto out;

@@ -2192,18 +2188,10 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out;
 		}
 	}
-	/*
-	 * section 14.2.33 of RFC 3530 (under the heading "IMPLEMENTATION")
-	 * has a description of SETCLIENTID request processing consisting
-	 * of 5 bullet points, labeled as CASE0 - CASE4 below.
-	 */
 	unconf = find_unconfirmed_client_by_str(dname, strhashval);
 	status = nfserr_jukebox;
 	if (!conf) {
-		/*
-		 * RFC 3530 14.2.33 CASE 4:
-		 * placed first, because it is the normal case
-		 */
+		/* case 4: placed first, because it's the normal case */
 		if (unconf)
 			expire_client(unconf);
 		new = create_client(clname, dname, rqstp, &clverifier);

@@ -2211,10 +2199,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out;
 		gen_clid(new);
 	} else if (same_verf(&conf->cl_verifier, &clverifier)) {
-		/*
-		 * RFC 3530 14.2.33 CASE 1:
-		 * probable callback update
-		 */
+		/* case 1: probable callback update */
 		if (unconf) {
 			/* Note this is removing unconfirmed {*x***},
 			 * which is stronger than RFC recommended {vxc**}.

@@ -2228,21 +2213,13 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out;
 		copy_clid(new, conf);
 	} else if (!unconf) {
-		/*
-		 * RFC 3530 14.2.33 CASE 2:
-		 * probable client reboot; state will be removed if
-		 * confirmed.
-		 */
+		/* case 2: probable client reboot: */
 		new = create_client(clname, dname, rqstp, &clverifier);
 		if (new == NULL)
 			goto out;
 		gen_clid(new);
 	} else {
-		/*
-		 * RFC 3530 14.2.33 CASE 3:
-		 * probable client reboot; state will be removed if
-		 * confirmed.
-		 */
+		/* case 3: probable client reboot: */
 		expire_client(unconf);
 		new = create_client(clname, dname, rqstp, &clverifier);
 		if (new == NULL)

@@ -2266,11 +2243,6 @@ out:
 }
 
 
-/*
- * Section 14.2.34 of RFC 3530 (under the heading "IMPLEMENTATION") has
- * a description of SETCLIENTID_CONFIRM request processing consisting of 4
- * bullets, labeled as CASE1 - CASE4 below.
- */
 __be32
 nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			 struct nfsd4_compound_state *cstate,

@@ -2293,17 +2265,10 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 	conf = find_confirmed_client(clid);
 	unconf = find_unconfirmed_client(clid);
 
-	/*
-	 * section 14.2.34 of RFC 3530 has a description of
-	 * SETCLIENTID_CONFIRM request processing consisting
-	 * of 4 bullet points, labeled as CASE1 - CASE4 below.
-	 */
+	/* cases below refer to rfc 3530 section 14.2.34: */
 	status = nfserr_clid_inuse;
 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
-		/*
-		 * RFC 3530 14.2.34 CASE 1:
-		 * callback update
-		 */
+		/* case 1: callback update */
 		if (!same_creds(&conf->cl_cred, &unconf->cl_cred))
 			status = nfserr_clid_inuse;
 		else {

@@ -2313,21 +2278,14 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			status = nfs_ok;
 		}
 	} else if (conf && !unconf) {
-		/*
-		 * RFC 3530 14.2.34 CASE 2:
-		 * probable retransmitted request; play it safe and
-		 * do nothing.
-		 */
+		/* case 2: probable retransmit: */
 		if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
 			status = nfserr_clid_inuse;
 		else
 			status = nfs_ok;
 	} else if (!conf && unconf
 			&& same_verf(&unconf->cl_confirm, &confirm)) {
-		/*
-		 * RFC 3530 14.2.34 CASE 3:
-		 * Normal case; new or rebooted client:
-		 */
+		/* case 3: normal case; new or rebooted client */
 		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred)) {
 			status = nfserr_clid_inuse;
 		} else {

@@ -2346,10 +2304,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 		}
 	} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
 	    && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
-		/*
-		 * RFC 3530 14.2.34 CASE 4:
-		 * Client probably hasn't noticed that we rebooted yet.
-		 */
+		/* case 4: client hasn't noticed we rebooted yet? */
 		status = nfserr_stale_clientid;
 	}

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #3 lore raw

PATCH 3/10 nfsd4: merge last two setclientid cases

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

The code here is mostly the same.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

fs/nfsd/nfs4state.c +4 -9

index 4ce1045..b4e9c35 100644
@@ -2212,15 +2212,10 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		if (new == NULL)
 			goto out;
 		copy_clid(new, conf);
-	} else if (!unconf) {
-		/* case 2: probable client reboot: */
-		new = create_client(clname, dname, rqstp, &clverifier);
-		if (new == NULL)
-			goto out;
-		gen_clid(new);
-	} else {
-		/* case 3: probable client reboot: */
-		expire_client(unconf);
+	} else { /* conf && !same_verf(): */
+		/* cases 2, 3: probable client reboot: */
+		if (unconf)
+			expire_client(unconf);
 		new = create_client(clname, dname, rqstp, &clverifier);
 		if (new == NULL)
 			goto out;

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #4 lore raw

PATCH 4/10 nfsd4: pull out common code from setclientid cases

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

fs/nfsd/nfs4state.c +5 ~~-21~~

index b4e9c35..af916a7 100644
@@ -2189,36 +2189,20 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		}
 	}
 	unconf = find_unconfirmed_client_by_str(dname, strhashval);
+	if (unconf)
+		expire_client(unconf);
 	status = nfserr_jukebox;
+	new = create_client(clname, dname, rqstp, &clverifier);
+	if (new == NULL)
+		goto out;
 	if (!conf) {
 		/* case 4: placed first, because it's the normal case */
-		if (unconf)
-			expire_client(unconf);
-		new = create_client(clname, dname, rqstp, &clverifier);
-		if (new == NULL)
-			goto out;
 		gen_clid(new);
 	} else if (same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
-		if (unconf) {
-			/* Note this is removing unconfirmed {*x***},
-			 * which is stronger than RFC recommended {vxc**}.
-			 * This has the advantage that there is at most
-			 * one {*x***} in either list at any time.
-			 */
-			expire_client(unconf);
-		}
-		new = create_client(clname, dname, rqstp, &clverifier);
-		if (new == NULL)
-			goto out;
 		copy_clid(new, conf);
 	} else { /* conf && !same_verf(): */
 		/* cases 2, 3: probable client reboot: */
-		if (unconf)
-			expire_client(unconf);
-		new = create_client(clname, dname, rqstp, &clverifier);
-		if (new == NULL)
-			goto out;
 		gen_clid(new);
 	}
 	/*

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #5 lore raw

PATCH 5/10 nfsd4: merge 3 setclientid cases to 2

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Boy, is this simpler.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

fs/nfsd/nfs4state.c +2 -7

index af916a7..2d28abf 100644
@@ -2195,16 +2195,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	new = create_client(clname, dname, rqstp, &clverifier);
 	if (new == NULL)
 		goto out;
-	if (!conf) {
-		/* case 4: placed first, because it's the normal case */
-		gen_clid(new);
-	} else if (same_verf(&conf->cl_verifier, &clverifier)) {
+	if (conf && same_verf(&conf->cl_verifier, &clverifier))
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	} else { /* conf && !same_verf(): */
-		/* cases 2, 3: probable client reboot: */
+	else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new);
-	}
 	/*
 	 * XXX: we should probably set this at creation time, and check
 	 * for consistent minorversion use throughout:

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #6 lore raw

PATCH 6/10 nfsd4: fix setclientid_confirm same_cred check

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

New clients are created only by nfsd4_setclientid(), which always gives any new client a unique clientid. The only exception is in the "callback update" case, in which case it may create an unconfirmed client with the same clientid as a confirmed client. In that case it also checks that the confirmed client has the same credential.

Therefore, it is pointless for setclientid_confirm to check whether a confirmed and unconfirmed client with the same clientid have matching credentials--they're guaranteed to.

Instead, it should be checking whether the credential on the setclientid_confirm matches either of those. Otherwise, it could be anyone sending the setclientid_confirm. Granted, I can't see why anyone would, but still it's probalby safer to check.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/nfsd/nfs4state.c +1 -1

index 2d28abf..286710d 100644
@@ -2243,7 +2243,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 	status = nfserr_clid_inuse;
 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
 		/* case 1: callback update */
-		if (!same_creds(&conf->cl_cred, &unconf->cl_cred))
+		if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
 			status = nfserr_clid_inuse;
 		else {
 			nfsd4_change_callback(conf, &unconf->cl_cb_conn);

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #7 lore raw

PATCH 7/10 nfsd4: fix error return in non-matching-creds case

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Note CLID_INUSE is for the case where two clients are trying to use the same client-provided long-form client identifiers. But what we're looking at here is the server-returned shorthand client id--if those clash there's a bug somewhere.

Fix the error return, pull the check out into common code, and do the check unconditionally in all cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   62 +++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

fs/nfsd/nfs4state.c +26 ~~-36~~

index 286710d..8e77782 100644
@@ -2229,59 +2229,49 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 
 	if (STALE_CLIENTID(clid))
 		return nfserr_stale_clientid;
-	/* 
-	 * XXX The Duplicate Request Cache (DRC) has been checked (??)
-	 * We get here on a DRC miss.
-	 */
-
 	nfs4_lock_state();
 
 	conf = find_confirmed_client(clid);
 	unconf = find_unconfirmed_client(clid);
-
+	/*
+	 * We try hard to give out unique clientid's, so if we get an
+	 * attempt to confirm the same clientid with a different cred,
+	 * there's a bug somewhere.  Let's charitably assume it's our
+	 * bug.
+	 */
+	status = nfserr_serverfault;
+	if (unconf && !same_creds(&unconf->cl_cred, &rqstp->rq_cred))
+		goto out;
+	if (conf && !same_creds(&conf->cl_cred, &rqstp->rq_cred))
+		goto out;
 	/* cases below refer to rfc 3530 section 14.2.34: */
-	status = nfserr_clid_inuse;
 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
 		/* case 1: callback update */
-		if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
-			status = nfserr_clid_inuse;
-		else {
-			nfsd4_change_callback(conf, &unconf->cl_cb_conn);
-			nfsd4_probe_callback(conf);
-			expire_client(unconf);
-			status = nfs_ok;
-		}
+		nfsd4_change_callback(conf, &unconf->cl_cb_conn);
+		nfsd4_probe_callback(conf);
+		expire_client(unconf);
+		status = nfs_ok;
 	} else if (conf && !unconf) {
-		/* case 2: probable retransmit: */
-		if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
-			status = nfserr_clid_inuse;
-		else
-			status = nfs_ok;
+		status = nfs_ok;
 	} else if (!conf && unconf
 			&& same_verf(&unconf->cl_confirm, &confirm)) {
 		/* case 3: normal case; new or rebooted client */
-		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred)) {
-			status = nfserr_clid_inuse;
-		} else {
-			unsigned int hash =
-				clientstr_hashval(unconf->cl_recdir);
-			conf = find_confirmed_client_by_str(unconf->cl_recdir,
-							    hash);
-			if (conf) {
-				nfsd4_client_record_remove(conf);
-				expire_client(conf);
-			}
-			move_to_confirmed(unconf);
-			conf = unconf;
-			nfsd4_probe_callback(conf);
-			status = nfs_ok;
+		unsigned int hash = clientstr_hashval(unconf->cl_recdir);
+		conf = find_confirmed_client_by_str(unconf->cl_recdir, hash);
+		if (conf) {
+			nfsd4_client_record_remove(conf);
+			expire_client(conf);
 		}
+		move_to_confirmed(unconf);
+		conf = unconf;
+		nfsd4_probe_callback(conf);
+		status = nfs_ok;
 	} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
 	    && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
 		/* case 4: client hasn't noticed we rebooted yet? */
 		status = nfserr_stale_clientid;
 	}
-
+out:
 	nfs4_unlock_state();
 	return status;
 }

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #8 lore raw

PATCH 8/10 nfsd4: setclientid: remove pointless assignment

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/nfsd/nfs4state.c +1 -2

index 8e77782..e8e3ce0 100644
@@ -2263,8 +2263,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			expire_client(conf);
 		}
 		move_to_confirmed(unconf);
-		conf = unconf;
-		nfsd4_probe_callback(conf);
+		nfsd4_probe_callback(unconf);
 		status = nfs_ok;
 	} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
 	    && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #9 lore raw

PATCH 9/10 nfsd4: simpler ordering of setclientid_confirm checks

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

The cases here divide into two main categories:

- if there's an uncomfirmed record with a matching verifier, then this is a "normal", succesful case: we're either creating a new client, or updating an existing one.
- otherwise, this is a weird case: a replay, or a server reboot.

Reordering to reflect that makes the code a bit more concise and the logic a lot easier to understand.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

fs/nfsd/nfs4state.c +11 ~~-13~~

index e8e3ce0..72bc277 100644
@@ -2245,18 +2245,21 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 	if (conf && !same_creds(&conf->cl_cred, &rqstp->rq_cred))
 		goto out;
 	/* cases below refer to rfc 3530 section 14.2.34: */
-	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
-		/* case 1: callback update */
+	if (!unconf || !same_verf(&confirm, &unconf->cl_confirm)) {
+		if (conf && !unconf) /* case 2: probable retransmit */
+			status = nfs_ok;
+		else /* case 4: client hasn't noticed we rebooted yet? */
+			status = nfserr_stale_clientid;
+		goto out;
+	}
+	status = nfs_ok;
+	if (conf) { /* case 1: callback update */
 		nfsd4_change_callback(conf, &unconf->cl_cb_conn);
 		nfsd4_probe_callback(conf);
 		expire_client(unconf);
-		status = nfs_ok;
-	} else if (conf && !unconf) {
-		status = nfs_ok;
-	} else if (!conf && unconf
-			&& same_verf(&unconf->cl_confirm, &confirm)) {
-		/* case 3: normal case; new or rebooted client */
+	} else { /* case 3: normal case; new or rebooted client */
 		unsigned int hash = clientstr_hashval(unconf->cl_recdir);
+
 		conf = find_confirmed_client_by_str(unconf->cl_recdir, hash);
 		if (conf) {
 			nfsd4_client_record_remove(conf);

@@ -2264,11 +2267,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 		}
 		move_to_confirmed(unconf);
 		nfsd4_probe_callback(unconf);
-		status = nfs_ok;
-	} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
-	    && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
-		/* case 4: client hasn't noticed we rebooted yet? */
-		status = nfserr_stale_clientid;
 	}
 out:
 	nfs4_unlock_state();

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2012-05-23 21:40 UTC #10 lore raw

PATCH 10/10 nfsd4: clarify that renewing expired client is a bug

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

This can't happen:
- cl_time is zeroed only by unhash_client_locked, which is only ever called under both the state lock and the client lock.
- every caller of renew_client() should have looked up a (non-expired) client and then called renew_client() all without dropping the state lock.
- the only other caller of renew_client_locked() is release_session_client(), which first checks under the client_lock that the cl_time is nonzero.

So make it clear that this is a bug, not something we handle. I can't quite bring myself to make this a BUG(), though, as there are a lot of renew_client() callers, and returning here is probably safer than a BUG().

We'll consider making it a BUG() after some more cleanup.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/nfsd/nfs4state.c +2 -1

index 72bc277..be1fc97 100644
@@ -1022,7 +1022,8 @@ static inline void
 renew_client_locked(struct nfs4_client *clp)
 {
 	if (is_client_expired(clp)) {
-		dprintk("%s: client (clientid %08x/%08x) already expired\n",
+		WARN_ON(1);
+		printk("%s: client (clientid %08x/%08x) already expired\n",
 			__func__,
 			clp->cl_clientid.cl_boot,
 			clp->cl_clientid.cl_id);

Signature · 1 line

1.7.9.5