OS-3506 dls and mac lock ordering isn't honored

Reviewed by: Robert Mustacchi <rm@joyent.com>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Jerry Jelinek <jerry.jelinek@joyent.com>

3 files changed, 53 insertions, 28 deletions

diff --git a/usr/src/uts/common/io/dld/dld_drv.c b/usr/src/uts/common/io/dld/dld_drv.c
index 62bc4a8ecf..b89b623a39 100644
--- a/usr/src/uts/common/io/dld/dld_drv.c
+++ b/usr/src/uts/common/io/dld/dld_drv.c
@@ -344,26 +344,17 @@ drv_ioc_attr(void *karg, intptr_t arg, int mode, cred_t *cred, int *rvalp)
 	    zone_check_datalink(&zoneid, diap->dia_linkid) != 0)
 		return (ENOENT);
 
-	if ((err = dls_devnet_hold_tmp(diap->dia_linkid, &dlh)) != 0)
+	if ((err = mac_perim_enter_by_linkid(diap->dia_linkid, &mph)) != 0)
 		return (err);
 
-	if ((err = mac_perim_enter_by_macname(
-	    dls_devnet_mac(dlh), &mph)) != 0) {
-		dls_devnet_rele_tmp(dlh);
-		return (err);
-	}
-
-	if ((err = dls_link_hold(dls_devnet_mac(dlh), &dlp)) != 0) {
+	if ((err = dls_devnet_hold_link(diap->dia_linkid, &dlh, &dlp)) != 0) {
 		mac_perim_exit(mph);
-		dls_devnet_rele_tmp(dlh);
 		return (err);
 	}
 
 	mac_sdu_get(dlp->dl_mh, NULL, &diap->dia_max_sdu);
-
-	dls_link_rele(dlp);
+	dls_devnet_rele_link(dlh, dlp);
 	mac_perim_exit(mph);
-	dls_devnet_rele_tmp(dlh);
 
 	return (0);
 }
@@ -671,11 +662,10 @@ drv_ioc_prop_common(dld_ioc_macprop_t *prop, intptr_t arg, boolean_t set,
 			goto done;
 	}
 
-	if ((err = dls_devnet_hold_tmp(linkid, &dlh)) != 0)
-		goto done;
-	if ((err = mac_perim_enter_by_macname(dls_devnet_mac(dlh), &mph)) != 0)
+	if ((err = mac_perim_enter_by_linkid(linkid, &mph)) != 0)
 		goto done;
