[dm-devel] [PATCH v2 0/6] Memory issues found by coverity

[dm-devel] [PATCH v2 0/6] Memory issues found by coverity

[Benjamin Marzinski]

This is collection of issues found by coverity. The first three patches
deal with ev_remove_path() removing the path, but returning failure,
causing a use-after-free error. The next two patches fix memory leaks.
The final patch cleans up the returns from ev_remove_path().

Changes from v1:
0001: changed comment based on Martin's suggestion
0004: moved location of atexit() call based on Martin's suggestion
0006: New patch, based on Martin's comments on patch 0003

Benjamin Marzinski (6):
  multipathd: don't fail to remove path once the map is removed
  multipathd: remove duplicate orphan_paths in flush_map
  multipathd: make ev_remove_path return success on path removal
  multipath: free vectors in configure
  kpartx: Don't leak memory when getblock returns NULL
  multipathd: use symbolic returns for ev_remove_path()

 kpartx/kpartx.c            |  2 ++
 libmultipath/structs_vec.c |  4 +--
 multipath/main.c           |  7 +++++-
 multipathd/cli_handlers.c  | 14 +++++++++--
 multipathd/main.c          | 51 ++++++++++++++++++++++----------------
 multipathd/main.h          |  9 +++++++
 6 files changed, 60 insertions(+), 27 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 1/6] multipathd: don't fail to remove path once the map is removed

[Benjamin Marzinski]

In ev_remove_path(), if update_mpp_paths() fails, we delete the entire
map. However, since update_mpp_paths() happens before we call
set_path_removed(), pp->initialized isn't set to INIT_REMOVED, so
remove_map_and_stop_waiter() doesn't remove the path when in removes the
map.  But with the map removed, there's nothing to keep us from removing
the path.

Call set_path_removed() before update_mpp_paths() to avoid the odd case
of ev_remove_path() removing the map but not the path.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c |  4 ++--
 multipathd/main.c          | 13 ++++++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index d242c06b..75390198 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -45,8 +45,8 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec)
 
 				/*
 				 * Avoid adding removed paths to the map again
-				 * when we reload it. Such paths may exist if
-				 * domap fails in ev_remove_path().
+				 * when we reload it. Such paths may exist in
+				 * ev_remove_paths() or if it returns failure.
 				 */
 				pp1 = find_path_by_devt(pathvec, pp->dev_t);
 				if (pp1 && pp->initialized != INIT_REMOVED &&
diff --git a/multipathd/main.c b/multipathd/main.c
index 102946bf..449ce384 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1199,6 +1199,13 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 	 * avoid referring to the map of an orphaned path
 	 */
 	if ((mpp = pp->mpp)) {
+		/*
+		 * Mark the path as removed. In case of success, we
+		 * will delete it for good. Otherwise, it will be deleted
+		 * later, unless all attempts to reload this map fail.
+		 */
+		set_path_removed(pp);
+
 		/*
 		 * transform the mp->pg vector of vectors of paths
 		 * into a mp->params string to feed the device-mapper
@@ -1210,13 +1217,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		}
 
 		/*
-		 * Mark the path as removed. In case of success, we
-		 * will delete it for good. Otherwise, it will be deleted
-		 * later, unless all attempts to reload this map fail.
-		 * Note: we have to explicitly remove pp from mpp->paths,
+		 * we have to explicitly remove pp from mpp->paths,
 		 * update_mpp_paths() doesn't do that.
 		 */
-		set_path_removed(pp);
 		i = find_slot(mpp->paths, pp);
 		if (i != -1)
 			vector_del_slot(mpp->paths, i);
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 2/6] multipathd: remove duplicate orphan_paths in flush_map

[Benjamin Marzinski]

remove_map_and_stop_waiter() already calls orphan_paths() so flush_map()
doesn't need to call orphan_paths() before calling
remove_map_and_stop_waiter().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 449ce384..6090434c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -660,7 +660,6 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths)
 	else
 		condlog(2, "%s: map flushed", mpp->alias);
 
-	orphan_paths(vecs->pathvec, mpp, "map flushed");
 	remove_map_and_stop_waiter(mpp, vecs);
 
 	return 0;
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 3/6] multipathd: make ev_remove_path return success on path removal

[Benjamin Marzinski]

When ev_remove_path() returns success, callers assume that the path (and
possibly the map) has been removed.  When ev_remove_path() returns
failure, callers assume that the path has 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. Change
ev_remove_path() to return success whenever the path is removed, even if
the map was removed due to a failure when trying to reload it. Found by
coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 6090434c..4bdf14bd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1284,7 +1284,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 
 			strlcpy(devt, pp->dev_t, sizeof(devt));
 			if (setup_multipath(vecs, mpp))
