[PATCH 0/3] discard broken maps in get_dm_mpvec

[PATCH 0/3] discard broken maps in get_dm_mpvec

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hello lixiaokeng,

please check if this fixes the issue you reported in
"libmultipath fix NULL dereference in select_action"
(https://www.redhat.com/archives/dm-devel/2020-August/msg00246.html).

As discussed before, the idea is to discard broken / incompletely
initialized maps in get_dm_mpvec().

Regards
Martin

Martin Wilck (3):
  libmultipath: update_multipath_table(): add flags argument
  libmultipath: remove_map(): separate pathvec and mpvec arguments
  multipath: get_dm_mpvec: discard broken maps

 libmultipath/configure.c   | 11 ++++++-----
 libmultipath/structs_vec.c | 20 ++++++++++----------
 libmultipath/structs_vec.h | 10 +++++++---
 multipath/main.c           | 23 +++++++++--------------
 multipathd/dmevents.c      |  2 +-
 multipathd/main.c          | 16 ++++++++--------
 6 files changed, 41 insertions(+), 41 deletions(-)

-- 
2.28.0

[PATCH 1/3] libmultipath: update_multipath_table(): add flags argument

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... to be passed to update_pathvec_from_dm().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 6 +++---
 libmultipath/structs_vec.h | 2 +-
 multipathd/main.c          | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index cc2dafa..3d4f855 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -406,7 +406,7 @@ extract_hwe_from_path(struct multipath * mpp)
 }
 
 int
-update_multipath_table (struct multipath *mpp, vector pathvec)
+update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 {
 	int r = DMP_ERR;
 	char params[PARAMS_SIZE] = {0};
@@ -426,7 +426,7 @@ update_multipath_table (struct multipath *mpp, vector pathvec)
 	}
 
 	/* FIXME: we should deal with the return value here */
-	update_pathvec_from_dm(pathvec, mpp, 0);
+	update_pathvec_from_dm(pathvec, mpp, flags);
 
 	return DMP_OK;
 }
@@ -532,7 +532,7 @@ update_multipath_strings(struct multipath *mpp, vector pathvec)
 	free_pgvec(mpp->pg, KEEP_PATHS);
 	mpp->pg = NULL;
 
-	r = update_multipath_table(mpp, pathvec);
+	r = update_multipath_table(mpp, pathvec, 0);
 	if (r != DMP_OK)
 		return r;
 	sync_paths(mpp, pathvec);
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 39cb074..478168f 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -39,7 +39,7 @@ struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
 void update_queue_mode_del_path(struct multipath *mpp);
 void update_queue_mode_add_path(struct multipath *mpp);
-int update_multipath_table (struct multipath *mpp, vector pathvec);
+int update_multipath_table (struct multipath *mpp, vector pathvec, int flags);
 int update_multipath_status (struct multipath *mpp);
 vector get_used_hwes(const struct _vector *pathvec);
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 9f12a57..b8019ec 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -552,7 +552,7 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
 	put_multipath_config(conf);
 
-	if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK)
+	if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK)
 		goto out;
 	if (update_multipath_status(mpp) != DMP_OK)
 		goto out;
@@ -1412,7 +1412,7 @@ map_discovery (struct vectors * vecs)
 		return 1;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i)
-		if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK ||
+		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK ||
 		    update_multipath_status(mpp) != DMP_OK) {
 			remove_map(mpp, vecs, 1);
 			i--;
-- 
2.28.0

[PATCH 2/3] libmultipath: remove_map(): separate pathvec and mpvec arguments

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This will enable us to use remove_map() from multipath.
Also, pass symbolic arguments for remove_map() everywhere.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   | 11 ++++++-----
 libmultipath/structs_vec.c | 14 +++++++-------
 libmultipath/structs_vec.h |  8 ++++++--
 multipathd/dmevents.c      |  2 +-
 multipathd/main.c          | 12 ++++++------
 5 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 5fb5767..5bc65fd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1186,7 +1186,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 		if (!mpp->paths) {
 			condlog(0, "%s: skip coalesce (no paths)", mpp->alias);
-			remove_map(mpp, vecs, 0);
+			remove_map(mpp, vecs->pathvec, vecs->mpvec, KEEP_VEC);
 			continue;
 		}
 
@@ -1215,7 +1215,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 		params[0] = '\0';
 		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
-			remove_map(mpp, vecs, 0);
+			remove_map(mpp, vecs->pathvec, vecs->mpvec, KEEP_VEC);
 			continue;
 		}
 
