[60/70] NFSd: Ensure lookup_clientid() takes client_lock

Commit Message

Trond Myklebust April 18, 2014, 6:44 p.m. UTC

Ensure that the client lookup is done safely under the client_lock.

Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig May 5, 2014, 8:53 a.m. UTC | #1

>  static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses)
>  {
>  	__be32 status;
> @@ -3614,18 +3623,27 @@ static __be32 lookup_clientid(clientid_t *clid, struct nfsd4_session *session, s
>  {
>  	struct nfs4_client *found;
>  
> +	spin_lock(&nn->client_lock);
>  	if (session != NULL) {
>  		found = session->se_client;
>  		if (!same_clid(&found->cl_clientid, clid))
> -			return nfserr_stale_clientid;
> +			goto out_stale;

Do we really need the lock for the sessions case?  I don't tink
se_client can change under us.

Btw, I wonder if it makes sense to split the current lookup_clientid
into two helpers for the sessions vs non-sessions case as all but one
caller already have conditionals for 4.0 vs later anyway, so something
like:

static struct nfs4_client *client_from_session(struct nfsd4_session *session,
		clientid_t *clid)
{
	if (!same_clid(&session->se_client->cl_clientid, clid))
		return NULL;
	return session->se_client;
}

static __be32 lookup_40_client(clientid_t *clid, struct nfsd_net *nn,
	struct nfs4_client **clp)
{
	...
}

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6a0c201c670f..ad79dca31cc4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -178,6 +178,15 @@  static void put_client_renew_locked(struct nfs4_client *clp)
 		renew_client_locked(clp);
 }
 
+static void put_client_renew(struct nfs4_client *clp)
+{
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+
+	spin_lock(&nn->client_lock);
+	put_client_renew_locked(clp);
+	spin_unlock(&nn->client_lock);
+}
+
 static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses)
 {
 	__be32 status;
@@ -3614,18 +3623,27 @@  static __be32 lookup_clientid(clientid_t *clid, struct nfsd4_session *session, s
 {
 	struct nfs4_client *found;
 
+	spin_lock(&nn->client_lock);
 	if (session != NULL) {
 		found = session->se_client;
 		if (!same_clid(&found->cl_clientid, clid))
-			return nfserr_stale_clientid;
+			goto out_stale;
 	} else {
 		if (STALE_CLIENTID(clid, nn))
-			return nfserr_stale_clientid;
+			goto out_stale;
 		found = find_confirmed_client(clid, false, nn);
 	}
-	if (clp)
-		*clp = found;
+	if (clp) {
+		if (get_client_locked(found) == nfs_ok)
+			*clp = found;
+		else
+			found = NULL;
+	}
+	spin_unlock(&nn->client_lock);
 	return found ? nfs_ok : nfserr_expired;
+out_stale:
+	spin_unlock(&nn->client_lock);
+	return nfserr_stale_clientid;
 }
 
 __be32
@@ -3645,8 +3663,10 @@  nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	status = nfserr_cb_path_down;
 	if (!list_empty(&clp->cl_delegations)
 			&& clp->cl_cb_state != NFSD4_CB_UP)
-		goto out;
+		goto put_client;
 	status = nfs_ok;
+put_client:
+	put_client_renew(clp);
 out:
 	nfs4_unlock_state();
 	return status;
@@ -3939,6 +3959,7 @@  static __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 	if (status)
 		return status;
 	*s = find_stateid_by_type(cl, stateid, typemask);
+	put_client_renew(cl);
 	if (!*s)
 		return nfserr_bad_stateid;
 	return nfs_ok;
@@ -5119,7 +5140,9 @@  nfs4_check_open_reclaim(clientid_t *clid, struct nfsd4_session *session, struct
 	if (status)
 		return nfserr_reclaim_bad;
 
-	return nfsd4_client_record_check(clp) ? nfserr_reclaim_bad : nfs_ok;
+	status = nfsd4_client_record_check(clp) ? nfserr_reclaim_bad : nfs_ok;
+	put_client_renew(clp);
+	return status;
 }
 
 #ifdef CONFIG_NFSD_FAULT_INJECTION