-	if ((err = dls_link_hold(dls_devnet_mac(dlh), &dlp)) != 0)
+
+	if ((err = dls_devnet_hold_link(linkid, &dlh, &dlp)) != 0)
 		goto done;
 
 	/*
@@ -796,8 +786,8 @@ done:
 	if (!set && ddi_copyout(kprop, (void *)arg, dsize, mode) != 0)
 		err = EFAULT;
 
-	if (dlp != NULL)
-		dls_link_rele(dlp);
+	if (dlh != NULL && dlp != NULL)
+		dls_devnet_rele_link(dlh, dlp);
 
 	if (mph != NULL) {
 		int32_t	cpuid;
@@ -814,9 +804,6 @@ done:
 			mac_client_set_intr_cpu(mdip, dlp->dl_mch, cpuid);
 	}
 
-	if (dlh != NULL)
-		dls_devnet_rele_tmp(dlh);
-
 	if (kprop != NULL)
 		kmem_free(kprop, dsize);
 	return (err);
diff --git a/usr/src/uts/common/io/dls/dls_mgmt.c b/usr/src/uts/common/io/dls/dls_mgmt.c
index 105c55c7ce..e014f0d748 100644
--- a/usr/src/uts/common/io/dls/dls_mgmt.c
+++ b/usr/src/uts/common/io/dls/dls_mgmt.c
@@ -1854,14 +1854,50 @@ dls_devnet_destroy(mac_handle_t mh, datalink_id_t *idp, boolean_t wait)
 	mac_perim_handle_t	mph;
 
 	*idp = DATALINK_INVALID_LINKID;
+	mac_perim_enter_by_mh(mh, &mph);
 	err = dls_devnet_unset(mac_name(mh), idp, wait);
-	if (err != 0 && err != ENOENT)
+
+	/*
+	 * We continue on in the face of ENOENT because the devnet
+	 * unset and DLS link release are not atomic and we may have a
+	 * scenario where there is no entry in i_dls_devnet_hash for
+	 * the MAC name but there is an entry in i_dls_link_hash. For
+	 * example, if the following occurred:
+	 *
+	 * 1. dls_devnet_unset() returns success, and
+	 *
+	 * 2. dls_link_rele_by_name() fails with ENOTEMPTY because
+	 *    flows still exist, and
+	 *
+	 * 3. dls_devnet_set() fails to set the zone id and calls
+	 *    dls_devnet_unset() -- leaving an entry in
+	 *    i_dls_link_hash but no corresponding entry in
+	 *    i_dls_devnet_hash.
+	 *
+	 * Even if #3 wasn't true the dls_devnet_set() may fail for
+	 * different reasons in the future; the point is that it _can_
+	 * fail as part of its contract. We can't rely on it working
+	 * so we must assume that these two pieces of state (devnet
+	 * and link hashes), which should always be in sync, can get
+	 * out of sync and thus even if we get ENOENT from the devnet
+	 * hash we should still try to delete from the link hash just
+	 * in case.
+	 *
+	 * We could prevent the ENOTEMPTY from dls_link_rele_by_name()
+	 * by calling mac_disable() before calling
+	 * dls_devnet_destroy() but that's not currently possible due
+	 * to a long-standing bug. OpenSolaris 6791335: The semantics
+	 * of mac_disable() were modified by Crossbow such that
+	 * dls_devnet_destroy() needs to be called before
+	 * mac_disable() can succeed. This is because of the implicit
+	 * reference that dls has on the mac_impl_t.
+	 */
+	if (err != 0 && err != ENOENT) {
+		mac_perim_exit(mph);
 		return (err);
+	}
 
-	mac_perim_enter_by_mh(mh, &mph);
 	err = dls_link_rele_by_name(mac_name(mh));
-	mac_perim_exit(mph);
-
 	if (err != 0) {
 		/*
 		 * XXX It is a general GLDv3 bug that dls_devnet_set() has to
@@ -1873,6 +1909,8 @@ dls_devnet_destroy(mac_handle_t mh, datalink_id_t *idp, boolean_t wait)
 		(void) dls_devnet_set(mac_name(mh), *idp, crgetzoneid(CRED()),
 		    NULL);
 	}
+
+	mac_perim_exit(mph);
 	return (err);
 }
 
diff --git a/usr/src/uts/common/io/mac/mac.c b/usr/src/uts/common/io/mac/mac.c
index 49958bd152..978b4684e0 100644
--- a/usr/src/uts/common/io/mac/mac.c
+++ b/usr/src/uts/common/io/mac/mac.c
@@ -158,7 +158,7 @@
  * perimeter) across a call to any other layer from the mac layer. The call to
  * any other layer could be via mi_* entry points, classifier entry points into
  * the driver or via upcall pointers into layers above. The mac perimeter may
- * be acquired or held only in the down direction, for e.g. when calling into
+ * be acquired or held only in the down direction, e.g. when calling into
  * a mi_* driver enty point to provide atomicity of the operation.
  *
  * R8. Since it is not guaranteed (see R14) that drivers won't hold locks across
@@ -207,7 +207,7 @@
  * number whenever the ring's stop routine is invoked.
  * See comments in mac_rx_ring();
  *
- * R17 Similarly mi_stop is another synchronization point and the driver must
+ * R17. Similarly mi_stop is another synchronization point and the driver must
  * ensure that all upcalls are done and there won't be any future upcall
  * before returning from mi_stop.
  *
@@ -227,7 +227,7 @@
  *
  * cpu_lock -> mac_srs_g_lock -> srs_lock -> s_ring_lock [mac_walk_srs_and_bind]
  *
- * i_dls_devnet_lock -> mac layer locks [dls_devnet_rename]
+ * mac perim -> i_dls_devnet_lock [dls_devnet_rename]
  *
  * Perimeters are ordered P1 -> P2 -> P3 from top to bottom in order of mac
  * client to driver. In the case of clients that explictly use the mac provided