diff --git a/sys/dev/disk/dm/dm_dev.c b/sys/dev/disk/dm/dm_dev.c --- a/sys/dev/disk/dm/dm_dev.c +++ b/sys/dev/disk/dm/dm_dev.c @@ -300,6 +300,10 @@ /* * dm_dev_remove is called to completely destroy & remove a dm disk device. + * + * NOTE: the caller MUST NOT hold a busy reference on @dmv. disable_dev() + * waits for ref_cnt to drop to zero, so any caller-held reference would + * deadlock it. */ int dm_dev_remove(dm_dev_t *dmv) @@ -313,6 +317,49 @@ dm_dev_destroy(dmv); return 0; +} + +/* + * Atomically look a device up by name/uuid/minor, verify it is not open, + * remove it from the global list, wait for in-flight references to drain, + * and destroy it. + * + * The lookup, the is_open check and the removal from dm_dev_list all happen + * under dm_dev_mutex, so two concurrent removers are serialized: the first + * one removes and frees the device, the second one finds nothing (ENOENT). + * This closes the use-after-free window that existed when dm_dev_remove_ioctl + * dropped its busy reference (dm_dev_unbusy) between the is_open check and the + * call to dm_dev_remove(): in that window a concurrent remover could free the + * very same dm_dev_t out from under the first remover. + * + * Unlike dm_dev_remove(), this helper performs the lookup itself and never + * takes a long-lived busy reference, so disable_dev()'s wait-for-refcnt-zero + * can complete. + */ +int +dm_dev_destroy_by_key(const char *name, const char *uuid, int minor) +{ + dm_dev_t *dmv; + + lockmgr(&dm_dev_mutex, LK_EXCLUSIVE); + + if ((dmv = _dm_dev_lookup(name, uuid, minor)) == NULL) { + lockmgr(&dm_dev_mutex, LK_RELEASE); + return ENOENT; + } + + if (dmv->is_open) { + lockmgr(&dm_dev_mutex, LK_RELEASE); + return EBUSY; + } + + /* disable_dev() removes dmv from dm_dev_list and waits for ref_cnt==0. */ + disable_dev(dmv); + lockmgr(&dm_dev_mutex, LK_RELEASE); + + dm_dev_destroy(dmv); + + return 0; } int diff --git a/sys/dev/disk/dm/dm.h b/sys/dev/disk/dm/dm.h --- a/sys/dev/disk/dm/dm.h +++ b/sys/dev/disk/dm/dm.h @@ -238,6 +238,7 @@ int dm_dev_create(dm_dev_t **, const char *, const char *, int); int dm_dev_remove(dm_dev_t *); int dm_dev_remove_all(int); +int dm_dev_destroy_by_key(const char *, const char *, int); void dm_dev_busy(dm_dev_t *); void dm_dev_unbusy(dm_dev_t *); prop_array_t dm_dev_prop_list(void); diff --git a/sys/dev/disk/dm/dm_ioctl.c b/sys/dev/disk/dm/dm_ioctl.c --- a/sys/dev/disk/dm/dm_ioctl.c +++ b/sys/dev/disk/dm/dm_ioctl.c @@ -330,9 +330,9 @@ int dm_dev_remove_ioctl(prop_dictionary_t dm_dict) { - dm_dev_t *dmv; const char *name, *uuid; - uint32_t flags, minor, is_open; + uint32_t flags, minor; + int error; flags = 0; name = NULL; @@ -346,19 +346,20 @@ dm_dbg_print_flags(flags); - if ((dmv = dm_dev_lookup(name, uuid, minor)) == NULL) { + /* + * Atomically look the device up, check that it is not open, remove it + * from the global list, drain in-flight references and destroy it -- all + * under dm_dev_mutex inside dm_dev_destroy_by_key(). We must NOT take a + * busy reference and drop it before the destroy: doing so opened a window + * (dm_dev_unbusy here, then dm_dev_remove(dmv)) in which a concurrent + * remover could free the same dm_dev_t out from under us -- a + * use-after-free (DF-2447). + */ + error = dm_dev_destroy_by_key(name, uuid, minor); + if (error == ENOENT) dm_remove_flag(dm_dict, &flags, DM_EXISTS_FLAG); - return ENOENT; - } - is_open = dmv->is_open; - - dm_dev_unbusy(dmv); - - if (is_open) - return EBUSY; - - return dm_dev_remove(dmv); + return error; } /* @@ -515,11 +516,16 @@ prop_dictionary_set_uint32(dm_dict, DM_IOCTL_FLAGS, flags); prop_dictionary_set_uint32(dm_dict, DM_IOCTL_MINOR, dmv->minor); - dm_dev_unbusy(dmv); - - /* Destroy inactive table after resume. */ + /* + * Destroy the inactive table BEFORE dropping our busy reference. + * dm_table_destroy() dereferences dmv->table_head; if we unbusied first + * (dropping the last reference), a concurrent remover could free dmv out + * from under us -- a use-after-free (DF-2447). + */ dm_table_destroy(&dmv->table_head, DM_TABLE_INACTIVE); + dm_dev_unbusy(dmv); + return 0; }