-				return 1;
+				return 0;
 			/*
 			 * Successful map reload without this path:
 			 * sync_map_state() will free it.
@@ -1304,8 +1304,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 0;
 }
 
 static int
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 4/6] multipath: free vectors in configure

[Benjamin Marzinski]

configure() can retry multiple times, each time reallocing a maps and
paths vector, and leaking the previous ones. Fix this by always freeing
the vectors before configure() exits. Found by coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/multipath/main.c b/multipath/main.c
index ef89c7cf..8fc0e15f 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -466,7 +466,6 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	 */
 	curmp = vector_alloc();
 	pathvec = vector_alloc();
-	atexit(cleanup_vecs);
 
 	if (!curmp || !pathvec) {
 		condlog(0, "can not allocate memory");
@@ -578,6 +577,11 @@ out:
 	if (refwwid)
 		FREE(refwwid);
 
+	free_multipathvec(curmp, KEEP_PATHS);
+	vecs.mpvec = NULL;
+	free_pathvec(pathvec, FREE_PATHS);
+	vecs.pathvec = NULL;
+
 	return r;
 }
 
@@ -823,6 +827,7 @@ main (int argc, char *argv[])
 	conf = get_multipath_config();
 	conf->retrigger_tries = 0;
 	conf->force_sync = 1;
+	atexit(cleanup_vecs);
 	while ((arg = getopt(argc, argv, ":adDcChl::eFfM:v:p:b:BrR:itTquUwW")) != EOF ) {
 		switch(arg) {
 		case 1: printf("optarg : %s\n",optarg);
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 5/6] kpartx: Don't leak memory when getblock returns NULL

[Benjamin Marzinski]

If a new block was allocated, but couldn't be filled, getblock will
discard it. When it does so, it needs to free the block to avoid leaking
memory. Found by coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 8ff116b8..7bc64543 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -766,6 +766,8 @@ getblock (int fd, unsigned int blknr) {
 	if (read(fd, bp->block, secsz) != secsz) {
 		fprintf(stderr, "read error, sector %d\n", secnr);
 		blockhead = bp->next;
+		free(bp->block);
+		free(bp);
 		return NULL;
 	}
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 6/6] multipathd: use symbolic returns for ev_remove_path()

[Benjamin Marzinski]

There are many possible outcomes of calling ev_remove_path(), and not
all callers agree on which outcomes are a success and which are a
failure. So ev_remove_path() should simply return a different value for
each outcome, and the callers can decide how to deal with them.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 14 ++++++++++++--
 multipathd/main.c         | 35 +++++++++++++++++++----------------
 multipathd/main.h         |  9 +++++++++
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 1de6ad8e..1462ea84 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,15 @@ 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");
+		*len = strlen(*reply) + 1;
+	} else if (ret == REMOVE_PATH_MAP_ERROR) {
+		*reply = strdup("map reload error. removed\n");
+		*len = strlen(*reply) + 1;
+	}
+	return (ret == REMOVE_PATH_FAILURE);
 }
 
 int
diff --git a/multipathd/main.c b/multipathd/main.c
index 4bdf14bd..72fb7e38 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,7 @@ static int
 uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 {
 	struct path *pp;
-	int ret;
+	int ret = 0;
 
 	condlog(3, "%s: remove path (uevent)", uev->kernel);
 	delete_foreign(uev->udev);
@@ -1176,8 +1177,8 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	lock(&vecs->lock);
 	pthread_testcancel();
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
-	if (pp)
-		ret = ev_remove_path(pp, vecs, need_do_map);
+	if (pp && ev_remove_path(pp, vecs, need_do_map) == REMOVE_PATH_FAILURE)
+		ret = 1;
 	lock_cleanup_pop(vecs->lock);
 	if (!pp) {
 		/* Not an error; path might have been purged earlier */
@@ -1191,7 +1192,7 @@ 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 +1246,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 +1262,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 +1278,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 +1286,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 0;
-			/*
-			 * 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",
@@ -1307,7 +1310,7 @@ fail:
 	condlog(0, "%s: error removing path. removing map %s", pp->dev,
 		mpp->alias);
 	remove_map_and_stop_waiter(mpp, vecs);
-	return 0;
+	return REMOVE_PATH_MAP_ERROR;
 }
 
 static int
diff --git a/multipathd/main.h b/multipathd/main.h
index ddd953f9..24d050c8 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -13,6 +13,15 @@ enum daemon_status {
 	DAEMON_STATUS_SIZE,
 };
 
+#define REMOVE_PATH_FAILURE 0x0 /* path was not removed */
+#define REMOVE_PATH_SUCCESS 0x1 /* path was removed */
+#define REMOVE_PATH_DELAY 0x2 /* path is set to be removed later. it
+			       * currently still exists and is part of the
+			       * kernel map */
+#define 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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 1/6] multipathd: don't fail to remove path once the map is removed

[Martin Wilck]

On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> In ev_remove_path(), if update_mpp_paths() fails, we delete the
> entire
> map. However, since update_mpp_paths() happens before we call
> set_path_removed(), pp->initialized isn't set to INIT_REMOVED, so
> remove_map_and_stop_waiter() doesn't remove the path when in removes
> the
> map.  But with the map removed, there's nothing to keep us from
> removing
> the path.
> 
> Call set_path_removed() before update_mpp_paths() to avoid the odd
> case
> of ev_remove_path() removing the map but not the path.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 4/6] multipath: free vectors in configure

[Martin Wilck]

On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> configure() can retry multiple times, each time reallocing a maps and
> paths vector, and leaking the previous ones. Fix this by always
> freeing
> the vectors before configure() exits. Found by coverity.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 3/6] multipathd: make ev_remove_path return success on path removal

