--- 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