[PATCH v3 00/87] multipath-tools series part VII: addional patches

[PATCH v3 00/87] multipath-tools series part VII: addional patches

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is v3 of part VII of a larger patch series for multipath-tools I've
been preparing. It's based on the previously submitted part VI.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-200819
This part is tagged "submit-200819-7".

This part contains patches added since the v1 and v2 submissions of the series.
One is Liu's fix for disassemble_map(), rebased on top of my previous changes
to that function.

84/87, related to inconsistent alias settings, belongs logically to part III.
I couldn't add it there because it would have changed the numbering in the
series wrt v1, which I wanted to avoid.

Changes v2 -> v3, as suggested by Ben Marzinski:

 84/87 "libmultipath: add consistency check for alias settings"
   - fixed possible freeing of NULL pointer
   - search vector backwards in add_binding()
   - avoid fd leak in check_alias_settings()

 85/87 "libmultipath: alias.c: use strtok_r() instead of strtok()"
   - new, provide thread safety in alias.c

 86/87 "libmultipath: adopt_paths(): set pp->mpp only on success"
   - new, implement Ben's suggestion in his review of patch 54 in the
     v2 series ("[PATCH v2 44/54] libmultipath: adopt_paths(): don't 
     bail out on single path failure")

Besides, a trivial warning fix was added (87/87).

Martin Wilck (6):
  multipath: check_path_valid(): eliminate some failure modes
  libmultipath: alias: static const variable for BINDINGS_FILE_HEADER
  libmultipath: add consistency check for alias settings
  libmultipath: alias.c: use strtok_r() instead of strtok()
  libmultipath: adopt_paths(): set pp->mpp only on success
  libmultipath: fix a -Wformat-truncation warning from gcc 10

Zhiqiang Liu (1):
  libmultipath: free pp if store_path fails in disassemble_map

 libmultipath/alias.c       | 294 +++++++++++++++++++++++++++++++++++--
 libmultipath/alias.h       |  15 +-
 libmultipath/dmparser.c    |  12 +-
 libmultipath/structs_vec.c |  15 +-
 libmultipath/util.c        |   6 +-
 multipath/main.c           |  21 +--
 multipathd/main.c          |   3 +
 7 files changed, 327 insertions(+), 39 deletions(-)

-- 
2.28.0

[PATCH v3 84/87] libmultipath: add consistency check for alias settings

[mwilck]

From: Martin Wilck <mwilck@suse.com>

A typo in a config file, assigning the same alias to multiple WWIDs,
can cause massive confusion and even data corruption. Check and
if possible fix the bindings file in such cases.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 265 +++++++++++++++++++++++++++++++++++++++++++
 libmultipath/alias.h |   3 +
 multipath/main.c     |   3 +
 multipathd/main.c    |   3 +
 4 files changed, 274 insertions(+)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 0759c4e..df44bdc 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -4,6 +4,7 @@
  */
 #include <stdlib.h>
 #include <errno.h>
+#include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
 #include <limits.h>
@@ -17,6 +18,9 @@
 #include "vector.h"
 #include "checkers.h"
 #include "structs.h"
+#include "config.h"
+#include "util.h"
+#include "errno.h"
 
 
 /*
@@ -438,3 +442,264 @@ get_user_friendly_wwid(const char *alias, char *buff, const char *file)
 	fclose(f);
 	return 0;
 }
+
+struct binding {
+	char *alias;
+	char *wwid;
+};
+
+static void _free_binding(struct binding *bdg)
+{
+	free(bdg->wwid);
+	free(bdg->alias);
+	free(bdg);
+}
+
+/*
+ * Perhaps one day we'll implement this more efficiently, thus use
+ * an abstract type.
+ */
+typedef struct _vector Bindings;
+
+static void free_bindings(Bindings *bindings)
+{
+	struct binding *bdg;
+	int i;
+
+	vector_foreach_slot(bindings, bdg, i)
+		_free_binding(bdg);
+	vector_reset(bindings);
+}
+
+enum {
+	BINDING_EXISTS,
+	BINDING_CONFLICT,
+	BINDING_ADDED,
+	BINDING_DELETED,
+	BINDING_NOTFOUND,
+	BINDING_ERROR,
+};
+
+static int add_binding(Bindings *bindings, const char *alias, const char *wwid)
+{
+	struct binding *bdg;
+	int i, cmp = 0;
+
+	/*
+	 * Keep the bindings array sorted by alias.
+	 * Optimization: Search backwards, assuming that the bindings file is
+	 * sorted already.
+	 */
+	vector_foreach_slot_backwards(bindings, bdg, i) {
+		if ((cmp = strcmp(bdg->alias, alias)) <= 0)
+			break;
+	}
+
+	/* Check for exact match */
+	if (i >= 0 && cmp == 0)
+		return strcmp(bdg->wwid, wwid) ?
+			BINDING_CONFLICT : BINDING_EXISTS;
+
+	i++;
+	bdg = calloc(1, sizeof(*bdg));
+	if (bdg) {
+		bdg->wwid = strdup(wwid);
+		bdg->alias = strdup(alias);
+		if (bdg->wwid && bdg->alias &&
+		    vector_insert_slot(bindings, i, bdg))
+			return BINDING_ADDED;
+		else
+			_free_binding(bdg);
+	}
+
+	return BINDING_ERROR;
+}
+
+static int write_bindings_file(const Bindings *bindings, int fd)
+{
+	struct binding *bnd;
+	char line[LINE_MAX];
+	int i;
+
+	if (write(fd, BINDINGS_FILE_HEADER, sizeof(BINDINGS_FILE_HEADER) - 1)
+	    != sizeof(BINDINGS_FILE_HEADER) - 1)
+		return -1;
+
+	vector_foreach_slot(bindings, bnd, i) {
+		int len;
+
+		len = snprintf(line, sizeof(line), "%s %s\n",
+			       bnd->alias, bnd->wwid);
+
+		if (len < 0 || (size_t)len >= sizeof(line)) {
+			condlog(1, "%s: line overflow", __func__);
+			return -1;
+		}
+
+		if (write(fd, line, len) != len)
+			return -1;
+	}
+	return 0;
+}
+
+static int fix_bindings_file(const struct config *conf,
+			     const Bindings *bindings)
+{
+	int rc;
+	long fd;
+	char tempname[PATH_MAX];
+
+	if (safe_sprintf(tempname, "%s.XXXXXX", conf->bindings_file))
+		return -1;
+	if ((fd = mkstemp(tempname)) == -1) {
+		condlog(1, "%s: mkstemp: %m", __func__);
+		return -1;
+	}
+	pthread_cleanup_push(close_fd, (void*)fd);
+	rc = write_bindings_file(bindings, fd);
+	pthread_cleanup_pop(1);
+	if (rc == -1) {
+		condlog(1, "failed to write new bindings file %s",
+			tempname);
+		unlink(tempname);
+		return rc;
+	}
+	if ((rc = rename(tempname, conf->bindings_file)) == -1)
+		condlog(0, "%s: rename: %m", __func__);
+	else
+		condlog(1, "updated bindings file %s", conf->bindings_file);
+	return rc;
+}
+
+static int _check_bindings_file(const struct config *conf, FILE *file,
+				 Bindings *bindings)
+{
+	int rc = 0;
+	unsigned int linenr = 0;
+	char *line = NULL;
+	size_t line_len = 0;
+	ssize_t n;
+
+	pthread_cleanup_push(free, line);
+	while ((n = getline(&line, &line_len, file)) >= 0) {
+		char *c, *alias, *wwid;
+		const char *mpe_wwid;
+
+		linenr++;
+		c = strpbrk(line, "#\n\r");
+		if (c)
+			*c = '\0';
+		alias = strtok(line, " \t");
+		if (!alias) /* blank line */
+			continue;
+		wwid = strtok(NULL, " \t");
+		if (!wwid) {
+			condlog(1, "invalid line %d in bindings file, missing WWID",
+				linenr);
+			continue;
+		}
+		c = strtok(NULL, " \t");
+		if (c)
+			/* This is non-fatal */
+			condlog(1, "invalid line %d in bindings file, extra args \"%s\"",
+				linenr, c);
+
+		mpe_wwid = get_mpe_wwid(conf->mptable, alias);
+		if (mpe_wwid && strcmp(mpe_wwid, wwid)) {
+			condlog(0, "ERROR: alias \"%s\" for WWID %s in bindings file "
+				"on line %u conflicts with multipath.conf entry for %s",
+				alias, wwid, linenr, mpe_wwid);
+			rc = -1;
+			continue;
+		}
+
+		switch (add_binding(bindings, alias, wwid)) {
+		case BINDING_CONFLICT:
+			condlog(0, "ERROR: multiple bindings for alias \"%s\" in "
+				"bindings file on line %u, discarding binding to WWID %s",
+				alias, linenr, wwid);
+			rc = -1;
+			break;
+		case BINDING_EXISTS:
+			condlog(2, "duplicate line for alias %s in bindings file on line %u",
+				alias, linenr);
+			break;
+		case BINDING_ERROR:
+			condlog(2, "error adding binding %s -> %s",
+				alias, wwid);
+			break;
+		default:
+			break;
+		}
+	}
+	pthread_cleanup_pop(1);
+	return rc;
+}
+
+static void cleanup_fclose(void *p)
+{
+	fclose(p);
+}
+
+/*
+ * check_alias_settings(): test for inconsistent alias configuration
+ *
+ * It's a fatal configuration error if the same alias is assigned to
+ * multiple WWIDs. In the worst case, it can cause data corruption
+ * by mangling devices with different WWIDs into the same multipath map.
+ * This function tests the configuration from multipath.conf and the
+ * bindings file for consistency, drops inconsistent multipath.conf
+ * alias settings, and rewrites the bindings file if necessary, dropping
+ * conflicting lines (if user_friendly_names is on, multipathd will
+ * fill in the deleted lines with a newly generated alias later).
+ * Note that multipath.conf is not rewritten. Use "multipath -T" for that.
+ *
+ * Returns: 0 in case of success, -1 if the configuration was bad
+ * and couldn't be fixed.
+ */
+int check_alias_settings(const struct config *conf)
+{
+	int can_write;
+	int rc = 0, i, fd;
+	Bindings bindings = {.allocated = 0, };
+	struct mpentry *mpe;
+
+	pthread_cleanup_push_cast(free_bindings, &bindings);
+	vector_foreach_slot(conf->mptable, mpe, i) {
+		if (!mpe->wwid || !mpe->alias)
+			continue;
+		if (add_binding(&bindings, mpe->alias, mpe->wwid) ==
+		    BINDING_CONFLICT) {
+			condlog(0, "ERROR: alias \"%s\" bound to multiple wwids in multipath.conf, "
+				"discarding binding to %s",
+				mpe->alias, mpe->wwid);
+			free(mpe->alias);
+			mpe->alias = NULL;
+		}
+	}
+	/* This clears the bindings */
+	pthread_cleanup_pop(1);
+
+	pthread_cleanup_push_cast(free_bindings, &bindings);
+	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
+	if (fd != -1) {
+		FILE *file = fdopen(fd, "r");
+
+		if (file != NULL) {
+			pthread_cleanup_push(cleanup_fclose, file);
+			rc = _check_bindings_file(conf, file, &bindings);
+			pthread_cleanup_pop(1);
+			if (rc == -1 && can_write && !conf->bindings_read_only)
+				rc = fix_bindings_file(conf, &bindings);
+			else if (rc == -1)
+				condlog(0, "ERROR: bad settings in read-only bindings file %s",
+					conf->bindings_file);
+		} else {
+			condlog(1, "failed to fdopen %s: %m",
+				conf->bindings_file);
+			close(fd);
+		}
+	}
+	pthread_cleanup_pop(1);
+	return rc;
+}
diff --git a/libmultipath/alias.h b/libmultipath/alias.h
index 236b3ba..dbc950c 100644
--- a/libmultipath/alias.h
+++ b/libmultipath/alias.h
@@ -10,4 +10,7 @@ char *use_existing_alias (const char *wwid, const char *file,
 			  const char *alias_old,
 			  const char *prefix, int bindings_read_only);
 
+struct config;
+int check_alias_settings(const struct config *);
+
 #endif /* _ALIAS_H */
diff --git a/multipath/main.c b/multipath/main.c
index 9e65070..80bc4b5 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -64,6 +64,7 @@
 #include "time-util.h"
 #include "file.h"
 #include "valid.h"
+#include "alias.h"
 
 int logsink;
 struct udev *udev;
@@ -958,6 +959,8 @@ main (int argc, char *argv[])
 		exit(RTVL_FAIL);
 	}
 
