From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Martin Wilck Date: Tue, 15 Sep 2020 23:38:48 +0200 Subject: [PATCH] libmultipath: protect racy libdevmapper calls with a mutex dm_udev_wait() and dm_task_run() may access global / static state in libdevmapper. They need to be protected by a lock in in our multithreaded library. The modified call sequence requires fixing the dmevents test: devmapper.c must be added to dmevents-test_OBJDEPS to catch calls to dm_task_run(). Also, the call to dmevent_poll_supported() in setup() will cause init_versions() to be called, which requires bypassing the wrappers in the test setup phase. Cc: lixiaokeng@huawei.com Reviewed-by: Benjamin Marzinski Signed-off-by: Benjamin Marzinski --- libmultipath/devmapper.c | 73 +++++++++++++++++++------------ libmultipath/devmapper.h | 2 + libmultipath/libmultipath.version | 6 +++ libmultipath/util.c | 5 +++ libmultipath/util.h | 1 + multipathd/dmevents.c | 2 +- multipathd/waiter.c | 2 +- tests/Makefile | 1 + tests/dmevents.c | 12 +++++ 9 files changed, 75 insertions(+), 29 deletions(-) diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 3e36129b..4eb6f539 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -42,6 +42,7 @@ static unsigned int dm_mpath_target_version[3] = { INVALID_VERSION, }; static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT; static pthread_once_t versions_initialized = PTHREAD_ONCE_INIT; +static pthread_mutex_t libmp_dm_lock = PTHREAD_MUTEX_INITIALIZER; static int dm_conf_verbosity; @@ -59,16 +60,34 @@ static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a) return 1; } -void dm_udev_wait(unsigned int c) +static void libmp_udev_wait(unsigned int c) { } -void dm_udev_set_sync_support(int c) +static void dm_udev_set_sync_support(int c) { } - +#else +static void libmp_udev_wait(unsigned int c) +{ + pthread_mutex_lock(&libmp_dm_lock); + pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock); + dm_udev_wait(c); + pthread_cleanup_pop(1); +} #endif +int libmp_dm_task_run(struct dm_task *dmt) +{ + int r; + + pthread_mutex_lock(&libmp_dm_lock); + pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock); + r = dm_task_run(dmt); + pthread_cleanup_pop(1); + return r; +} + __attribute__((format(printf, 4, 5))) static void dm_write_log (int level, const char *file, int line, const char *f, ...) { @@ -198,7 +217,7 @@ static int dm_tgt_version (unsigned int *version, char *str) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt); condlog(0, "Can not communicate with kernel DM"); goto out; @@ -382,12 +401,12 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags)) goto out; - r = dm_task_run (dmt); + r = libmp_dm_task_run (dmt); if (!r) dm_log_error(2, task, dmt); if (udev_wait_flag) - dm_udev_wait(cookie); + libmp_udev_wait(cookie); out: dm_task_destroy (dmt); return r; @@ -474,12 +493,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp, !dm_task_set_cookie(dmt, &cookie, udev_flags)) goto freeout; - r = dm_task_run (dmt); + r = libmp_dm_task_run (dmt); if (!r) dm_log_error(2, task, dmt); if (task == DM_DEVICE_CREATE) - dm_udev_wait(cookie); + libmp_udev_wait(cookie); freeout: if (prefixed_uuid) FREE(prefixed_uuid); @@ -589,7 +608,7 @@ do_get_info(const char *name, struct dm_info *info) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_INFO, dmt); goto out; } @@ -630,7 +649,7 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams) dm_task_no_open_count(dmt); errno = 0; - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_TABLE, dmt); if (dm_task_get_errno(dmt) == ENXIO) r = DMP_NOT_FOUND; @@ -672,7 +691,7 @@ dm_get_prefixed_uuid(const char *name, char *uuid, int uuid_len) if (!dm_task_set_name (dmt, name)) goto uuidout; - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_INFO, dmt); goto uuidout; } @@ -743,7 +762,7 @@ int dm_get_status(const char *name, char *outstatus) dm_task_no_open_count(dmt); errno = 0; - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_STATUS, dmt); if (dm_task_get_errno(dmt) == ENXIO) r = DMP_NOT_FOUND; @@ -796,7 +815,7 @@ int dm_type(const char *name, char *type) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_TABLE, dmt); goto out; } @@ -840,7 +859,7 @@ int dm_is_mpath(const char *name) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_TABLE, dmt); goto out_task; } @@ -904,7 +923,7 @@ dm_map_present_by_uuid(const char *uuid) if (!dm_task_set_uuid(dmt, prefixed_uuid)) goto out_task; - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_INFO, dmt); goto out_task; } @@ -950,7 +969,7 @@ dm_get_opencount (const char * mapname) if (!dm_task_set_name(dmt, mapname)) goto out; - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_INFO, dmt); goto out; } @@ -1110,7 +1129,7 @@ int dm_flush_maps (int need_suspend, int retries) dm_task_no_open_count(dmt); - if (!dm_task_run (dmt)) { + if (!libmp_dm_task_run (dmt)) { dm_log_error(3, DM_DEVICE_LIST, dmt); goto out; } @@ -1156,7 +1175,7 @@ dm_message(const char * mapname, char * message) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(2, DM_DEVICE_TARGET_MSG, dmt); goto out; } @@ -1276,7 +1295,7 @@ dm_get_maps (vector mp) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_LIST, dmt); goto out; } @@ -1361,7 +1380,7 @@ dm_mapname(int major, int minor) * daemon uev_trigger -> uev_add_map */ while (--loop) { - r = dm_task_run(dmt); + r = libmp_dm_task_run(dmt); if (r) break; @@ -1406,7 +1425,7 @@ do_foreach_partmaps (const char * mapname, dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_LIST, dmt); goto out; } @@ -1641,11 +1660,11 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx) if (!dm_task_set_cookie(dmt, &cookie, udev_flags)) goto out; - r = dm_task_run(dmt); + r = libmp_dm_task_run(dmt); if (!r) dm_log_error(2, DM_DEVICE_RENAME, dmt); - dm_udev_wait(cookie); + libmp_udev_wait(cookie); out: dm_task_destroy(dmt); @@ -1687,7 +1706,7 @@ int dm_reassign_table(const char *name, char *old, char *new) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_TABLE, dmt); goto out; } @@ -1720,7 +1739,7 @@ int dm_reassign_table(const char *name, char *old, char *new) if (modified) { dm_task_no_open_count(reload_dmt); - if (!dm_task_run(reload_dmt)) { + if (!libmp_dm_task_run(reload_dmt)) { dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt); condlog(3, "%s: failed to reassign targets", name); goto out_reload; @@ -1767,7 +1786,7 @@ int dm_reassign(const char *mapname) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_DEPS, dmt); goto out; } @@ -1835,7 +1854,7 @@ int dm_setgeometry(struct multipath *mpp) goto out; } - r = dm_task_run(dmt); + r = libmp_dm_task_run(dmt); if (!r) dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt); out: diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h index a0bcd137..fa6b3c53 100644 --- a/libmultipath/devmapper.h +++ b/libmultipath/devmapper.h @@ -93,6 +93,8 @@ enum { MULTIPATH_VERSION }; int libmp_get_version(int which, unsigned int version[3]); +struct dm_task; +int libmp_dm_task_run(struct dm_task *dmt); #define dm_log_error(lvl, cmd, dmt) \ condlog(lvl, "%s: libdm task=%d error: %s", __func__, \ diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version index ab5bb0ad..97acdbb2 100644 --- a/libmultipath/libmultipath.version +++ b/libmultipath/libmultipath.version @@ -245,3 +245,9 @@ global: local: *; }; + +LIBMULTIPATH_2.1.0 { +global: + libmp_dm_task_run; + cleanup_mutex; +} LIBMULTIPATH_2.0.0; diff --git a/libmultipath/util.c b/libmultipath/util.c index 1f977792..0e37f3ff 100644 --- a/libmultipath/util.c +++ b/libmultipath/util.c @@ -424,6 +424,11 @@ void cleanup_free_ptr(void *arg) free(*p); } +void cleanup_mutex(void *arg) +{ + pthread_mutex_unlock(arg); +} + struct bitfield *alloc_bitfield(unsigned int maxbit) { unsigned int n; diff --git a/libmultipath/util.h b/libmultipath/util.h index ac19473e..e9b48f9f 100644 --- a/libmultipath/util.h +++ b/libmultipath/util.h @@ -49,6 +49,7 @@ int should_exit(void); void close_fd(void *arg); void cleanup_free_ptr(void *arg); +void cleanup_mutex(void *arg); struct scandir_result { struct dirent **di; diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c index fc97c8a2..b561cbfd 100644 --- a/multipathd/dmevents.c +++ b/multipathd/dmevents.c @@ -156,7 +156,7 @@ static int dm_get_events(void) dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) { + if (!libmp_dm_task_run(dmt)) { dm_log_error(3, DM_DEVICE_LIST, dmt); goto fail; } diff --git a/multipathd/waiter.c b/multipathd/waiter.c index 3bc69807..bbe6c2a1 100644 --- a/multipathd/waiter.c +++ b/multipathd/waiter.c @@ -118,7 +118,7 @@ static int waiteventloop (struct event_thread *waiter) pthread_sigmask(SIG_UNBLOCK, &set, &oldset); pthread_testcancel(); - r = dm_task_run(waiter->dmt); + r = libmp_dm_task_run(waiter->dmt); if (!r) dm_log_error(2, DM_DEVICE_WAITEVENT, waiter->dmt); pthread_testcancel(); diff --git a/tests/Makefile b/tests/Makefile index 78777bec..908407ea 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -40,6 +40,7 @@ endif # linker input file). # XYZ-test_LIBDEPS: Additional libs to link for this test +dmevents-test_OBJDEPS = ../libmultipath/devmapper.o dmevents-test_LIBDEPS = -lpthread -ldevmapper -lurcu hwtable-test_TESTDEPS := test-lib.o hwtable-test_OBJDEPS := ../libmultipath/discovery.o ../libmultipath/blacklist.o \ diff --git a/tests/dmevents.c b/tests/dmevents.c index bee117ac..b7c5122b 100644 --- a/tests/dmevents.c +++ b/tests/dmevents.c @@ -179,6 +179,8 @@ struct dm_names *build_dm_names(void) return names; } +static bool setup_done; + static int setup(void **state) { if (dmevent_poll_supported()) { @@ -186,6 +188,7 @@ static int setup(void **state) *state = &data; } else *state = NULL; + setup_done = true; return 0; } @@ -262,14 +265,20 @@ struct dm_task *__wrap_libmp_dm_task_create(int task) return mock_type(struct dm_task *); } +int __real_dm_task_no_open_count(struct dm_task *dmt); int __wrap_dm_task_no_open_count(struct dm_task *dmt) { + if (!setup_done) + return __real_dm_task_no_open_count(dmt); assert_ptr_equal((struct test_data *)dmt, &data); return mock_type(int); } +int __real_dm_task_run(struct dm_task *dmt); int __wrap_dm_task_run(struct dm_task *dmt) { + if (!setup_done) + return __real_dm_task_run(dmt); assert_ptr_equal((struct test_data *)dmt, &data); return mock_type(int); } @@ -291,8 +300,11 @@ struct dm_names * __wrap_dm_task_get_names(struct dm_task *dmt) return data.names; } +void __real_dm_task_destroy(struct dm_task *dmt); void __wrap_dm_task_destroy(struct dm_task *dmt) { + if (!setup_done) + return __real_dm_task_destroy(dmt); assert_ptr_equal((struct test_data *)dmt, &data); if (data.names) {