⬢ DragonFlyBSD Kernel Audit
DF-5037 / fix.diff
← back to finding ↓ download raw
--- a/sys/dev/disk/dm/dm_pdev.c
+++ b/sys/dev/disk/dm/dm_pdev.c
@@ -115,11 +115,24 @@
  * Create entry for device with name dev_name and open vnode for it.
  * If entry already exists in global TAILQ I will only increment
  * reference counter.
+ *
+ * NOTE: vn_open()/VOP_GETATTR()/dev_dioctl() below are blocking VFS
+ * operations that can re-enter the device-mapper subsystem (dm-on-dm
+ * stacking: vn_open of /dev/mapper/X runs dmopen() -> dm_dev_lookup()
+ * which takes dm_dev_mutex).  They MUST NOT run while dm_pdev_mutex is
+ * held: dm_dev.c holds dm_dev_mutex across reference-drain waits and
+ * table destruction (-> target destroy -> dm_pdev_decr ->
+ * dm_pdev_mutex), so holding dm_pdev_mutex across the vnode open
+ * creates an AB-BA lock cycle that permanently deadlocks the whole dm
+ * subsystem.  dm_pdev_mutex is therefore only held for the list
+ * lookup/insertion; a concurrent insert of the same name is resolved
+ * by re-checking the list under the lock after the vnode was opened.
  */
 dm_pdev_t *
 dm_pdev_insert(const char *dev_name)
 {
 	dm_pdev_t *dmp;
+	dm_pdev_t *dmp2;
 	struct vattr va;
 	int error;
 
@@ -134,18 +147,18 @@
 		lockmgr(&dm_pdev_mutex, LK_RELEASE);
 		return dmp;
 	}
+	lockmgr(&dm_pdev_mutex, LK_RELEASE);
 
 	if ((dmp = dm_pdev_alloc(dev_name)) == NULL) {
-		lockmgr(&dm_pdev_mutex, LK_RELEASE);
 		return NULL;
 	}
 
+	/* Open the backing device WITHOUT holding dm_pdev_mutex. */
 	error = dm_dk_lookup(dev_name, &dmp->pdev_vnode);
 	if (error) {
 		dmdebug("Lookup on %s failed with error %d!\n",
 		    dev_name, error);
 		dm_pdev_free(dmp);
-		lockmgr(&dm_pdev_mutex, LK_RELEASE);
 		return NULL;
 	}
 	dmp->ref_cnt = 1;
@@ -153,7 +166,6 @@
 	if (dm_pdev_get_vattr(dmp, &va) == -1) {
 		dmdebug("getattr on %s failed\n", dev_name);
 		dm_pdev_free(dmp);
-		lockmgr(&dm_pdev_mutex, LK_RELEASE);
 		return NULL;
 	}
 	ksnprintf(dmp->udev_name, sizeof(dmp->udev_name),
@@ -178,6 +190,19 @@
 		kprintf("dmp_pdev_insert DIOCGPART failed %d\n", error);
 	}
 
+	/*
+	 * Re-take the lock and insert, unless another thread created a
+	 * pdev with the same name while we had the lock dropped.
+	 */
+	lockmgr(&dm_pdev_mutex, LK_EXCLUSIVE);
+	dmp2 = dm_pdev_lookup_name(dev_name);
+	if (dmp2 != NULL) {
+		/* Lost the race: bump the existing entry and drop ours. */
+		dmp2->ref_cnt++;
+		lockmgr(&dm_pdev_mutex, LK_RELEASE);
+		dm_pdev_free(dmp);
+		return dmp2;
+	}
 	TAILQ_INSERT_TAIL(&dm_pdev_list, dmp, next_pdev);
 	lockmgr(&dm_pdev_mutex, LK_RELEASE);
 
--- a/sys/dev/disk/dm/dm_dev.c
+++ b/sys/dev/disk/dm/dm_dev.c
@@ -61,6 +61,14 @@
 
 /*
  * dm_dev_mutex must be held by caller before using disable_dev.
+ *
+ * NOTE: disable_dev only unlinks the device from the global list now.
+ * It must NOT wait for ref_cnt to drain (or destroy the device) while
+ * dm_dev_mutex is held: a thread holding a dm_dev busy reference may
+ * need dm_dev_mutex itself before it can unbusy (dm-on-dm stacking:
+ * dmopen/dmclose -> dm_dev_lookup inside a table load/destroy), so
+ * sleeping on the reference count with dm_dev_mutex held deadlocks the
+ * whole subsystem.
  */
 static void
 disable_dev(dm_dev_t *dmv)
@@ -69,7 +77,15 @@
 
 	TAILQ_REMOVE(&dm_dev_list, dmv, next_devlist);
 	dm_dev_counter--;
+}
 
+/*
+ * Wait for all busy references to drain.  Must be called WITHOUT any
+ * dm locks held (see the note above disable_dev).
+ */
+static void
+dm_dev_wait_refs(dm_dev_t *dmv)
+{
 	lockmgr(&dmv->dev_mtx, LK_EXCLUSIVE);
 	while (dmv->ref_cnt != 0)
 		cv_wait(&dmv->dev_cv, &dmv->dev_mtx);
@@ -304,11 +320,14 @@
 int
 dm_dev_remove(dm_dev_t *dmv)
 {
-	/* Remove device from list and wait for refcnt to drop to zero */
+	/* Remove device from list and wait for refcnt to drop to zero,
+	 * then destroy -- all without holding dm_dev_mutex. */
 	lockmgr(&dm_dev_mutex, LK_EXCLUSIVE);
 	disable_dev(dmv);
 	lockmgr(&dm_dev_mutex, LK_RELEASE);
 
+	dm_dev_wait_refs(dmv);
+
 	/* Destroy and free the device */
 	dm_dev_destroy(dmv);