⬢ DragonFlyBSD Kernel Audit
DF-4472 / fix.diff
← back to finding ↓ download raw
--- a/sys/dev/disk/dm/dm_ioctl.c
+++ b/sys/dev/disk/dm/dm_ioctl.c
 	if ((dmv = dm_dev_lookup(name, uuid, minor)) == NULL) {
 		dm_remove_flag(dm_dict, &flags, DM_EXISTS_FLAG);
+		prop_object_release(cmd_array);
 		return ENOENT;
 	}
 	prop_dictionary_set_uint32(dm_dict, DM_IOCTL_MINOR, dmv->minor);
@@ -693,6 +694,7 @@
 	uuid = NULL;
 	dmv = NULL;
 	str = NULL;
+	type = NULL;
 
 	/*
 	 * char *xml; xml = prop_dictionary_externalize(dm_dict);
@@ -710,12 +712,23 @@
 
 	if ((dmv = dm_dev_lookup(name, uuid, minor)) == NULL) {
 		dm_remove_flag(dm_dict, &flags, DM_EXISTS_FLAG);
+		if (iter != NULL)
+			prop_object_iterator_release(iter);
 		return ENOENT;
 	}
 	dmdebug("Loading table to device: %s--%d\n", name,
 	    dmv->table_head.cur_active_table);
 
 	/*
+	 * Serialize concurrent table loads on the same device: the table
+	 * lock is only acquired SHARED by dm_table_get_entry(), which does
+	 * not exclude other loads mutating the same inactive table TAILQ.
+	 * dev_mtx is recursive (LK_CANRECURSE) and only held briefly by the
+	 * busy/unbusy paths, so holding it across the load is safe.
+	 */
+	lockmgr(&dmv->dev_mtx, LK_EXCLUSIVE);
+
+	/*
 	 * I have to check if this table slot is not used by another table list.
 	 * if it is used I should free them.
 	 */
@@ -728,8 +741,16 @@
 	prop_dictionary_set_uint32(dm_dict, DM_IOCTL_MINOR, dmv->minor);
 
 	while ((target_dict = prop_object_iterator_next(iter)) != NULL) {
-		prop_dictionary_get_cstring_nocopy(target_dict,
-		    DM_TABLE_TYPE, &type);
+		type = NULL;
+		if (!prop_dictionary_get_cstring_nocopy(target_dict,
+		    DM_TABLE_TYPE, &type) || type == NULL) {
+			dm_table_release(&dmv->table_head, DM_TABLE_INACTIVE);
+			dm_table_destroy(&dmv->table_head, DM_TABLE_INACTIVE);
+			prop_object_iterator_release(iter);
+			lockmgr(&dmv->dev_mtx, LK_RELEASE);
+			dm_dev_unbusy(dmv);
+			return EINVAL;
+		}
 		/*
 		 * If we want to deny table with 2 or more different
 		 * target we should do it here
@@ -737,12 +758,16 @@
 		if (((target = dm_target_lookup(type)) == NULL) &&
 		    ((target = dm_target_autoload(type)) == NULL)) {
 			dm_table_release(&dmv->table_head, DM_TABLE_INACTIVE);
+			prop_object_iterator_release(iter);
+			lockmgr(&dmv->dev_mtx, LK_RELEASE);
 			dm_dev_unbusy(dmv);
 			return ENOENT;
 		}
 		if ((table_en = kmalloc(sizeof(dm_table_entry_t),
 			    M_DM, M_WAITOK)) == NULL) {
 			dm_table_release(&dmv->table_head, DM_TABLE_INACTIVE);
+			prop_object_iterator_release(iter);
+			lockmgr(&dmv->dev_mtx, LK_RELEASE);
 			dm_dev_unbusy(dmv);
 			dm_target_unbusy(target);
 			return ENOMEM;
@@ -771,8 +796,6 @@
 		prop_dictionary_get_cstring(target_dict,
 		    DM_TABLE_PARAMS, &str);
 
-		TAILQ_INSERT_TAIL(tbl, table_en, next);
-
 		/*
 		 * Params string is different for every target,
 		 * therfore I have to pass it to target init
@@ -781,14 +804,33 @@
 		dmdebug("String passed in is: \"%s\"\n", str);
 
 		if ((ret = dm_table_init(target, table_en, str)) != 0) {
+			/*
+			 * Entry is not linked into the table yet, so clean
+			 * it up directly instead of relying on
+			 * dm_table_destroy() finding it in the list.
+			 */
+			if (table_en->target->destroy)
+				table_en->target->destroy(table_en);
+			dm_table_free_deps(table_en);
+			dm_target_unbusy(target);
+			kfree(table_en, M_DM);
 			dm_table_release(&dmv->table_head, DM_TABLE_INACTIVE);
 			dm_table_destroy(&dmv->table_head, DM_TABLE_INACTIVE);
 			kfree(str, M_TEMP);
-
+			prop_object_iterator_release(iter);
+			lockmgr(&dmv->dev_mtx, LK_RELEASE);
 			dm_dev_unbusy(dmv);
 			return ret;
 		}
 		kfree(str, M_TEMP);
+
+		/*
+		 * Publish the fully initialized entry.  Holding dev_mtx
+		 * keeps concurrent loads from racing TAILQ_INSERT_TAIL, and
+		 * the SHARED table lock keeps list walkers away while we
+		 * link the entry in.
+		 */
+		TAILQ_INSERT_TAIL(tbl, table_en, next);
 	}
 	prop_object_iterator_release(iter);
 
@@ -797,6 +839,7 @@
 
 	dm_table_release(&dmv->table_head, DM_TABLE_INACTIVE);
 
+	lockmgr(&dmv->dev_mtx, LK_RELEASE);
 	dm_dev_unbusy(dmv);
 #if 0
 	dmsetdiskinfo(dmv->diskp, &dmv->table_head);
@@ -896,8 +939,10 @@
 
 	if ((dmv = dm_dev_lookup(name, uuid, minor)) == NULL) {
 		dm_remove_flag(dm_dict, &flags, DM_EXISTS_FLAG);
+		prop_object_release(cmd_array);
 		return ENOENT;
 	}
+
 	/*
 	 * if DM_QUERY_INACTIVE_TABLE_FLAG is passed we need to query
 	 * INACTIVE TABLE