From 229c042113ba05893f00270477f45974b89c94ff Mon Sep 17 00:00:00 2001
From: Suhrud Patankar <suhrudpatankar@gmail.com>
Date: Wed, 21 Nov 2018 03:34:02 -0800
Subject: [PATCH] NFSv3- NLM - Fix refcount for NLM share

Windows client uses NLM_SHARE. It does not correctly take ref on the nlm state.

Test case:
Using Windows client and Eclipse IDE
1) open a workspace (hosted on NFS export)
2) create a New project - this will create bunch of files and dirs
3) Let the Eclipse IDE open and then
4) from NFS export (using windows explorer) try to delete the project dir
This crashes Ganesha with state ref count.

---
 src/SAL/nlm_state.c    |   18 +++++--
 src/SAL/state_share.c  |  120 +++++++++++++++++++++++++++++------------------
 src/include/sal_data.h |    1 +
 3 files changed, 87 insertions(+), 52 deletions(-)

diff --git a/src/SAL/nlm_state.c b/src/SAL/nlm_state.c
index f6cc73a..1678e1f 100644
--- a/src/SAL/nlm_state.c
+++ b/src/SAL/nlm_state.c
@@ -282,6 +282,7 @@ void dec_nlm_state_ref(state_t *state)
 
 		return;
 	}
+	assert(refcount == 0);
 
 	if (str_valid)
 		LogFullDebug(COMPONENT_STATE, "Try to remove {%s}", str);