@@ -1235,7 +1235,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				condlog(2, "%s: %s map",
 					mpp->alias, (mpp->action == ACT_CREATE)?
 					"ignoring" : "removing");
-				remove_map(mpp, vecs, 0);
+				remove_map(mpp, vecs->pathvec, vecs->mpvec, KEEP_VEC);
 				continue;
 			} else /* if (r == DOMAP_RETRY && !is_daemon) */ {
 				ret = CP_RETRY;
@@ -1286,7 +1286,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				vector_set_slot(newmp, mpp);
 			}
 			else
-				remove_map(mpp, vecs, 0);
+				remove_map(mpp, vecs->pathvec, vecs->mpvec,
+					   KEEP_VEC);
 		}
 	}
 	/*
@@ -1304,7 +1305,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 			vector_del_slot(newmp, i);
 			i--;
-			remove_map(mpp, vecs, 0);
+			remove_map(mpp, vecs->pathvec, vecs->mpvec, KEEP_VEC);
 
 			if (dm_flush_map(alias))
 				condlog(2, "%s: remove failed (dead)",
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 3d4f855..7fd860e 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -329,18 +329,18 @@ void set_path_removed(struct path *pp)
 }
 
 void
-remove_map(struct multipath * mpp, struct vectors * vecs, int purge_vec)
+remove_map(struct multipath *mpp, vector pathvec, vector mpvec, int purge_vec)
 {
 	int i;
 
 	/*
 	 * clear references to this map
 	 */
