9951 hook_stack_notify_unregister can leave stack locked

Reviewed by: Andy Fiddaman <omnios@citrus-it.net>
Reviewed by: Igor Kozhukhov <igor@dilos.org>
Reviewed by: Yuri Pankov <yuripv@yuripv.net>
Approved by: Dan McDonald <danmcd@joyent.com>

1 files changed, 46 insertions, 31 deletions

diff --git a/usr/src/uts/common/io/hook.c b/usr/src/uts/common/io/hook.c
index 2edbe752ce..c3ebfa0e47 100644
--- a/usr/src/uts/common/io/hook.c
+++ b/usr/src/uts/common/io/hook.c
@@ -22,7 +22,7 @@
  * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  *
- * Copyright 2013 Joyent, Inc.  All rights reserved.
+ * Copyright 2018 Joyent, Inc.  All rights reserved.
  * Copyright (c) 2016 by Delphix. All rights reserved.
  */
 #include <sys/param.h>
@@ -707,50 +707,65 @@ hook_stack_notify_unregister(netstackid_t stackid, hook_notify_fn_t callback)
 {
 	hook_family_int_t *hfi;
 	hook_stack_t *hks;
-	boolean_t canrun;
 	char buffer[16];
 	void *arg;
 	int error;
 
 	mutex_enter(&hook_stack_lock);
 	hks = hook_stack_get(stackid);
-	if (hks != NULL) {
-		CVW_ENTER_WRITE(&hks->hks_lock);
-		canrun = (hook_wait_setflag(&hks->hks_waiter, FWF_ADD_WAIT_MASK,
-		    FWF_ADD_WANTED, FWF_ADD_ACTIVE) != -1);
+	if (hks == NULL) {
+		mutex_exit(&hook_stack_lock);
+		return (ESRCH);
+	}
 
-		error = hook_notify_unregister(&hks->hks_nhead, callback, &arg);
+	CVW_ENTER_WRITE(&hks->hks_lock);
+	/*
+	 * If hook_wait_setflag returns -1, another thread has flagged that it
+	 * is attempting to destroy this hook stack.  Before it can flag that
+	 * it's destroying the hook stack, it must first verify (with
+	 * hook_stack_lock held) that the hook stack is empty.  If we
+	 * encounter this, it means we should have nothing to do and we
+	 * just snuck in.
+	 */
+	if (hook_wait_setflag(&hks->hks_waiter, FWF_DEL_WAIT_MASK,
+	    FWF_DEL_WANTED, FWF_DEL_ACTIVE) == -1) {
+		VERIFY(TAILQ_EMPTY(&hks->hks_nhead));
 		CVW_EXIT_WRITE(&hks->hks_lock);
-	} else {
-		error = ESRCH;
+		mutex_exit(&hook_stack_lock);
+		return (ESRCH);
 	}
+
+	error = hook_notify_unregister(&hks->hks_nhead, callback, &arg);
+	CVW_EXIT_WRITE(&hks->hks_lock);
 	mutex_exit(&hook_stack_lock);
 
 	if (error == 0) {
-		if (canrun) {
-			/*
-			 * Generate fake unregister event for callback that
-			 * is being removed, letting it know everything that
-			 * currently exists is now "disappearing."
-			 */
-			(void) snprintf(buffer, sizeof (buffer), "%u",
-			    hks->hks_netstackid);
-
-			SLIST_FOREACH(hfi, &hks->hks_familylist, hfi_entry) {
-				callback(HN_UNREGISTER, arg, buffer, NULL,
-				    hfi->hfi_family.hf_name);
-			}
+		/*
+		 * Generate fake unregister event for callback that
+		 * is being removed, letting it know everything that
+		 * currently exists is now "disappearing."
+		 */
+		(void) snprintf(buffer, sizeof (buffer), "%u",
+		    hks->hks_netstackid);
 
-			hook_wait_unsetflag(&hks->hks_waiter, FWF_ADD_ACTIVE);
+		SLIST_FOREACH(hfi, &hks->hks_familylist, hfi_entry) {
+			callback(HN_UNREGISTER, arg, buffer, NULL,
+			    hfi->hfi_family.hf_name);
 		}
-
-		mutex_enter(&hook_stack_lock);
-		hks = hook_stack_get(stackid);
-		if ((error == 0) && (hks->hks_shutdown == 2))
-			hook_stack_remove(hks);
-		mutex_exit(&hook_stack_lock);
+	} else {
+		/*
+		 * hook_notify_unregister() should only fail if the callback has
+		 * already been deleted (ESRCH).
+		 */
+		VERIFY3S(error, ==, ESRCH);
 	}
 
+	mutex_enter(&hook_stack_lock);
+	hook_wait_unsetflag(&hks->hks_waiter, FWF_DEL_ACTIVE);
+	if (hks->hks_shutdown == 2)
+		hook_stack_remove(hks);
+	mutex_exit(&hook_stack_lock);
+
 	return (error);
 }
 
@@ -1126,7 +1141,7 @@ hook_family_copy(hook_family_t *src)
  * Parameters:	family(I) - family name string
  *
  * Search family list with family name
- * 	A lock on hfi_lock must be held when called.
+ *	A lock on hfi_lock must be held when called.
  */
 static hook_family_int_t *
 hook_family_find(char *family, hook_stack_t *hks)
@@ -1651,7 +1666,7 @@ hook_event_copy(hook_event_t *src)
  *		event(I) - event name string
  *
  * Search event list with event name
- * 	A lock on hfi->hfi_lock must be held when called.
+ *	A lock on hfi->hfi_lock must be held when called.
  */
 static hook_event_int_t *
 hook_event_find(hook_family_int_t *hfi, char *event)