@@ -468,8 +469,11 @@ int get_nlm_state(enum state_type state_type,
 
 	PTHREAD_MUTEX_init(&state->state_mutex, NULL);
 
-	if (state_type == STATE_TYPE_NLM_LOCK)
-		glist_init(&state->state_data.lock.state_locklist);
+	glist_init(&state->state_data.lock.state_locklist);
+	glist_init(&state->state_export_list);
+	glist_init(&state->state_list);
+	glist_init(&state->state_data.nlm_share.share_perclient);
+	glist_init(&state->state_owner_list);
 
 	state->state_refcount = 1;
 
@@ -485,6 +489,9 @@ int get_nlm_state(enum state_type state_type,
 
 	state_obj->obj_ops.get_ref(state_obj);
 
+	get_gsh_export_ref(state->state_export);
+	inc_state_owner_ref(state->state_owner);
+
 	rc = hashtable_setlatched(ht_nlm_states, &buffval, &buffval, &latch,
 				  false, NULL, NULL);
 
@@ -495,6 +502,9 @@ int get_nlm_state(enum state_type state_type,
 		LogCrit(COMPONENT_STATE, "Error %s, inserting {%s}",
 			hash_table_err_to_str(rc), str);
 
+		dec_state_owner_ref(state->state_owner);
+		put_gsh_export(state->state_export);
+
 		PTHREAD_MUTEX_destroy(&state->state_mutex);
 
 		/* Free the ref taken above and the state.
@@ -508,10 +518,6 @@ int get_nlm_state(enum state_type state_type,
 		return NLM4_DENIED_NOLOCKS;
 	}
 
-	get_gsh_export_ref(state->state_export);
-
-	inc_state_owner_ref(state->state_owner);
-
 	*pstate = state;
 
 	return 0;
diff --git a/src/SAL/state_share.c b/src/SAL/state_share.c
index 39a7543..4041e67 100644
--- a/src/SAL/state_share.c
+++ b/src/SAL/state_share.c
@@ -62,6 +62,13 @@ void remove_nlm_share(state_t *state)
 	state_owner_t *owner = state->state_owner;
 	state_nlm_client_t *client = owner->so_owner.so_nlm_owner.so_client;
 
+	if (! state->has_reservation) {
+		/* No share reservation has given with this state. */
+		LogFullDebug(COMPONENT_STATE,
+			"ignoring state %p as no reservation", state);
+		return;
+	}
+
 	/* Remove from share list for export */
 	PTHREAD_RWLOCK_wrlock(&op_ctx->ctx_export->lock);
 	glist_del(&state->state_export_list);
@@ -72,25 +79,70 @@ void remove_nlm_share(state_t *state)
 
 	/* Remove the share from the NSM Client list */
 	PTHREAD_MUTEX_lock(&client->slc_nsm_client->ssc_mutex);
-
 	glist_del(&state->state_data.nlm_share.share_perclient);
-
 	PTHREAD_MUTEX_unlock(&client->slc_nsm_client->ssc_mutex);
 
 	dec_nsm_client_ref(client->slc_nsm_client);
 
 	/* Remove the share from the NLM Owner list */
 	PTHREAD_MUTEX_lock(&owner->so_mutex);
-
 	glist_del(&state->state_owner_list);
-
 	PTHREAD_MUTEX_unlock(&owner->so_mutex);
 
+	state->has_reservation = false;
+
 	/* Release the state_t reference for active share. If extended FSAL
 	 * operations are supported, this will close the file when the last
 	 * reference is released.
 	 */
 	dec_state_t_ref(state);
+
+	LogFullDebug(COMPONENT_STATE, "Removing state %p done", state);
+}
+
+void add_nlm_share(struct fsal_obj_handle *obj, state_t *state)
+{
+	state_owner_t *owner = state->state_owner;
+	state_nlm_client_t *client = owner->so_owner.so_nlm_owner.so_client;
+
+	if (state->has_reservation) {
+		/* share reservation has given with this state.
+		 * the state must already be part of different lists and
+		 * ref taken for the same
+		 */
+		LogFullDebug(COMPONENT_STATE,
+			"ignoring state %p as already has reservation", state);
+		return;
+	}
+
+	inc_state_t_ref(state);
+	state->has_reservation = true;
+
+	/* Add share to list for NLM Owner */
+	PTHREAD_MUTEX_lock(&owner->so_mutex);
+	glist_add_tail(&owner->so_owner.so_nlm_owner.so_nlm_shares,
+		       &state->state_owner_list);
+	PTHREAD_MUTEX_unlock(&owner->so_mutex);
+
+	/* Add share to list for NSM Client */
+	inc_nsm_client_ref(client->slc_nsm_client);
+
+	PTHREAD_MUTEX_lock(&client->slc_nsm_client->ssc_mutex);
+	glist_add_tail(&client->slc_nsm_client->ssc_share_list,
+		       &state->state_data.nlm_share.share_perclient);
+	PTHREAD_MUTEX_unlock(&client->slc_nsm_client->ssc_mutex);
+
+	/* Add share to list for file. */
+	glist_add_tail(&obj->state_hdl->file.nlm_share_list,
+		       &state->state_list);
+
+	/* Add to share list for export */
+	PTHREAD_RWLOCK_wrlock(&op_ctx->ctx_export->lock);
+	glist_add_tail(&op_ctx->ctx_export->exp_nlm_share_list,
+		       &state->state_export_list);
+	PTHREAD_RWLOCK_unlock(&op_ctx->ctx_export->lock);
+
+	LogFullDebug(COMPONENT_STATE, "Adding state %p done", state);
 }
 
 /**
@@ -116,8 +168,9 @@ state_status_t state_nlm_share(struct fsal_obj_handle *obj,
 			       bool unshare)
 {
 	fsal_status_t fsal_status = {0, 0};
-	fsal_openflags_t openflags = 0;
-	state_nlm_client_t *client = owner->so_owner.so_nlm_owner.so_client;
+	fsal_openflags_t openflags = FSAL_O_CLOSED;
+
+	PTHREAD_MUTEX_lock(&state->state_mutex);
 
 	if (unshare) {
 		unsigned int old_access;
@@ -140,6 +193,12 @@ state_status_t state_nlm_share(struct fsal_obj_handle *obj,
 	if ((share_access & fsa_W) != 0)
 		openflags |= FSAL_O_WRITE;
 
+	if ((share_deny & fsm_DR) != 0)
+		openflags |= FSAL_O_DENY_READ;
+
+	if ((share_deny & fsm_DW) != 0)
+		openflags |= FSAL_O_DENY_WRITE;
+
 	if (openflags == FSAL_O_CLOSED) {
 		/* This share or unshare is removing the final share. The file
 		 * will be closed when the final reference to the state is
@@ -149,12 +208,6 @@ state_status_t state_nlm_share(struct fsal_obj_handle *obj,
 		goto out_unlock;
 	}
 
-	if ((share_deny & fsm_DR) != 0)
-		openflags |= FSAL_O_DENY_READ;
-
-	if ((share_deny & fsm_DW) != 0)
-		openflags |= FSAL_O_DENY_WRITE;
-
 	if (reclaim)
 		openflags |= FSAL_O_RECLAIM;
 
@@ -170,39 +223,11 @@ state_status_t state_nlm_share(struct fsal_obj_handle *obj,
 		goto out_unlock;
 	}
 
-	/* Add share to list for NLM Owner */
-	PTHREAD_MUTEX_lock(&owner->so_mutex);
-
-	glist_add_tail(&owner->so_owner.so_nlm_owner.so_nlm_shares,
-		       &state->state_owner_list);
-
-	PTHREAD_MUTEX_unlock(&owner->so_mutex);
-
-	/* Add share to list for NSM Client */
-	inc_nsm_client_ref(client->slc_nsm_client);
-
-	PTHREAD_MUTEX_lock(&client->slc_nsm_client->ssc_mutex);
-
-	glist_add_tail(&client->slc_nsm_client->ssc_share_list,
-		       &state->state_data.nlm_share.share_perclient);
-
-	PTHREAD_MUTEX_unlock(&client->slc_nsm_client->ssc_mutex);
-
-	/* Add share to list for file. */
-	glist_add_tail(&obj->state_hdl->file.nlm_share_list,
-		       &state->state_list);
-
-	/* Add to share list for export */
-	PTHREAD_RWLOCK_wrlock(&op_ctx->ctx_export->lock);
-	glist_add_tail(&op_ctx->ctx_export->exp_nlm_share_list,
-		       &state->state_export_list);
-	PTHREAD_RWLOCK_unlock(&op_ctx->ctx_export->lock);
-
-	/* If we had never had a share, take a reference on the state_t
-	 * to retain it.
+	/* If no share given with this state
+	 * Add the state to different lists and take ref on it.
 	 */
-	if (state->state_data.nlm_share.share_access != OPEN4_SHARE_ACCESS_NONE)
-		inc_state_t_ref(state);
+	if (! state->has_reservation)
+		add_nlm_share(obj, state);
 
 	/* Update the current share type */
 	state->state_data.nlm_share.share_access = share_access;
@@ -212,7 +237,7 @@ state_status_t state_nlm_share(struct fsal_obj_handle *obj,
 		     share_access, share_deny);
 
  out_unlock:
-
+	PTHREAD_MUTEX_unlock(&state->state_mutex);
 	return state_error_convert(fsal_status);
 }
 