[Martin Wilck]

On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> When ev_remove_path() returns success, callers assume that the path
> (and
> possibly the map) has been removed.  When ev_remove_path() returns
> failure, callers assume that the path has 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. Change
> ev_remove_path() to return success whenever the path is removed, even
> if
> the map was removed due to a failure when trying to reload it. Found
> by
> coverity.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

I'd have preferred to to join this with patch no. 6. Anyway:

Reviewed-by: Martin Wilck <mwilck@suse.com>


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 6/6] multipathd: use symbolic returns for ev_remove_path()

[Martin Wilck]

On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> There are many possible outcomes of calling ev_remove_path(), and not
> all callers agree on which outcomes are a success and which are a
> failure. So ev_remove_path() should simply return a different value
> for
> each outcome, and the callers can decide how to deal with them.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/cli_handlers.c | 14 ++++++++++++--
>  multipathd/main.c         | 35 +++++++++++++++++++----------------
>  multipathd/main.h         |  9 +++++++++
>  3 files changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 1de6ad8e..1462ea84 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,15 @@ 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");
> +               *len = strlen(*reply) + 1;
> +       } else if (ret == REMOVE_PATH_MAP_ERROR) {
> +               *reply = strdup("map reload error. removed\n");
> +               *len = strlen(*reply) + 1;
> +       }
> +       return (ret == REMOVE_PATH_FAILURE);
>  }
>  
>  int
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4bdf14bd..72fb7e38 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,7 @@ static int
>  uev_remove_path (struct uevent *uev, struct vectors * vecs, int
> need_do_map)
>  {
>         struct path *pp;
> -       int ret;
> +       int ret = 0;
>  
>         condlog(3, "%s: remove path (uevent)", uev->kernel);
>         delete_foreign(uev->udev);
> @@ -1176,8 +1177,8 @@ uev_remove_path (struct uevent *uev, struct
> vectors * vecs, int need_do_map)
>         lock(&vecs->lock);
>         pthread_testcancel();
>         pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> -       if (pp)
> -               ret = ev_remove_path(pp, vecs, need_do_map);
> +       if (pp && ev_remove_path(pp, vecs, need_do_map) ==
> REMOVE_PATH_FAILURE)
> +               ret = 1;
>         lock_cleanup_pop(vecs->lock);
>         if (!pp) {
>                 /* Not an error; path might have been purged earlier
> */
> @@ -1191,7 +1192,7 @@ 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 +1246,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 +1262,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 +1278,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;

Hm. With the introduction of INIT_REMOVED, this failure isn't fatal any
more. As far as multipathd is concerned, the path is removed and will
be deleted from the map as soon as the reason for the domap() failure
(likely a problem with some other device in the map) is resolved.
There's no difference from the REMOVE_PATH_DELAY case wrt how the path
will be treated in the future.

So while I agree it's reasonable to distinguish this case from the
"delay without failure" cases above, I'm unsure if we should treat it
as an error in uev_remove_path() (or uev_trigger(), for that matter).


>                 } else {
>                         /*
>                          * update our state from kernel
> @@ -1283,12 +1286,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 0;
> -                       /*
> -                        * 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",
> @@ -1307,7 +1310,7 @@ fail:
>         condlog(0, "%s: error removing path. removing map %s", pp-
> >dev,
>                 mpp->alias);
>         remove_map_and_stop_waiter(mpp, vecs);
> -       return 0;
> +       return REMOVE_PATH_MAP_ERROR;
>  }
>  
>  static int
> diff --git a/multipathd/main.h b/multipathd/main.h
> index ddd953f9..24d050c8 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -13,6 +13,15 @@ enum daemon_status {
>         DAEMON_STATUS_SIZE,
>  };
>  
> +#define REMOVE_PATH_FAILURE 0x0 /* path was not removed */

We should add a remark that this is normally non-fatal, it's init state
is set to INIT_REMOVED, and that it will be removed at the next
possible occasion. The only thing that should be avoided is to try and
add another path with the same major/minor number.

Use an enum, maybe?

Regards,
Martin


> +#define REMOVE_PATH_SUCCESS 0x1 /* path was removed */
> +#define REMOVE_PATH_DELAY 0x2 /* path is set to be removed later. it
> +                              * currently still exists and is part
> of the
> +                              * kernel map */
> +#define 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;
>  


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 6/6] multipathd: use symbolic returns for ev_remove_path()

[Benjamin Marzinski]

On Thu, May 13, 2021 at 08:11:13PM +0000, Martin Wilck wrote:
> On Thu, 2021-05-13 at 12:23 -0500, Benjamin Marzinski wrote:
> > There are many possible outcomes of calling ev_remove_path(), and not
> > all callers agree on which outcomes are a success and which are a
> > failure. So ev_remove_path() should simply return a different value
> > for
> > each outcome, and the callers can decide how to deal with them.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  multipathd/cli_handlers.c | 14 ++++++++++++--
> >  multipathd/main.c         | 35 +++++++++++++++++++----------------
> >  multipathd/main.h         |  9 +++++++++
> >  3 files changed, 40 insertions(+), 18 deletions(-)
> > 
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 1de6ad8e..1462ea84 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,15 @@ 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");
> > +               *len = strlen(*reply) + 1;
> > +       } else if (ret == REMOVE_PATH_MAP_ERROR) {
> > +               *reply = strdup("map reload error. removed\n");
> > +               *len = strlen(*reply) + 1;
> > +       }
> > +       return (ret == REMOVE_PATH_FAILURE);
> >  }
> >  
> >  int
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 4bdf14bd..72fb7e38 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,7 @@ static int
> >  uev_remove_path (struct uevent *uev, struct vectors * vecs, int
> > need_do_map)
> >  {
> >         struct path *pp;
> > -       int ret;
> > +       int ret = 0;
> >  
> >         condlog(3, "%s: remove path (uevent)", uev->kernel);
> >         delete_foreign(uev->udev);
> > @@ -1176,8 +1177,8 @@ uev_remove_path (struct uevent *uev, struct
> > vectors * vecs, int need_do_map)
> >         lock(&vecs->lock);
> >         pthread_testcancel();
> >         pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > -       if (pp)
> > -               ret = ev_remove_path(pp, vecs, need_do_map);
> > +       if (pp && ev_remove_path(pp, vecs, need_do_map) ==
> > REMOVE_PATH_FAILURE)
> > +               ret = 1;
> >         lock_cleanup_pop(vecs->lock);
> >         if (!pp) {
> >                 /* Not an error; path might have been purged earlier
> > */
> > @@ -1191,7 +1192,7 @@ 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 +1246,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 +1262,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 +1278,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;
> 
> Hm. With the introduction of INIT_REMOVED, this failure isn't fatal any
> more. As far as multipathd is concerned, the path is removed and will
> be deleted from the map as soon as the reason for the domap() failure
> (likely a problem with some other device in the map) is resolved.
> There's no difference from the REMOVE_PATH_DELAY case wrt how the path
> will be treated in the future.
> 
> So while I agree it's reasonable to distinguish this case from the
> "delay without failure" cases above, I'm unsure if we should treat it
> as an error in uev_remove_path() (or uev_trigger(), for that matter).

Sure. All that a failure does is print an error message anyway, and a
message already gets printed if domap fails, so another message won't
help with debugging problems.
 
> 
> >                 } else {
> >                         /*
> >                          * update our state from kernel
> > @@ -1283,12 +1286,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 0;
> > -                       /*
> > -                        * 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",
> > @@ -1307,7 +1310,7 @@ fail:
> >         condlog(0, "%s: error removing path. removing map %s", pp-
> > >dev,
> >                 mpp->alias);
> >         remove_map_and_stop_waiter(mpp, vecs);
> > -       return 0;
> > +       return REMOVE_PATH_MAP_ERROR;
> >  }
> >  
> >  static int
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index ddd953f9..24d050c8 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -13,6 +13,15 @@ enum daemon_status {
> >         DAEMON_STATUS_SIZE,
> >  };
> >  
> > +#define REMOVE_PATH_FAILURE 0x0 /* path was not removed */
> 
> We should add a remark that this is normally non-fatal, it's init state
> is set to INIT_REMOVED, and that it will be removed at the next
> possible occasion. The only thing that should be avoided is to try and
> add another path with the same major/minor number.

Sure.

> Use an enum, maybe?

I can do that. 

-Ben

> Regards,
> Martin
> 
> 
> > +#define REMOVE_PATH_SUCCESS 0x1 /* path was removed */
> > +#define REMOVE_PATH_DELAY 0x2 /* path is set to be removed later. it
> > +                              * currently still exists and is part
> > of the
> > +                              * kernel map */
> > +#define 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;
> >  

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel