Chonyun Wu's latest patch has shown that the handling of the daemon
state variable running_state is racy and difficult to get right. It's
not a good candidate for a "benign race" annotation. So, as a first
step to sanitizing it, make sure all accesses to the state variable
are protected by config_lock.
The patch also replaces "if" with "while" in several places where the
code was supposed to wait until a certain state was reached. It's
important that DAEMON_SHUTDOWN terminates all loops of this kind.
Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
---
multipathd/main.c | 79 ++++++++++++++++++++++++++++-------------------
1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 6a5d105a..6fc6a3ac 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -126,11 +126,21 @@ int poll_dmevents = 0;
#else
int poll_dmevents = 1;
#endif
-enum daemon_status running_state = DAEMON_INIT;
+enum daemon_status _running_state = DAEMON_INIT;
pid_t daemon_pid;
pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t config_cond;
+static inline enum daemon_status get_running_state(void)
+{
+ enum daemon_status st;
+
+ pthread_mutex_lock(&config_lock);
+ st = _running_state;
+ pthread_mutex_unlock(&config_lock);
+ return st;
+}
+
/*
* global copy of vecs for use in sig handlers
*/
@@ -148,7 +158,7 @@ static volatile sig_atomic_t log_reset_sig;
const char *
daemon_status(void)
{
- switch (running_state) {
+ switch (get_running_state()) {
case DAEMON_INIT:
return "init";
case DAEMON_START:
@@ -168,10 +178,10 @@ daemon_status(void)
/*
* I love you too, systemd ...
*/
-const char *
-sd_notify_status(void)
+static const char *
+sd_notify_status(enum daemon_status state)
{
- switch (running_state) {
+ switch (state) {
case DAEMON_INIT:
return "STATUS=init";
case DAEMON_START:
@@ -188,17 +198,18 @@ sd_notify_status(void)
}
#ifdef USE_SYSTEMD
-static void do_sd_notify(enum daemon_status old_state)
+static void do_sd_notify(enum daemon_status old_state,
+ enum daemon_status new_state)
{
/*
* Checkerloop switches back and forth between idle and running state.
* No need to tell systemd each time.
* These notifications cause a lot of overhead on dbus.
*/
- if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
+ if ((new_state == DAEMON_IDLE || new_state == DAEMON_RUNNING) &&
(old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
return;
- sd_notify(0, sd_notify_status());
+ sd_notify(0, sd_notify_status(new_state));
}
#endif
@@ -207,15 +218,16 @@ static void config_cleanup(void *arg)
pthread_mutex_unlock(&config_lock);
}
+/* must be called with config_lock held */
static void __post_config_state(enum daemon_status state)
{
- if (state != running_state && running_state != DAEMON_SHUTDOWN) {
- enum daemon_status old_state = running_state;
+ if (state != _running_state && _running_state != DAEMON_SHUTDOWN) {
+ enum daemon_status old_state = _running_state;
- running_state = state;
+ _running_state = state;
pthread_cond_broadcast(&config_cond);
#ifdef USE_SYSTEMD
- do_sd_notify(old_state);
+ do_sd_notify(old_state, state);
#endif
}
}
@@ -234,12 +246,12 @@ int set_config_state(enum daemon_status state)
pthread_cleanup_push(config_cleanup, NULL);
pthread_mutex_lock(&config_lock);
- if (running_state != state) {
- enum daemon_status old_state = running_state;
+ if (_running_state != state) {
+ enum daemon_status old_state = _running_state;
- if (running_state == DAEMON_SHUTDOWN)
+ if (_running_state == DAEMON_SHUTDOWN)
rc = EINVAL;
- else if (running_state != DAEMON_IDLE) {
+ else if (_running_state != DAEMON_IDLE) {
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
@@ -247,11 +259,11 @@ int set_config_state(enum daemon_status state)
rc = pthread_cond_timedwait(&config_cond,
&config_lock, &ts);
}
- if (!rc && (running_state != DAEMON_SHUTDOWN)) {
- running_state = state;
+ if (!rc && (_running_state != DAEMON_SHUTDOWN)) {
+ _running_state = state;
pthread_cond_broadcast(&config_cond);
#ifdef USE_SYSTEMD
- do_sd_notify(old_state);
+ do_sd_notify(old_state, state);
#endif
}
}
@@ -1405,17 +1417,20 @@ uev_trigger (struct uevent * uev, void * trigger_data)
int r = 0;
struct vectors * vecs;
struct uevent *merge_uev, *tmp;
+ enum daemon_status state;
vecs = (struct vectors *)trigger_data;
pthread_cleanup_push(config_cleanup, NULL);
pthread_mutex_lock(&config_lock);
- if (running_state != DAEMON_IDLE &&
- running_state != DAEMON_RUNNING)
+ while (_running_state != DAEMON_IDLE &&
+ _running_state != DAEMON_RUNNING &&
+ _running_state != DAEMON_SHUTDOWN)
pthread_cond_wait(&config_cond, &config_lock);
+ state = _running_state;
pthread_cleanup_pop(1);
- if (running_state == DAEMON_SHUTDOWN)
+ if (state == DAEMON_SHUTDOWN)
return 0;
/*
@@ -2661,6 +2676,7 @@ child (void * param)
struct config *conf;
char *envp;
int queue_without_daemon;
+ enum daemon_status state;
mlockall(MCL_CURRENT | MCL_FUTURE);
signal_init();
@@ -2756,8 +2772,9 @@ child (void * param)
rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
if (!rc) {
/* Wait for uxlsnr startup */
- while (running_state == DAEMON_IDLE)
+ while (_running_state == DAEMON_IDLE)
pthread_cond_wait(&config_cond, &config_lock);
+ state = _running_state;
}
pthread_cleanup_pop(1);
@@ -2765,7 +2782,7 @@ child (void * param)
condlog(0, "failed to create cli listener: %d", rc);
goto failed;
}
- else if (running_state != DAEMON_CONFIGURE) {
+ else if (state != DAEMON_CONFIGURE) {
condlog(0, "cli listener failed to start");
goto failed;
}
@@ -2805,15 +2822,17 @@ child (void * param)
}
pthread_attr_destroy(&misc_attr);
- while (running_state != DAEMON_SHUTDOWN) {
+ while (1) {
pthread_cleanup_push(config_cleanup, NULL);
pthread_mutex_lock(&config_lock);
- if (running_state != DAEMON_CONFIGURE &&
- running_state != DAEMON_SHUTDOWN) {
+ while (_running_state != DAEMON_CONFIGURE &&
+ _running_state != DAEMON_SHUTDOWN)
pthread_cond_wait(&config_cond, &config_lock);
- }
+ state = _running_state;
pthread_cleanup_pop(1);
- if (running_state == DAEMON_CONFIGURE) {
+ if (state == DAEMON_SHUTDOWN)
+ break;
+ if (state == DAEMON_CONFIGURE) {
pthread_cleanup_push(cleanup_lock, &vecs->lock);
lock(&vecs->lock);
pthread_testcancel();
@@ -2983,8 +3002,6 @@ main (int argc, char *argv[])
ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
"Manipulated through RCU");
- ANNOTATE_BENIGN_RACE_SIZED(&running_state, sizeof(running_state),
- "Suppress complaints about unprotected running_state reads");
ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
"Suppress complaints about this scalar variable");
--
2.19.2
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