@@ -229,8 +254,10 @@ void state_share_wipe(struct state_hdl *hstate)
 
 	glist_for_each_safe(glist, glistn, &hstate->file.nlm_share_list) {
 		state = glist_entry(glist, state_t, state_list);
-
+		LogFullDebug(COMPONENT_STATE, "state %p", state);
+		PTHREAD_MUTEX_lock(&state->state_mutex);
 		remove_nlm_share(state);
+		PTHREAD_MUTEX_unlock(&state->state_mutex);
 	}
 }
 
@@ -254,6 +281,7 @@ void state_export_unshare_all(void)
 			PTHREAD_RWLOCK_unlock(&op_ctx->ctx_export->lock);
 			break;
 		}
+		LogFullDebug(COMPONENT_STATE, "state %p", state);
 
 		obj = get_state_obj_ref(state);
 
diff --git a/src/include/sal_data.h b/src/include/sal_data.h
index 3af9bd8..8ef89c0 100644
--- a/src/include/sal_data.h
+++ b/src/include/sal_data.h
@@ -387,6 +387,7 @@ struct state_t {
 	struct state_refer state_refer;	/**< For NFSv4.1, track the
 					   call that created a
 					   state. */
+	bool has_reservation; /**< True if any reservation is given */
 };
 
 /* Macros to compare and copy state_t to a struct stateid4 */
-- 
1.7.1