-	orphan_paths(vecs->pathvec, mpp, "map removed internally");
+	orphan_paths(pathvec, mpp, "map removed internally");
 
 	if (purge_vec &&
-	    (i = find_slot(vecs->mpvec, (void *)mpp)) != -1)
-		vector_del_slot(vecs->mpvec, i);
+	    (i = find_slot(mpvec, (void *)mpp)) != -1)
+		vector_del_slot(mpvec, i);
 
 	/*
 	 * final free
@@ -354,7 +354,7 @@ remove_map_by_alias(const char *alias, struct vectors * vecs, int purge_vec)
 	struct multipath * mpp = find_mp_by_alias(vecs->mpvec, alias);
 	if (mpp) {
 		condlog(2, "%s: removing map by alias", alias);
-		remove_map(mpp, vecs, purge_vec);
+		remove_map(mpp, vecs->pathvec, vecs->mpvec, purge_vec);
 	}
 }
 
@@ -368,7 +368,7 @@ remove_maps(struct vectors * vecs)
 		return;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		remove_map(mpp, vecs, 1);
+		remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 		i--;
 	}
 
@@ -712,7 +712,7 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
 	return mpp;
 
 out:
-	remove_map(mpp, vecs, PURGE_VEC);
+	remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 	return NULL;
 }
 
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 478168f..ee2b723 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -27,9 +27,13 @@ int update_mpp_paths(struct multipath * mpp, vector pathvec);
 int update_multipath_strings (struct multipath *mpp, vector pathvec);
 void extract_hwe_from_path(struct multipath * mpp);
 
-#define PURGE_VEC 1
+enum {
+	KEEP_VEC,
+	PURGE_VEC,
+};
 
-void remove_map (struct multipath * mpp, struct vectors * vecs, int purge_vec);
+void remove_map (struct multipath *mpp, vector pathvec, vector mpvec,
+		 int purge_vec);
 void remove_map_by_alias(const char *alias, struct vectors * vecs,
 			 int purge_vec);
 void remove_maps (struct vectors * vecs);
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index b235dd4..5f2d210 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -357,7 +357,7 @@ static int dmevent_loop (void)
 		pthread_testcancel();
 		r = 0;
 		if (curr_dev.action == EVENT_REMOVE)
-			remove_map_by_alias(curr_dev.name, waiter->vecs, 1);
+			remove_map_by_alias(curr_dev.name, waiter->vecs, PURGE_VEC);
 		else
 			r = update_multipath(waiter->vecs, curr_dev.name, 1);
 		pthread_cleanup_pop(1);
diff --git a/multipathd/main.c b/multipathd/main.c
index b8019ec..67e9af1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -379,7 +379,7 @@ remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs)
 	condlog(3, "%s: removing map from internal tables", mpp->alias);
 	if (!poll_dmevents)
 		stop_waiter_thread(mpp);
-	remove_map(mpp, vecs, PURGE_VEC);
+	remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 }
 
 static void
@@ -511,7 +511,7 @@ retry:
 fail:
 	if (new_map && (retries < 0 || wait_for_events(mpp, vecs))) {
 		condlog(0, "%s: failed to create new map", mpp->alias);
-		remove_map(mpp, vecs, 1);
+		remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 		return 1;
 	}
 
@@ -567,7 +567,7 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 
 	return mpp;
 out:
-	remove_map(mpp, vecs, PURGE_VEC);
+	remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 	return NULL;
 }
 
@@ -1099,7 +1099,7 @@ rescan:
 		goto fail;
 
 fail_map:
-	remove_map(mpp, vecs, 1);
+	remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 fail:
 	orphan_path(pp, "failed to add path");
 	return 1;
@@ -1414,7 +1414,7 @@ map_discovery (struct vectors * vecs)
 	vector_foreach_slot (vecs->mpvec, mpp, i)
 		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK ||
 		    update_multipath_status(mpp) != DMP_OK) {
-			remove_map(mpp, vecs, 1);
+			remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 			i--;
 		}
 
@@ -2653,7 +2653,7 @@ configure (struct vectors * vecs)
 	 */
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
 		if (wait_for_events(mpp, vecs)) {
-			remove_map(mpp, vecs, 1);
+			remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 			i--;
 			continue;
 		}
-- 
2.28.0

[PATCH 3/3] multipath: get_dm_mpvec: discard broken maps

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Use the same logic as map_discovery() to discard maps that
couldn't be parsed successfully. If map parsing fails,
certain vital fields of the mpp, like features or hwhandler,
will not be set, which might cause multipath to crash later on.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 80bc4b5..2d7ec74 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -193,7 +193,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 {
 	int i;
 	struct multipath * mpp;
-	char params[PARAMS_SIZE], status[PARAMS_SIZE];
+	int flags = (cmd == CMD_LIST_SHORT ? DI_NOIO : DI_ALL);
 
 	if (dm_get_maps(curmp))
 		return 1;
@@ -205,27 +205,22 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 		if (refwwid && strlen(refwwid) &&
 		    strncmp(mpp->wwid, refwwid, WWID_SIZE)) {
 			condlog(3, "skip map %s: out of scope", mpp->alias);
-			free_multipath(mpp, KEEP_PATHS);
-			vector_del_slot(curmp, i);
+			remove_map(mpp, pathvec, curmp, PURGE_VEC);
 			i--;
 			continue;
 		}
 
-		dm_get_map(mpp->alias, &mpp->size, params);
-		condlog(3, "params = %s", params);
-		dm_get_status(mpp->alias, status);
-		condlog(3, "status = %s", status);
-
-		disassemble_map(pathvec, params, mpp);
-		update_pathvec_from_dm(pathvec, mpp,
-				       (cmd == CMD_LIST_SHORT ?
-					DI_NOIO : DI_ALL));
+		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK ||
+		    update_multipath_status(mpp) != DMP_OK) {
+			condlog(1, "error parsing map %s", mpp->wwid);
+			remove_map(mpp, pathvec, curmp, PURGE_VEC);
+			i--;
+			continue;
+		}
 
 		if (cmd == CMD_LIST_LONG)
 			mpp->bestpg = select_path_group(mpp);
 