+	check_alias_settings(conf);
+
 	if (optind < argc) {
 		dev = MALLOC(FILE_NAME_SIZE);
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 343ee95..9f12a57 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -63,6 +63,7 @@
 #include "uevent.h"
 #include "log.h"
 #include "uxsock.h"
+#include "alias.h"
 
 #include "mpath_cmd.h"
 #include "mpath_persist.h"
@@ -2717,6 +2718,8 @@ reconfigure (struct vectors * vecs)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
+	check_alias_settings(conf);
+
 	uxsock_timeout = conf->uxsock_timeout;
 
 	old = rcu_dereference(multipath_conf);
-- 
2.28.0

[PATCH v3 85/87] libmultipath: alias.c: use strtok_r() instead of strtok()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... for thread-safety.

Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index df44bdc..de28f25 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -141,14 +141,14 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
 	rewind(f);
 	while (fgets(buf, LINE_MAX, f)) {
 		const char *alias, *wwid;
-		char *c;
+		char *c, *saveptr;
 		int curr_id;
 
 		line_nr++;
 		c = strpbrk(buf, "#\n\r");
 		if (c)
 			*c = '\0';
-		alias = strtok(buf, " \t");
+		alias = strtok_r(buf, " \t", &saveptr);
 		if (!alias) /* blank line */
 			continue;
 		curr_id = scan_devname(alias, prefix);
@@ -164,7 +164,7 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
 			biggest_id = curr_id;
 		if (curr_id > id && curr_id < smallest_bigger_id)
 			smallest_bigger_id = curr_id;
-		wwid = strtok(NULL, " \t");
+		wwid = strtok_r(NULL, " \t", &saveptr);
 		if (!wwid){
 			condlog(3,
 				"Ignoring malformed line %u in bindings file",
@@ -206,17 +206,17 @@ rlookup_binding(FILE *f, char *buff, const char *map_alias)
 	buff[0] = '\0';
 
 	while (fgets(line, LINE_MAX, f)) {
-		char *c;
+		char *c, *saveptr;
 		const char *alias, *wwid;
 
 		line_nr++;
 		c = strpbrk(line, "#\n\r");
 		if (c)
 			*c = '\0';
-		alias = strtok(line, " \t");
+		alias = strtok_r(line, " \t", &saveptr);
 		if (!alias) /* blank line */
 			continue;
-		wwid = strtok(NULL, " \t");
+		wwid = strtok_r(NULL, " \t", &saveptr);
 		if (!wwid){
 			condlog(3,
 				"Ignoring malformed line %u in bindings file",
@@ -582,23 +582,23 @@ static int _check_bindings_file(const struct config *conf, FILE *file,
 
 	pthread_cleanup_push(free, line);
 	while ((n = getline(&line, &line_len, file)) >= 0) {
-		char *c, *alias, *wwid;
+		char *c, *alias, *wwid, *saveptr;
 		const char *mpe_wwid;
 
 		linenr++;
 		c = strpbrk(line, "#\n\r");
 		if (c)
 			*c = '\0';
-		alias = strtok(line, " \t");
+		alias = strtok_r(line, " \t", &saveptr);
 		if (!alias) /* blank line */
 			continue;
-		wwid = strtok(NULL, " \t");
+		wwid = strtok_r(NULL, " \t", &saveptr);
 		if (!wwid) {
 			condlog(1, "invalid line %d in bindings file, missing WWID",
 				linenr);
 			continue;
 		}
-		c = strtok(NULL, " \t");
+		c = strtok_r(NULL, " \t", &saveptr);
 		if (c)
 			/* This is non-fatal */
 			condlog(1, "invalid line %d in bindings file, extra args \"%s\"",
-- 
2.28.0

[PATCH v3 86/87] libmultipath: adopt_paths(): set pp->mpp only on success

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Make sure that pp->mpp is only set for paths that have been
successfully added to mpp->paths.

Suggested-by: Benjamin Marzinki <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 2d85df9..cc2dafa 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -248,14 +248,10 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 					pp->dev, mpp->alias);
 				continue;
 			}
-			pp->mpp = mpp;
 			if (pp->initialized == INIT_REMOVED)
 				continue;
-			condlog(3, "%s: ownership set to %s",
-				pp->dev, mpp->alias);
-
 			if (!mpp->paths && !(mpp->paths = vector_alloc()))
-				return 1;
+				goto err;
 
 			conf = get_multipath_config();
 			pthread_cleanup_push(put_multipath_config, conf);
@@ -270,10 +266,17 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 
 			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
 			    store_path(mpp->paths, pp))
-					return 1;
+				goto err;
+
+			pp->mpp = mpp;
+			condlog(3, "%s: ownership set to %s",
+				pp->dev, mpp->alias);
 		}
 	}
 	return 0;
+err:
+	condlog(1, "error setting ownership of %s to %s", pp->dev, mpp->alias);
+	return 1;
 }
 
 void orphan_path(struct path *pp, const char *reason)
-- 
2.28.0

[PATCH v3 87/87] libmultipath: fix a -Wformat-truncation warning from gcc 10

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This fixes a warning seen with gcc 10 on x86 (32 bit).
Fix it by checking the snprintf() return value.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index 207e63c..1512424 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -250,9 +250,9 @@ int systemd_service_enabled_in(const char *dev, const char *prefix)
 		p = d->d_name + strlen(d->d_name) - 6;
 		if (strcmp(p, ".wants"))
 			continue;
-		snprintf(file, sizeof(file), "%s/%s/%s",
-			 path, d->d_name, service);
-		if (stat(file, &stbuf) == 0) {
+		if (!safe_sprintf(file, "%s/%s/%s",
+				  path, d->d_name, service)
+		    && stat(file, &stbuf) == 0) {
 			condlog(3, "%s: found %s", dev, file);
 			found++;
 			break;
-- 
2.28.0

Re: [PATCH v3 84/87] libmultipath: add consistency check for alias settings

[Benjamin Marzinski]

On Wed, Aug 19, 2020 at 03:18:16PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> A typo in a config file, assigning the same alias to multiple WWIDs,
> can cause massive confusion and even data corruption. Check and
> if possible fix the bindings file in such cases.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c | 265 +++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/alias.h |   3 +
>  multipath/main.c     |   3 +
>  multipathd/main.c    |   3 +
>  4 files changed, 274 insertions(+)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 0759c4e..df44bdc 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -4,6 +4,7 @@
>   */
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
>  #include <limits.h>
> @@ -17,6 +18,9 @@
>  #include "vector.h"
>  #include "checkers.h"
>  #include "structs.h"
> +#include "config.h"
> +#include "util.h"
> +#include "errno.h"
>  
>  
>  /*
> @@ -438,3 +442,264 @@ get_user_friendly_wwid(const char *alias, char *buff, const char *file)
>  	fclose(f);
>  	return 0;
>  }
> +
> +struct binding {
> +	char *alias;
> +	char *wwid;
> +};
> +
> +static void _free_binding(struct binding *bdg)
> +{
> +	free(bdg->wwid);
> +	free(bdg->alias);
> +	free(bdg);
> +}
> +
> +/*
> + * Perhaps one day we'll implement this more efficiently, thus use
> + * an abstract type.
> + */
> +typedef struct _vector Bindings;
> +
> +static void free_bindings(Bindings *bindings)
> +{
> +	struct binding *bdg;
> +	int i;
> +
> +	vector_foreach_slot(bindings, bdg, i)
> +		_free_binding(bdg);
> +	vector_reset(bindings);
> +}
> +
> +enum {
> +	BINDING_EXISTS,
> +	BINDING_CONFLICT,
> +	BINDING_ADDED,
> +	BINDING_DELETED,
> +	BINDING_NOTFOUND,
> +	BINDING_ERROR,
> +};
> +
> +static int add_binding(Bindings *bindings, const char *alias, const char *wwid)
> +{
> +	struct binding *bdg;
> +	int i, cmp = 0;
> +
> +	/*
> +	 * Keep the bindings array sorted by alias.
> +	 * Optimization: Search backwards, assuming that the bindings file is
> +	 * sorted already.
> +	 */
> +	vector_foreach_slot_backwards(bindings, bdg, i) {
> +		if ((cmp = strcmp(bdg->alias, alias)) <= 0)
> +			break;
> +	}
> +
> +	/* Check for exact match */
> +	if (i >= 0 && cmp == 0)
> +		return strcmp(bdg->wwid, wwid) ?
> +			BINDING_CONFLICT : BINDING_EXISTS;
> +
> +	i++;
> +	bdg = calloc(1, sizeof(*bdg));
> +	if (bdg) {
> +		bdg->wwid = strdup(wwid);
> +		bdg->alias = strdup(alias);
> +		if (bdg->wwid && bdg->alias &&
> +		    vector_insert_slot(bindings, i, bdg))
> +			return BINDING_ADDED;
> +		else
> +			_free_binding(bdg);
> +	}
> +
> +	return BINDING_ERROR;
> +}
> +
> +static int write_bindings_file(const Bindings *bindings, int fd)
> +{
> +	struct binding *bnd;
> +	char line[LINE_MAX];
> +	int i;
> +
> +	if (write(fd, BINDINGS_FILE_HEADER, sizeof(BINDINGS_FILE_HEADER) - 1)
> +	    != sizeof(BINDINGS_FILE_HEADER) - 1)
> +		return -1;
> +
> +	vector_foreach_slot(bindings, bnd, i) {
> +		int len;
> +
> +		len = snprintf(line, sizeof(line), "%s %s\n",
> +			       bnd->alias, bnd->wwid);
> +
> +		if (len < 0 || (size_t)len >= sizeof(line)) {
> +			condlog(1, "%s: line overflow", __func__);
> +			return -1;
> +		}
> +
> +		if (write(fd, line, len) != len)
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +static int fix_bindings_file(const struct config *conf,
> +			     const Bindings *bindings)
> +{
> +	int rc;
> +	long fd;
> +	char tempname[PATH_MAX];
> +
> +	if (safe_sprintf(tempname, "%s.XXXXXX", conf->bindings_file))
> +		return -1;
> +	if ((fd = mkstemp(tempname)) == -1) {
> +		condlog(1, "%s: mkstemp: %m", __func__);
> +		return -1;
> +	}
> +	pthread_cleanup_push(close_fd, (void*)fd);
> +	rc = write_bindings_file(bindings, fd);
> +	pthread_cleanup_pop(1);
> +	if (rc == -1) {
> +		condlog(1, "failed to write new bindings file %s",
> +			tempname);
> +		unlink(tempname);
> +		return rc;
> +	}
> +	if ((rc = rename(tempname, conf->bindings_file)) == -1)
> +		condlog(0, "%s: rename: %m", __func__);
> +	else
> +		condlog(1, "updated bindings file %s", conf->bindings_file);
> +	return rc;
> +}
> +
> +static int _check_bindings_file(const struct config *conf, FILE *file,
> +				 Bindings *bindings)
> +{
> +	int rc = 0;
> +	unsigned int linenr = 0;
> +	char *line = NULL;
> +	size_t line_len = 0;
> +	ssize_t n;
> +
> +	pthread_cleanup_push(free, line);
> +	while ((n = getline(&line, &line_len, file)) >= 0) {
> +		char *c, *alias, *wwid;
> +		const char *mpe_wwid;
> +
> +		linenr++;
> +		c = strpbrk(line, "#\n\r");
> +		if (c)
> +			*c = '\0';
> +		alias = strtok(line, " \t");
> +		if (!alias) /* blank line */
> +			continue;
> +		wwid = strtok(NULL, " \t");
> +		if (!wwid) {
> +			condlog(1, "invalid line %d in bindings file, missing WWID",
> +				linenr);
> +			continue;
> +		}
> +		c = strtok(NULL, " \t");
> +		if (c)
> +			/* This is non-fatal */
> +			condlog(1, "invalid line %d in bindings file, extra args \"%s\"",
> +				linenr, c);
> +
> +		mpe_wwid = get_mpe_wwid(conf->mptable, alias);
> +		if (mpe_wwid && strcmp(mpe_wwid, wwid)) {
> +			condlog(0, "ERROR: alias \"%s\" for WWID %s in bindings file "
> +				"on line %u conflicts with multipath.conf entry for %s",
> +				alias, wwid, linenr, mpe_wwid);
> +			rc = -1;
> +			continue;
> +		}
> +
> +		switch (add_binding(bindings, alias, wwid)) {
> +		case BINDING_CONFLICT:
> +			condlog(0, "ERROR: multiple bindings for alias \"%s\" in "
> +				"bindings file on line %u, discarding binding to WWID %s",
> +				alias, linenr, wwid);
> +			rc = -1;
> +			break;
> +		case BINDING_EXISTS:
> +			condlog(2, "duplicate line for alias %s in bindings file on line %u",
> +				alias, linenr);
> +			break;
> +		case BINDING_ERROR:
> +			condlog(2, "error adding binding %s -> %s",
> +				alias, wwid);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +	pthread_cleanup_pop(1);
> +	return rc;
> +}
> +
> +static void cleanup_fclose(void *p)
> +{
> +	fclose(p);
> +}
> +
> +/*
> + * check_alias_settings(): test for inconsistent alias configuration
> + *
> + * It's a fatal configuration error if the same alias is assigned to
> + * multiple WWIDs. In the worst case, it can cause data corruption
> + * by mangling devices with different WWIDs into the same multipath map.
> + * This function tests the configuration from multipath.conf and the
> + * bindings file for consistency, drops inconsistent multipath.conf
> + * alias settings, and rewrites the bindings file if necessary, dropping
> + * conflicting lines (if user_friendly_names is on, multipathd will
> + * fill in the deleted lines with a newly generated alias later).
> + * Note that multipath.conf is not rewritten. Use "multipath -T" for that.
> + *
> + * Returns: 0 in case of success, -1 if the configuration was bad
> + * and couldn't be fixed.
> + */
> +int check_alias_settings(const struct config *conf)
> +{
> +	int can_write;
> +	int rc = 0, i, fd;
> +	Bindings bindings = {.allocated = 0, };
> +	struct mpentry *mpe;
> +
> +	pthread_cleanup_push_cast(free_bindings, &bindings);
> +	vector_foreach_slot(conf->mptable, mpe, i) {
> +		if (!mpe->wwid || !mpe->alias)
> +			continue;
> +		if (add_binding(&bindings, mpe->alias, mpe->wwid) ==
> +		    BINDING_CONFLICT) {
> +			condlog(0, "ERROR: alias \"%s\" bound to multiple wwids in multipath.conf, "
> +				"discarding binding to %s",
> +				mpe->alias, mpe->wwid);
> +			free(mpe->alias);
> +			mpe->alias = NULL;
> +		}
> +	}
> +	/* This clears the bindings */
> +	pthread_cleanup_pop(1);
> +
> +	pthread_cleanup_push_cast(free_bindings, &bindings);
> +	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
> +	if (fd != -1) {
> +		FILE *file = fdopen(fd, "r");
> +
> +		if (file != NULL) {
> +			pthread_cleanup_push(cleanup_fclose, file);
> +			rc = _check_bindings_file(conf, file, &bindings);
> +			pthread_cleanup_pop(1);
> +			if (rc == -1 && can_write && !conf->bindings_read_only)
> +				rc = fix_bindings_file(conf, &bindings);
> +			else if (rc == -1)
> +				condlog(0, "ERROR: bad settings in read-only bindings file %s",
> +					conf->bindings_file);
> +		} else {
> +			condlog(1, "failed to fdopen %s: %m",
> +				conf->bindings_file);
> +			close(fd);
> +		}
> +	}
> +	pthread_cleanup_pop(1);
> +	return rc;
> +}
> diff --git a/libmultipath/alias.h b/libmultipath/alias.h
> index 236b3ba..dbc950c 100644
> --- a/libmultipath/alias.h
> +++ b/libmultipath/alias.h
> @@ -10,4 +10,7 @@ char *use_existing_alias (const char *wwid, const char *file,
>  			  const char *alias_old,
>  			  const char *prefix, int bindings_read_only);
>  
> +struct config;
> +int check_alias_settings(const struct config *);
> +
>  #endif /* _ALIAS_H */
> diff --git a/multipath/main.c b/multipath/main.c
> index 9e65070..80bc4b5 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -64,6 +64,7 @@
>  #include "time-util.h"
>  #include "file.h"
>  #include "valid.h"
> +#include "alias.h"
>  
>  int logsink;
>  struct udev *udev;
> @@ -958,6 +959,8 @@ main (int argc, char *argv[])
>  		exit(RTVL_FAIL);
>  	}
>  
> +	check_alias_settings(conf);
> +
>  	if (optind < argc) {
>  		dev = MALLOC(FILE_NAME_SIZE);
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 343ee95..9f12a57 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -63,6 +63,7 @@
>  #include "uevent.h"
>  #include "log.h"
>  #include "uxsock.h"
> +#include "alias.h"
>  
>  #include "mpath_cmd.h"
>  #include "mpath_persist.h"
> @@ -2717,6 +2718,8 @@ reconfigure (struct vectors * vecs)
>  		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
> +	check_alias_settings(conf);
> +
>  	uxsock_timeout = conf->uxsock_timeout;
>  
>  	old = rcu_dereference(multipath_conf);
> -- 
> 2.28.0

Re: [PATCH v3 86/87] libmultipath: adopt_paths(): set pp->mpp only on success

[Benjamin Marzinski]

On Wed, Aug 19, 2020 at 03:18:18PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Make sure that pp->mpp is only set for paths that have been
> successfully added to mpp->paths.
> 
> Suggested-by: Benjamin Marzinki <bmarzins@redhat.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 2d85df9..cc2dafa 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -248,14 +248,10 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
>  					pp->dev, mpp->alias);
>  				continue;
>  			}
> -			pp->mpp = mpp;
>  			if (pp->initialized == INIT_REMOVED)
>  				continue;
> -			condlog(3, "%s: ownership set to %s",
> -				pp->dev, mpp->alias);
> -
>  			if (!mpp->paths && !(mpp->paths = vector_alloc()))
> -				return 1;
> +				goto err;
>  
>  			conf = get_multipath_config();
>  			pthread_cleanup_push(put_multipath_config, conf);
> @@ -270,10 +266,17 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
>  
>  			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
>  			    store_path(mpp->paths, pp))
> -					return 1;
> +				goto err;
> +
> +			pp->mpp = mpp;
> +			condlog(3, "%s: ownership set to %s",
> +				pp->dev, mpp->alias);
>  		}
>  	}
>  	return 0;
> +err:
> +	condlog(1, "error setting ownership of %s to %s", pp->dev, mpp->alias);
> +	return 1;
>  }
>  
>  void orphan_path(struct path *pp, const char *reason)
> -- 
> 2.28.0

Re: [PATCH v3 85/87] libmultipath: alias.c: use strtok_r() instead of strtok()

[Benjamin Marzinski]

On Wed, Aug 19, 2020 at 03:18:17PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> ... for thread-safety.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index df44bdc..de28f25 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -141,14 +141,14 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
>  	rewind(f);
>  	while (fgets(buf, LINE_MAX, f)) {
>  		const char *alias, *wwid;
> -		char *c;
> +		char *c, *saveptr;
>  		int curr_id;
>  
>  		line_nr++;
>  		c = strpbrk(buf, "#\n\r");
>  		if (c)
>  			*c = '\0';
> -		alias = strtok(buf, " \t");
> +		alias = strtok_r(buf, " \t", &saveptr);
>  		if (!alias) /* blank line */
>  			continue;
>  		curr_id = scan_devname(alias, prefix);
> @@ -164,7 +164,7 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
>  			biggest_id = curr_id;
>  		if (curr_id > id && curr_id < smallest_bigger_id)
>  			smallest_bigger_id = curr_id;
> -		wwid = strtok(NULL, " \t");
> +		wwid = strtok_r(NULL, " \t", &saveptr);
>  		if (!wwid){
>  			condlog(3,
>  				"Ignoring malformed line %u in bindings file",
> @@ -206,17 +206,17 @@ rlookup_binding(FILE *f, char *buff, const char *map_alias)
>  	buff[0] = '\0';
>  
>  	while (fgets(line, LINE_MAX, f)) {
> -		char *c;
> +		char *c, *saveptr;
>  		const char *alias, *wwid;
>  
>  		line_nr++;
>  		c = strpbrk(line, "#\n\r");
>  		if (c)
>  			*c = '\0';
> -		alias = strtok(line, " \t");
> +		alias = strtok_r(line, " \t", &saveptr);
>  		if (!alias) /* blank line */
>  			continue;
> -		wwid = strtok(NULL, " \t");
> +		wwid = strtok_r(NULL, " \t", &saveptr);
>  		if (!wwid){
>  			condlog(3,
>  				"Ignoring malformed line %u in bindings file",
> @@ -582,23 +582,23 @@ static int _check_bindings_file(const struct config *conf, FILE *file,
>  
>  	pthread_cleanup_push(free, line);
>  	while ((n = getline(&line, &line_len, file)) >= 0) {
> -		char *c, *alias, *wwid;
> +		char *c, *alias, *wwid, *saveptr;
>  		const char *mpe_wwid;
>  
>  		linenr++;
>  		c = strpbrk(line, "#\n\r");
>  		if (c)
>  			*c = '\0';
> -		alias = strtok(line, " \t");
> +		alias = strtok_r(line, " \t", &saveptr);
>  		if (!alias) /* blank line */
>  			continue;
> -		wwid = strtok(NULL, " \t");
> +		wwid = strtok_r(NULL, " \t", &saveptr);
>  		if (!wwid) {
>  			condlog(1, "invalid line %d in bindings file, missing WWID",
>  				linenr);
>  			continue;
>  		}
> -		c = strtok(NULL, " \t");
> +		c = strtok_r(NULL, " \t", &saveptr);
>  		if (c)
>  			/* This is non-fatal */
>  			condlog(1, "invalid line %d in bindings file, extra args \"%s\"",
> -- 
> 2.28.0

Re: [PATCH v3 87/87] libmultipath: fix a -Wformat-truncation warning from gcc 10

[Benjamin Marzinski]

On Wed, Aug 19, 2020 at 03:18:19PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This fixes a warning seen with gcc 10 on x86 (32 bit).
> Fix it by checking the snprintf() return value.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/util.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index 207e63c..1512424 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -250,9 +250,9 @@ int systemd_service_enabled_in(const char *dev, const char *prefix)
>  		p = d->d_name + strlen(d->d_name) - 6;
>  		if (strcmp(p, ".wants"))
>  			continue;
> -		snprintf(file, sizeof(file), "%s/%s/%s",
> -			 path, d->d_name, service);
> -		if (stat(file, &stbuf) == 0) {
> +		if (!safe_sprintf(file, "%s/%s/%s",
> +				  path, d->d_name, service)
> +		    && stat(file, &stbuf) == 0) {
>  			condlog(3, "%s: found %s", dev, file);
>  			found++;
>  			break;
> -- 
> 2.28.0

Re: [PATCH v3 87/87] libmultipath: fix a -Wformat-truncation warning from gcc 10

[Martin Wilck]

On Wed, 2020-08-19 at 17:40 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 19, 2020 at 03:18:19PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > This fixes a warning seen with gcc 10 on x86 (32 bit).
> > Fix it by checking the snprintf() return value.
> > 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> > Signed-off-by: Martin Wilck <mwilck@suse.com>

Thanks a lot, Ben!

This was the last Reviewed-by: tag missing on this series.
I've pushed it to github.com/openSUSE/multipath-tools (upstream-queue)
now.

Regards
Martin