From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Benjamin Marzinski <bmarzins@redhat.com>

Date: Tue, 11 May 2021 13:58:57 -0500

Subject: [PATCH] multipathd: fix ev_remove_path return code handling

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@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;