-		disassemble_status(status, mpp);
-
 		if (cmd == CMD_LIST_SHORT ||
 		    cmd == CMD_LIST_LONG) {
 			struct config *conf = get_multipath_config();
-- 
2.28.0

Re: [PATCH 0/3] discard broken maps in get_dm_mpvec

[Benjamin Marzinski]

On Fri, Aug 21, 2020 at 03:54:15PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hello lixiaokeng,
> 
> please check if this fixes the issue you reported in
> "libmultipath fix NULL dereference in select_action"
> (https://www.redhat.com/archives/dm-devel/2020-August/msg00246.html).
> 
> As discussed before, the idea is to discard broken / incompletely
> initialized maps in get_dm_mpvec().
> 
> Regards
> Martin
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Martin Wilck (3):
>   libmultipath: update_multipath_table(): add flags argument
>   libmultipath: remove_map(): separate pathvec and mpvec arguments
>   multipath: get_dm_mpvec: discard broken maps
> 
>  libmultipath/configure.c   | 11 ++++++-----
>  libmultipath/structs_vec.c | 20 ++++++++++----------
>  libmultipath/structs_vec.h | 10 +++++++---
>  multipath/main.c           | 23 +++++++++--------------
>  multipathd/dmevents.c      |  2 +-
>  multipathd/main.c          | 16 ++++++++--------
>  6 files changed, 41 insertions(+), 41 deletions(-)
> 
> -- 
> 2.28.0

Re: [PATCH 3/3] multipath: get_dm_mpvec: discard broken maps

[lixiaokeng]

Hi Martin:
  I'm sorry for forgetting to reply this. When I test with
this patch, the multipath coredump don't cause again.

Regards
Lixiaokeng

On 2020/8/21 21:54, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Use the same logic as map_discovery() to discard maps that
> couldn't be parsed successfully. If map parsing fails,
> certain vital fields of the mpp, like features or hwhandler,
> will not be set, which might cause multipath to crash later on.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 80bc4b5..2d7ec74 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -193,7 +193,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  {
>  	int i;
>  	struct multipath * mpp;
> -	char params[PARAMS_SIZE], status[PARAMS_SIZE];
> +	int flags = (cmd == CMD_LIST_SHORT ? DI_NOIO : DI_ALL);
>  
>  	if (dm_get_maps(curmp))
>  		return 1;
> @@ -205,27 +205,22 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  		if (refwwid && strlen(refwwid) &&
>  		    strncmp(mpp->wwid, refwwid, WWID_SIZE)) {
>  			condlog(3, "skip map %s: out of scope", mpp->alias);
> -			free_multipath(mpp, KEEP_PATHS);
> -			vector_del_slot(curmp, i);
> +			remove_map(mpp, pathvec, curmp, PURGE_VEC);
>  			i--;
>  			continue;
>  		}
>  
> -		dm_get_map(mpp->alias, &mpp->size, params);
> -		condlog(3, "params = %s", params);
> -		dm_get_status(mpp->alias, status);
> -		condlog(3, "status = %s", status);
> -
> -		disassemble_map(pathvec, params, mpp);
> -		update_pathvec_from_dm(pathvec, mpp,
> -				       (cmd == CMD_LIST_SHORT ?
> -					DI_NOIO : DI_ALL));
> +		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK ||
> +		    update_multipath_status(mpp) != DMP_OK) {
> +			condlog(1, "error parsing map %s", mpp->wwid);
> +			remove_map(mpp, pathvec, curmp, PURGE_VEC);
> +			i--;
> +			continue;
> +		}
>  
>  		if (cmd == CMD_LIST_LONG)
>  			mpp->bestpg = select_path_group(mpp);
>  
> -		disassemble_status(status, mpp);
> -
>  		if (cmd == CMD_LIST_SHORT ||
>  		    cmd == CMD_LIST_LONG) {
>  			struct config *conf = get_multipath_config();
>