[dm-devel] [PATCH v4 3/6] multipathd: fix ev_remove_path return code handling
Benjamin Marzinski
bmarzins at redhat.com
Mon May 17 16:29:56 UTC 2021
When ev_remove_path() returned success, callers assumed that the path
(and possibly the map) had been removed. When ev_remove_path() returned
failure, callers assumed that the path had not been removed. However,
the path could be removed on both success or failure. This could cause
callers to dereference the path after it was removed.
To deal with this, make ev_remove_path() return a different symbolic
value for each outcome, and make the callers react appropriately for
the different values. Found by coverity.
Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
---
multipathd/cli_handlers.c | 24 +++++++++++++++++++++--
multipathd/main.c | 41 ++++++++++++++++++++-------------------
multipathd/main.h | 14 +++++++++++++
3 files changed, 57 insertions(+), 22 deletions(-)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 1de6ad8e..6765fcf0 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -752,7 +752,8 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
/* Have the checker reinstate this path asap */
pp->tick = 1;
return 0;
- } else if (!ev_remove_path(pp, vecs, true))
+ } else if (ev_remove_path(pp, vecs, true) &
+ REMOVE_PATH_SUCCESS)
/* Path removed in ev_remove_path() */
pp = NULL;
else {
@@ -813,6 +814,7 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
struct vectors * vecs = (struct vectors *)data;
char * param = get_keyparam(v, PATH);
struct path *pp;
+ int ret;
param = convert_dev(param, 1);
condlog(2, "%s: remove path (operator)", param);
@@ -821,7 +823,25 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
condlog(0, "%s: path already removed", param);
return 1;
}
- return ev_remove_path(pp, vecs, 1);
+ ret = ev_remove_path(pp, vecs, 1);
+ if (ret == REMOVE_PATH_DELAY) {
+ *reply = strdup("delayed\n");
+ if (*reply)
+ *len = strlen(*reply) + 1;
+ else {
+ *len = 0;
+ ret = REMOVE_PATH_FAILURE;
+ }
+ } else if (ret == REMOVE_PATH_MAP_ERROR) {
+ *reply = strdup("map reload error. removed\n");
+ if (*reply)
+ *len = strlen(*reply) + 1;
+ else {
+ *len = 0;
+ ret = REMOVE_PATH_FAILURE;
+ }
+ }
+ return (ret == REMOVE_PATH_FAILURE);
}
int
diff --git a/multipathd/main.c b/multipathd/main.c
index 6090434c..8e2beddd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -838,7 +838,7 @@ handle_path_wwid_change(struct path *pp, struct vectors *vecs)
return;
udd = udev_device_ref(pp->udev);
- if (ev_remove_path(pp, vecs, 1) != 0 && pp->mpp) {
+ if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) && pp->mpp) {
pp->dmstate = PSTATE_FAILED;
dm_fail_path(pp->mpp->alias, pp->dev_t);
}
@@ -948,8 +948,8 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
* Make another attempt to remove the path
*/
pp->mpp = prev_mpp;
- ret = ev_remove_path(pp, vecs, true);
- if (ret != 0) {
+ if (!(ev_remove_path(pp, vecs, true) &
+ REMOVE_PATH_SUCCESS)) {
/*
* Failure in ev_remove_path will keep
* path in pathvec in INIT_REMOVED state
@@ -960,6 +960,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
dm_fail_path(pp->mpp->alias, pp->dev_t);
condlog(1, "%s: failed to re-add path still mapped in %s",
pp->dev, pp->mpp->alias);
+ ret = 1;
} else if (r == PATHINFO_OK)
/*
* Path successfully freed, move on to
@@ -1167,7 +1168,6 @@ static int
uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
{
struct path *pp;
- int ret;
condlog(3, "%s: remove path (uevent)", uev->kernel);
delete_foreign(uev->udev);
@@ -1177,21 +1177,18 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
pthread_testcancel();
pp = find_path_by_dev(vecs->pathvec, uev->kernel);
if (pp)
- ret = ev_remove_path(pp, vecs, need_do_map);
+ ev_remove_path(pp, vecs, need_do_map);
lock_cleanup_pop(vecs->lock);
- if (!pp) {
- /* Not an error; path might have been purged earlier */
+ if (!pp) /* Not an error; path might have been purged earlier */
condlog(0, "%s: path already removed", uev->kernel);
- return 0;
- }
- return ret;
+ return 0;
}
int
ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
{
struct multipath * mpp;
- int i, retval = 0;
+ int i, retval = REMOVE_PATH_SUCCESS;
char params[PARAMS_SIZE] = {0};
/*
@@ -1245,7 +1242,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
condlog(2, "%s: removed map after"
" removing all paths",
alias);
- retval = 0;
/* flush_map() has freed the path */
goto out;
}
@@ -1262,11 +1258,14 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
if (mpp->wait_for_udev) {
mpp->wait_for_udev = 2;
+ retval = REMOVE_PATH_DELAY;
goto out;
}
- if (!need_do_map)
+ if (!need_do_map) {
+ retval = REMOVE_PATH_DELAY;
goto out;
+ }
/*
* reload the map
*/
@@ -1275,7 +1274,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
condlog(0, "%s: failed in domap for "
"removal of path %s",
mpp->alias, pp->dev);
- retval = 1;
+ retval = REMOVE_PATH_FAILURE;
} else {
/*
* update our state from kernel
@@ -1283,12 +1282,12 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
char devt[BLK_DEV_SIZE];
strlcpy(devt, pp->dev_t, sizeof(devt));
+
+ /* setup_multipath will free the path
+ * regardless of whether it succeeds or
+ * fails */
if (setup_multipath(vecs, mpp))
- return 1;
- /*
- * Successful map reload without this path:
- * sync_map_state() will free it.
- */
+ return REMOVE_PATH_MAP_ERROR;
sync_map_state(mpp);
condlog(2, "%s: path removed from map %s",
@@ -1304,8 +1303,10 @@ out:
return retval;
fail:
+ condlog(0, "%s: error removing path. removing map %s", pp->dev,
+ mpp->alias);
remove_map_and_stop_waiter(mpp, vecs);
- return 1;
+ return REMOVE_PATH_MAP_ERROR;
}
static int
diff --git a/multipathd/main.h b/multipathd/main.h
index ddd953f9..bc1f938f 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -13,6 +13,20 @@ enum daemon_status {
DAEMON_STATUS_SIZE,
};
+enum remove_path_result {
+ REMOVE_PATH_FAILURE = 0x0, /* path could not be removed. It is still
+ * part of the kernel map, but its state
+ * is set to INIT_REMOVED, and it will be
+ * removed at the next possible occassion */
+ REMOVE_PATH_SUCCESS = 0x1, /* path was removed */
+ REMOVE_PATH_DELAY = 0x2, /* path is set to be removed later. it
+ * currently still exists and is part of the
+ * kernel map */
+ REMOVE_PATH_MAP_ERROR = 0x5, /* map was removed because of error. value
+ * includes REMOVE_PATH_SUCCESS bit
+ * because the path was also removed */
+};
+
struct prout_param_descriptor;
struct prin_resp;
--
2.17.2
More information about the dm-devel
mailing list