8149 deadlock between datalink deletion and kstat read

Reviewed by: Steve Gonczi <steve.gonczi@delphix.com>
Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Ryan Zezeski <ryan@zinascii.com>
Approved by: Robert Mustacchi <rm@joyent.com>

6 files changed, 135 insertions, 46 deletions

diff --git a/usr/src/pkg/manifests/system-test-ostest.mf b/usr/src/pkg/manifests/system-test-ostest.mf
index c4c6d98836..23e941d67a 100644
--- a/usr/src/pkg/manifests/system-test-ostest.mf
+++ b/usr/src/pkg/manifests/system-test-ostest.mf
@@ -10,7 +10,7 @@
 #
 
 #
-# Copyright (c) 2012 by Delphix. All rights reserved.
+# Copyright (c) 2012, 2016 by Delphix. All rights reserved.
 # Copyright 2014, OmniTI Computer Consulting, Inc. All rights reserved.
 # Copyright 2016, Joyent, Inc.
 #
@@ -29,6 +29,7 @@ dir path=opt/os-tests/tests/file-locking
 dir path=opt/os-tests/tests/sdevfs
 dir path=opt/os-tests/tests/secflags
 dir path=opt/os-tests/tests/sigqueue
+dir path=opt/os-tests/tests/stress
 file path=opt/os-tests/README mode=0444
 file path=opt/os-tests/bin/ostest mode=0555
 file path=opt/os-tests/runfiles/default.run mode=0444
@@ -56,6 +57,7 @@ file path=opt/os-tests/tests/secflags/secflags_zonecfg mode=0555
 file path=opt/os-tests/tests/secflags/stacky mode=0555
 file path=opt/os-tests/tests/sigqueue/sigqueue_queue_size mode=0555
 file path=opt/os-tests/tests/spoof-ras mode=0555
+file path=opt/os-tests/tests/stress/dladm-kstat mode=0555
 license cr_Sun license=cr_Sun
 license lic_CDDL license=lic_CDDL
 depend fmri=system/test/testrunner type=require
diff --git a/usr/src/test/os-tests/runfiles/default.run b/usr/src/test/os-tests/runfiles/default.run
index 8b7e82f916..fa1049c6e6 100644
--- a/usr/src/test/os-tests/runfiles/default.run
+++ b/usr/src/test/os-tests/runfiles/default.run
@@ -48,5 +48,9 @@ tests = ['sigqueue_queue_size']
 user = root
 tests = ['sdevfs_eisdir']
 
+[/opt/os-tests/tests/stress]
+user = root
+tests = ['dladm-kstat']
+
 [/opt/os-tests/tests/file-locking]
 tests = ['runtests.32', 'runtests.64']
diff --git a/usr/src/test/os-tests/tests/Makefile b/usr/src/test/os-tests/tests/Makefile
index 6b6e0f9fce..3b2fd26082 100644
--- a/usr/src/test/os-tests/tests/Makefile
+++ b/usr/src/test/os-tests/tests/Makefile
@@ -10,9 +10,9 @@
 #
 
 #
-# Copyright (c) 2012 by Delphix. All rights reserved.
+# Copyright (c) 2012, 2016 by Delphix. All rights reserved.
 #
 
-SUBDIRS = poll secflags sigqueue spoof-ras sdevfs file-locking
+SUBDIRS = poll secflags sigqueue spoof-ras sdevfs stress file-locking
 
 include $(SRC)/test/Makefile.com
