DF-4474 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 | --- 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 |