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

[dm-devel] [PATCH v4 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 removes an unnecessary call to rescan_paths() from
uev_update_path()

Changes from v3:
0003: Handle NULL returns from strdup(). Suggested by Martin Wilck

Changes from v2:
0003: Combined with v2 patch 0006, with changes changes in comments,
      how the return codes are defined, and how they are handled, based
      on Martin's suggestions
0006: New patch based on Martin's suggestions about wwid change handling
      in uev_update_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: fix ev_remove_path return code handling
  multipath: free vectors in configure
  kpartx: Don't leak memory when getblock returns NULL
  multipathd: don't rescan_path on wwid change in uev_update_path

 kpartx/kpartx.c            |  2 ++
 libmultipath/structs_vec.c |  4 +--
 multipath/main.c           |  7 ++++-
 multipathd/cli_handlers.c  | 24 ++++++++++++++--
 multipathd/main.c          | 56 ++++++++++++++++++++------------------
 multipathd/main.h          | 14 ++++++++++
 6 files changed, 75 insertions(+), 32 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH v4 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>
Reviewed-by: Martin Wilck <mwilck@suse.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 v4 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 v4 3/6] multipathd: fix ev_remove_path return code handling

[Benjamin Marzinski]

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

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

[dm-devel] [PATCH v4 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>
Reviewed-by: Martin Wilck <mwilck@suse.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 v4 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 v4 6/6] multipathd: don't rescan_path on wwid change in uev_update_path

[Benjamin Marzinski]

If get_uid() is returning a different wwid in uev_update_path(), then
the uid_attribute must have already gotten updated, which was the
purpose behind calling rescan_path() in the first place.

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 8e2beddd..2750f5e9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1359,7 +1359,6 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			condlog(0, "%s: path wwid changed from '%s' to '%s'",
 				uev->kernel, wwid, pp->wwid);
 			ev_remove_path(pp, vecs, 1);
-			rescan_path(uev->udev);
 			needs_reinit = 1;
 			goto out;
 		} else {
-- 
2.17.2

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

Re: [dm-devel] [PATCH v4 3/6] multipathd: fix ev_remove_path return code handling

[Martin Wilck]

On Mon, 2021-05-17 at 11:29 -0500, Benjamin Marzinski wrote:
> 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>+

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 v4 3/6] multipathd: fix ev_remove_path return code handling

[Martin Wilck]

On Mon, 2021-05-17 at 20:51 +0200, Martin Wilck wrote:
> On Mon, 2021-05-17 at 11:29 -0500, Benjamin Marzinski wrote:
> > 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>+
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> 

It just occured to me that we should probably not have set changed the
return code of cli_del_path() because of a strdup() failure for the
reply string. (It was my suggestion, I know).

Anyway, I've pushed this to "queue" already.
We can change this in a follow-up patch.

Regards
Martin


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

Re: [dm-devel] [PATCH v4 3/6] multipathd: fix ev_remove_path return code handling

[Benjamin Marzinski]

On Mon, May 17, 2021 at 07:33:34PM +0000, Martin Wilck wrote:
> On Mon, 2021-05-17 at 20:51 +0200, Martin Wilck wrote:
> > On Mon, 2021-05-17 at 11:29 -0500, Benjamin Marzinski wrote:
> > > 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>+
> > 
> > Reviewed-by: Martin Wilck <mwilck@suse.com>
> > 
> 
> It just occured to me that we should probably not have set changed the
> return code of cli_del_path() because of a strdup() failure for the
> reply string. (It was my suggestion, I know).

If we're at a point where we get an error on that strdup(), things are
probably going badly in general. But yeah, I agree that success makes
more sense than failure here.

-Ben
 
> Anyway, I've pushed this to "queue" already.
> We can change this in a follow-up patch.
> 
> Regards
> Martin

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