diff --git a/usr/src/test/os-tests/tests/stress/Makefile b/usr/src/test/os-tests/tests/stress/Makefile
new file mode 100644
index 0000000000..7aed99b982
--- /dev/null
+++ b/usr/src/test/os-tests/tests/stress/Makefile
@@ -0,0 +1,42 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2016 by Delphix. All rights reserved.
+#
+
+include $(SRC)/cmd/Makefile.cmd
+include $(SRC)/test/Makefile.com
+
+PROG = dladm-kstat
+
+ROOTOPTPKG = $(ROOT)/opt/os-tests
+TESTDIR = $(ROOTOPTPKG)/tests/stress
+
+CMDS = $(PROG:%=$(TESTDIR)/%)
+$(CMDS) := FILEMODE = 0555
+
+all: $(PROG)
+
+install: all $(CMDS)
+
+clobber: clean
+
+clean:
+	-$(RM) $(PROG)
+
+$(CMDS): $(TESTDIR) $(PROG)
+
+$(TESTDIR):
+	$(INS.dir)
+
+$(TESTDIR)/%: %
+	$(INS.file)
diff --git a/usr/src/test/os-tests/tests/stress/dladm-kstat.sh b/usr/src/test/os-tests/tests/stress/dladm-kstat.sh
new file mode 100755
index 0000000000..f0ab886d9f
--- /dev/null
+++ b/usr/src/test/os-tests/tests/stress/dladm-kstat.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2016 by Delphix. All rights reserved.
+#
+
+#
+# This test attempts to stress the interaction between threads adding
+# and deleting datalinks, and those reading kstats associated with
+# those datalinks.
+#
+
+RUNFILE=$(mktemp)
+linkname1=laverne0
+linkname2=shirley0
+duration=20 # seconds
+
+#
+# Delete any potential datalinks left behind by the etherstub function.
+#
+function cleanup
+{
+	rm -f $RUNFILE
+}
+
+function etherstub
+{
+	while [[ -e $RUNFILE ]]; do
+		dladm create-etherstub -t $linkname1
+		dladm rename-link $linkname1 $linkname2
+		dladm delete-etherstub -t $linkname2
+	done
+}
+
+function readkstat
+{
+	local linkname=$1
+	while [[ -e $RUNFILE ]]; do
+		kstat link:0:$linkname &>/dev/null
+	done
+}
+
+trap "cleanup; exit" SIGHUP SIGINT SIGTERM
+
+etherstub &
+readkstat $linkname1 &
+readkstat $linkname1 &
+readkstat $linkname2 &
+readkstat $linkname2 &
+
+sleep $duration
+cleanup
+
+wait
+
+exit 0
diff --git a/usr/src/uts/common/io/dls/dls_mgmt.c b/usr/src/uts/common/io/dls/dls_mgmt.c
index 049c4bd757..4473266242 100644
--- a/usr/src/uts/common/io/dls/dls_mgmt.c
+++ b/usr/src/uts/common/io/dls/dls_mgmt.c
@@ -22,6 +22,9 @@
  * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
+/*
+ * Copyright (c) 2016 by Delphix. All rights reserved.
+ */
 
 /*
  * Datalink management routines.
@@ -79,9 +82,9 @@ boolean_t		devnet_need_rebuild;
 /* Upcall door handle */
 static door_handle_t	dls_mgmt_dh = NULL;
 
+/* dls_devnet_t dd_flags */
 #define	DD_CONDEMNED		0x1
