dm-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
From: mwilck@suse.com
To: Benjamin Marzinski <bmarzins@redhat.com>,
	Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: dm-devel@redhat.com, Martin Wilck <mwilck@suse.com>
Subject: [dm-devel] [PATCH v2 1/9] libmultipath: variable-size parameters in dm_get_map()
Date: Wed, 11 Aug 2021 17:41:42 +0200	[thread overview]
Message-ID: <20210811154150.24714-2-mwilck@suse.com> (raw)
In-Reply-To: <20210811154150.24714-1-mwilck@suse.com>

From: Martin Wilck <mwilck@suse.com>

We've seen a crash of multipath in disassemble_map because of a params
string exceeding PARAMS_SIZE. While the crash could have been fixed by
a simple error check, I believe multipath should be able to work with
arbitrary long parameter strings passed from the kernel.

The parameter list of dm_get_map() has changed. Bumped ABI version and
removed dm_get_map() and some functions from the ABI, which are only
called from libmultipath itself.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c          | 44 ++++++++++++++++++++-----------
 libmultipath/devmapper.h          |  4 +--
 libmultipath/libmultipath.version |  6 +----
 libmultipath/structs_vec.c        | 11 +++++---
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 945e625..c05dc20 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -648,7 +648,7 @@ int dm_map_present(const char * str)
 	return (do_get_info(str, &info) == 0);
 }
 
-int dm_get_map(const char *name, unsigned long long *size, char *outparams)
+int dm_get_map(const char *name, unsigned long long *size, char **outparams)
 {
 	int r = DMP_ERR;
 	struct dm_task *dmt;
@@ -682,12 +682,13 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
 	if (size)
 		*size = length;
 
-	if (!outparams) {
+	if (!outparams)
 		r = DMP_OK;
-		goto out;
+	else {
+		*outparams = strdup(params);
+		r = *outparams ? DMP_OK : DMP_ERR;
 	}
-	if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= PARAMS_SIZE)
-		r = DMP_OK;
+
 out:
 	dm_task_destroy(dmt);
 	return r;
@@ -761,7 +762,7 @@ is_mpath_part(const char *part_name, const char *map_name)
 	return 0;
 }
 
-int dm_get_status(const char *name, char *outstatus)
+int dm_get_status(const char *name, char **outstatus)
 {
 	int r = DMP_ERR;
 	struct dm_task *dmt;
@@ -799,8 +800,12 @@ int dm_get_status(const char *name, char *outstatus)
 		goto out;
 	}
 
-	if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= PARAMS_SIZE)
+	if (!outstatus)
 		r = DMP_OK;
+	else {
+		*outstatus = strdup(status);
+		r = *outstatus ? DMP_OK : DMP_ERR;
+	}
 out:
 	if (r != DMP_OK)
 		condlog(0, "%s: error getting map status string", name);
@@ -1049,7 +1054,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 	int queue_if_no_path = 0;
 	int udev_flags = 0;
 	unsigned long long mapsize;
-	char params[PARAMS_SIZE] = {0};
+	char *params = NULL;
 
 	if (dm_is_mpath(mapname) != 1)
 		return 0; /* nothing to do */
@@ -1065,7 +1070,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 			return 1;
 
 	if (need_suspend &&
-	    dm_get_map(mapname, &mapsize, params) == DMP_OK &&
+	    dm_get_map(mapname, &mapsize, &params) == DMP_OK &&
 	    strstr(params, "queue_if_no_path")) {
 		if (!dm_queue_if_no_path(mapname, 0))
 			queue_if_no_path = 1;
@@ -1073,6 +1078,8 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 			/* Leave queue_if_no_path alone if unset failed */
 			queue_if_no_path = -1;
 	}
+	free(params);
+	params = NULL;
 
 	if (dm_remove_partmaps(mapname, need_sync, deferred_remove))
 		return 1;