-#define	DD_KSTAT_CHANGING	0x2
-#define	DD_IMPLICIT_IPTUN	0x4 /* Implicitly-created ip*.*tun* tunnel */
+#define	DD_IMPLICIT_IPTUN	0x2 /* Implicitly-created ip*.*tun* tunnel */
 
 /*
  * This structure is used to keep the <linkid, macname> mapping.
@@ -702,27 +705,20 @@ dls_devnet_rele_link(dls_dl_handle_t dlh, dls_link_t *dlp)
 static int
 dls_devnet_stat_update(kstat_t *ksp, int rw)
 {
-	dls_devnet_t	*ddp = ksp->ks_private;
+	datalink_id_t	linkid = (datalink_id_t)(uintptr_t)ksp->ks_private;
+	dls_devnet_t	*ddp;
 	dls_link_t	*dlp;
 	int		err;
 
-	/*
-	 * Check the link is being renamed or if the link is going away
-	 * before incrementing dd_tref which in turn prevents the link
-	 * from being renamed or deleted until we finish.
-	 */
-	mutex_enter(&ddp->dd_mutex);
-	if (ddp->dd_flags & (DD_CONDEMNED | DD_KSTAT_CHANGING)) {
-		mutex_exit(&ddp->dd_mutex);
-		return (ENOENT);
+	if ((err = dls_devnet_hold_tmp(linkid, &ddp)) != 0) {
+		return (err);
 	}
-	ddp->dd_tref++;
-	mutex_exit(&ddp->dd_mutex);
 
 	/*
 	 * If a device detach happens at this time, it will block in
-	 * dls_devnet_unset since the dd_tref has been bumped up above. So the
-	 * access to 'dlp' is safe even though we don't hold the mac perimeter.
+	 * dls_devnet_unset since the dd_tref has been bumped in
+	 * dls_devnet_hold_tmp(). So the access to 'dlp' is safe even though
+	 * we don't hold the mac perimeter.
 	 */
 	if (mod_hash_find(i_dls_link_hash, (mod_hash_key_t)ddp->dd_mac,
 	    (mod_hash_val_t *)&dlp) != 0) {
@@ -745,7 +741,8 @@ dls_devnet_stat_create(dls_devnet_t *ddp, zoneid_t zoneid)
 	kstat_t	*ksp;
 
 	if (dls_stat_create("link", 0, ddp->dd_linkname, zoneid,
-	    dls_devnet_stat_update, ddp, &ksp) == 0) {
+	    dls_devnet_stat_update, (void *)(uintptr_t)ddp->dd_linkid,
+	    &ksp) == 0) {
 		ASSERT(ksp != NULL);
 		if (zoneid == ddp->dd_owner_zid) {
 			ASSERT(ddp->dd_ksp == NULL);
@@ -988,7 +985,7 @@ dls_devnet_hold_common(datalink_id_t linkid, dls_devnet_t **ddpp,
 	if (dls_mgmt_get_phydev(linkid, &phydev) == 0)
 		(void) softmac_hold_device(phydev, &ddh);
 
-	rw_enter(&i_dls_devnet_lock, RW_WRITER);
+	rw_enter(&i_dls_devnet_lock, RW_READER);
 	if ((err = mod_hash_find(i_dls_devnet_id_hash,
 	    (mod_hash_key_t)(uintptr_t)linkid, (mod_hash_val_t *)&ddp)) != 0) {
 		ASSERT(err == MH_ERR_NOTFOUND);
@@ -1069,7 +1066,7 @@ dls_devnet_hold_by_dev(dev_t dev, dls_dl_handle_t *ddhp)
 	if (DLS_MINOR2INST(getminor(dev)) <= DLS_MAX_PPA)
 		(void) softmac_hold_device(dev, &ddh);
 
-	rw_enter(&i_dls_devnet_lock, RW_WRITER);
+	rw_enter(&i_dls_devnet_lock, RW_READER);
 	if ((err = mod_hash_find(i_dls_devnet_hash,
 	    (mod_hash_key_t)name, (mod_hash_val_t *)&ddp)) != 0) {
 		ASSERT(err == MH_ERR_NOTFOUND);
@@ -1272,7 +1269,6 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link)
 	mac_perim_handle_t	mph = NULL;
 	mac_handle_t		mh;
 	mod_hash_val_t		val;
-	boolean_t		clear_dd_flag = B_FALSE;
 
 	/*
 	 * In the second case, id2 must be a REMOVED physical link.
@@ -1308,22 +1304,12 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link)
 		goto done;
 	}
 
-	/*
-	 * Return EBUSY if any applications have this link open, if any thread
-	 * is currently accessing the link kstats, or if the link is on-loan
-	 * to a non-global zone. Then set the DD_KSTAT_CHANGING flag to
-	 * prevent any access to the kstats while we delete and recreate
-	 * kstats below.
-	 */
 	mutex_enter(&ddp->dd_mutex);
 	if (ddp->dd_ref > 1) {
 		mutex_exit(&ddp->dd_mutex);
 		err = EBUSY;
 		goto done;
 	}
-
-	ddp->dd_flags |= DD_KSTAT_CHANGING;
-	clear_dd_flag = B_TRUE;
 	mutex_exit(&ddp->dd_mutex);
 
 	if (id2 == DATALINK_INVALID_LINKID) {
@@ -1397,23 +1383,11 @@ dls_devnet_rename(datalink_id_t id1, datalink_id_t id2, const char *link)
 	mutex_exit(&ddp->dd_mutex);
 
 done:
-	/*
-	 * Change the name of the kstat based on the new link name.
-	 * We can't hold the i_dls_devnet_lock across calls to the kstat
-	 * subsystem. Instead the DD_KSTAT_CHANGING flag set above in this
-	 * function prevents any access to the dd_ksp while we delete and
-	 * recreate it below.
-	 */
 	rw_exit(&i_dls_devnet_lock);
+
 	if (err == 0)
 		dls_devnet_stat_rename(ddp);
 
-	if (clear_dd_flag) {
-		mutex_enter(&ddp->dd_mutex);
-		ddp->dd_flags &= ~DD_KSTAT_CHANGING;
-		mutex_exit(&ddp->dd_mutex);
-	}
-
 	if (mph != NULL)
 		mac_perim_exit(mph);
 	softmac_rele_device(ddh);