@@ -1431,7 +1438,7 @@ do_foreach_partmaps (const char * mapname,
 	struct dm_task *dmt;
 	struct dm_names *names;
 	unsigned next = 0;
-	char params[PARAMS_SIZE];
+	char *params = NULL;
 	unsigned long long size;
 	char dev_t[32];
 	int r = 1;
@@ -1474,7 +1481,7 @@ do_foreach_partmaps (const char * mapname,
 		    /*
 		     * and we can fetch the map table from the kernel
 		     */
-		    dm_get_map(names->name, &size, &params[0]) == DMP_OK &&
+		    dm_get_map(names->name, &size, &params) == DMP_OK &&
 
 		    /*
 		     * and the table maps over the multipath map
@@ -1486,12 +1493,15 @@ do_foreach_partmaps (const char * mapname,
 				goto out;
 		}
 
+		free(params);
+		params = NULL;
 		next = names->next;
 		names = (void *) names + next;
 	} while (next);
 
 	r = 0;
 out:
+	free(params);
 	dm_task_destroy (dmt);
 	return r;
 }
@@ -1620,17 +1630,19 @@ struct rename_data {
 static int
 rename_partmap (const char *name, void *data)
 {
-	char buff[PARAMS_SIZE];
+	char *buff = NULL;
 	int offset;
 	struct rename_data *rd = (struct rename_data *)data;
 
 	if (strncmp(name, rd->old, strlen(rd->old)) != 0)
 		return 0;
 	for (offset = strlen(rd->old); name[offset] && !(isdigit(name[offset])); offset++); /* do nothing */
-	snprintf(buff, PARAMS_SIZE, "%s%s%s", rd->new, rd->delim,
-		 name + offset);
-	dm_rename(name, buff, rd->delim, SKIP_KPARTX_OFF);
-	condlog(4, "partition map %s renamed", name);
+	if (asprintf(&buff, "%s%s%s", rd->new, rd->delim, name + offset) >= 0) {
+		dm_rename(name, buff, rd->delim, SKIP_KPARTX_OFF);
+		free(buff);
+		condlog(4, "partition map %s renamed", name);
+	} else
+		condlog(1, "failed to rename partition map %s", name);
 	return 0;
 }
 
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index e29b4d4..45a676d 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -44,8 +44,8 @@ int dm_addmap_create (struct multipath *mpp, char *params);
 int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
 int dm_map_present (const char *);
 int dm_map_present_by_uuid(const char *uuid);
-int dm_get_map(const char *, unsigned long long *, char *);
-int dm_get_status(const char *, char *);
+int dm_get_map(const char *, unsigned long long *, char **);
+int dm_get_status(const char *, char **);
 int dm_type(const char *, char *);
 int dm_is_mpath(const char *);
 int _dm_flush_map (const char *, int, int, int, int);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 0cff311..7567837 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_5.0.0 {
+LIBMULTIPATH_6.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -58,8 +58,6 @@ global:
 	count_active_paths;
 	delete_all_foreign;
 	delete_foreign;
-	disassemble_map;
-	disassemble_status;
 	dlog;
 	dm_cancel_deferred_remove;
 	dm_enablegroup;
@@ -70,10 +68,8 @@ global:
 	dm_geteventnr;
 	dm_get_info;
 	dm_get_major_minor;
-	dm_get_map;
 	dm_get_maps;
 	dm_get_multipath;
-	dm_get_status;
 	dm_get_uuid;
 	dm_is_mpath;
 	dm_mapname;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 7539019..24d6fd2 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -416,12 +416,12 @@ int
 update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 {
 	int r = DMP_ERR;
-	char params[PARAMS_SIZE] = {0};
+	char *params = NULL;
 
 	if (!mpp)
 		return r;
 
-	r = dm_get_map(mpp->alias, &mpp->size, params);
+	r = dm_get_map(mpp->alias, &mpp->size, &params);
 	if (r != DMP_OK) {
 		condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting table" : "map not present");
 		return r;
@@ -429,14 +429,17 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 
 	if (disassemble_map(pathvec, params, mpp)) {
 		condlog(2, "%s: cannot disassemble map", mpp->alias);
+		free(params);
 		return DMP_ERR;
 	}
 
-	*params = '\0';
-	if (dm_get_status(mpp->alias, params) != DMP_OK)
+	free(params);
+	params = NULL;
+	if (dm_get_status(mpp->alias, &params) != DMP_OK)
 		condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
 	else if (disassemble_status(params, mpp))
 		condlog(2, "%s: cannot disassemble status", mpp->alias);
+	free(params);
 
 	/* FIXME: we should deal with the return value here */
 	update_pathvec_from_dm(pathvec, mpp, flags);
-- 
2.32.0


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


  reply	other threads:[~2021-08-11 15:43 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-11 15:41 [dm-devel] [PATCH v2 0/9] multipath-tools: use variable-size string buffers mwilck
2021-08-11 15:41 ` mwilck [this message]
2021-08-30 20:44   ` [dm-devel] [PATCH v2 1/9] libmultipath: variable-size parameters in dm_get_map() Benjamin Marzinski
2021-08-11 15:41 ` [dm-devel] [PATCH v2 2/9] libmultipath: strbuf: simple api for growing string buffers mwilck
2021-08-11 16:08   ` Bart Van Assche
2021-08-12  7:19     ` Martin Wilck
2021-08-30 20:45   ` Benjamin Marzinski
2021-08-11 15:41 ` [dm-devel] [PATCH v2 3/9] libmultipath: variable-size parameters in assemble_map() mwilck
2021-08-30 20:45   ` Benjamin Marzinski
2021-08-11 15:41 ` [dm-devel] [PATCH v2 4/9] libmultipath: use strbuf in dict.c mwilck
2021-08-30 20:46   ` Benjamin Marzinski
2021-08-11 15:41 ` [dm-devel] [PATCH v2 5/9] libmultipath: use strbuf in print.c mwilck
2021-08-30 20:47   ` Benjamin Marzinski
2021-08-11 15:41 ` [dm-devel] [PATCH v2 6/9] libmultipath: print.c: fail hard if keywords are not found mwilck
2021-08-11 15:41 ` [dm-devel] [PATCH v2 7/9] libmultipath: print.h: move macros to print.c mwilck
2021-08-11 15:41 ` [dm-devel] [PATCH v2 8/9] libmultipath: use strbuf in alias.c mwilck
2021-08-11 15:41 ` [dm-devel] [PATCH v2 9/9] multipathd: use strbuf in cli.c mwilck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210811154150.24714-2-mwilck@suse.com \
    --to=mwilck@suse.com \
    --cc=bmarzins@redhat.com \
    --cc=christophe.varoqui@opensvc.com \
    --cc=dm-devel@redhat.com \
    --subject='Re: [dm-devel] [PATCH v2 1/9] libmultipath: variable-size parameters in dm_get_map()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).