[PATCH v2 00/84] multipath-tools series part VII: addional patches

[PATCH v2 00/84] multipath-tools series part VII: addional patches

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is part VI 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-2008
This part is tagged "submit-200812-7".

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

84/84, 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.

Martin Wilck (3):
  multipath: check_path_valid(): eliminate some failure modes
  libmultipath: alias: static const variable for BINDINGS_FILE_HEADER
  libmultipath: add consistency check for alias settings

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

 libmultipath/alias.c    | 274 +++++++++++++++++++++++++++++++++++++++-
 libmultipath/alias.h    |  15 +--
 libmultipath/dmparser.c |  12 +-
 multipath/main.c        |  21 +--
 multipathd/main.c       |   3 +
 5 files changed, 301 insertions(+), 24 deletions(-)

-- 
2.28.0

[PATCH v2 81/84] multipath: check_path_valid(): eliminate some failure modes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The memory allocations can fail, and pathvec is not needed until the
path_discovery() call. Eliminate the failure modes by not setting up
pathvec before it's actually needed.

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

diff --git a/multipath/main.c b/multipath/main.c
index 9d6b482..9e65070 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -637,15 +637,6 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 			 minor(devt));
 	}
 
-	pathvec = vector_alloc();
-	if (!pathvec)
-		goto fail;
-
-	if (store_path(pathvec, pp) != 0) {
-		free_path(pp);
-		goto fail;
-	}
-
 	if ((r == PATH_IS_VALID || r == PATH_IS_MAYBE_VALID) &&
 	    released_to_systemd())
 		r = PATH_IS_NOT_VALID;
@@ -684,6 +675,15 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 		goto out;
 	}
 
+	pathvec = vector_alloc();
+	if (!pathvec)
+		goto fail;
+
+	if (store_path(pathvec, pp) != 0) {
+		free_path(pp);
+		goto fail;
+	}
+
 	/* For find_multipaths = SMART, if there is more than one path
 	 * matching the refwwid, then the path is valid */
 	if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0)
-- 
2.28.0

Re: [PATCH v2 81/84] multipath: check_path_valid(): eliminate some failure modes

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:35:58PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The memory allocations can fail, and pathvec is not needed until the
> path_discovery() call. Eliminate the failure modes by not setting up
> pathvec before it's actually needed.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 9d6b482..9e65070 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -637,15 +637,6 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
>  			 minor(devt));
>  	}
>  
> -	pathvec = vector_alloc();
> -	if (!pathvec)
> -		goto fail;
> -
> -	if (store_path(pathvec, pp) != 0) {
> -		free_path(pp);
> -		goto fail;
> -	}
> -
>  	if ((r == PATH_IS_VALID || r == PATH_IS_MAYBE_VALID) &&
>  	    released_to_systemd())
>  		r = PATH_IS_NOT_VALID;
> @@ -684,6 +675,15 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
>  		goto out;
>  	}
>  
> +	pathvec = vector_alloc();
> +	if (!pathvec)
> +		goto fail;
> +
> +	if (store_path(pathvec, pp) != 0) {
> +		free_path(pp);
> +		goto fail;
> +	}
> +
>  	/* For find_multipaths = SMART, if there is more than one path
>  	 * matching the refwwid, then the path is valid */
>  	if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0)
> -- 
> 2.28.0

[PATCH v2 82/84] libmultipath: free pp if store_path fails in disassemble_map

[mwilck]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

In disassemble_map func, one pp will be allocated and stored in
pgp->paths. However, if store_path fails, pp will not be freed,
then memory leak problem occurs.

Here, we will call free_path to free pp when store_path fails.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dmparser.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 25e6a4a..c103161 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -306,11 +306,15 @@ int disassemble_map(const struct _vector *pathvec,
 					goto out1;
 
 				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
-			}
-			FREE(word);
 
-			if (store_path(pgp->paths, pp))
-				goto out;
+				if (store_path(pgp->paths, pp)) {
+					free_path(pp);
+					goto out1;
+				}
+			} else if (store_path(pgp->paths, pp))
+				goto out1;
+
+			FREE(word);
 
 			pgp->id ^= (long)pp;
 			pp->pgindex = i + 1;
-- 
2.28.0

Re: [PATCH v2 82/84] libmultipath: free pp if store_path fails in disassemble_map

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:35:59PM +0200, mwilck@suse.com wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> In disassemble_map func, one pp will be allocated and stored in
> pgp->paths. However, if store_path fails, pp will not be freed,
> then memory leak problem occurs.
> 
> Here, we will call free_path to free pp when store_path fails.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/dmparser.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 25e6a4a..c103161 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -306,11 +306,15 @@ int disassemble_map(const struct _vector *pathvec,
>  					goto out1;
>  
>  				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
> -			}
> -			FREE(word);
>  
> -			if (store_path(pgp->paths, pp))
> -				goto out;
> +				if (store_path(pgp->paths, pp)) {
> +					free_path(pp);
> +					goto out1;
> +				}
> +			} else if (store_path(pgp->paths, pp))
> +				goto out1;
> +
> +			FREE(word);
>  
>  			pgp->id ^= (long)pp;
>  			pp->pgindex = i + 1;
> -- 
> 2.28.0

[PATCH v2 83/84] libmultipath: alias: static const variable for BINDINGS_FILE_HEADER

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... and fixup the header file.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 17 ++++++++++++++---
 libmultipath/alias.h | 12 ++++--------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 35f1beb..0759c4e 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -37,6 +37,17 @@
  * See the file COPYING included with this distribution for more details.
  */
 
+#define BINDINGS_FILE_HEADER		\
+"# Multipath bindings, Version : 1.0\n" \
+"# NOTE: this file is automatically maintained by the multipath program.\n" \
+"# You should not need to edit this file in normal circumstances.\n" \
+"#\n" \
+"# Format:\n" \
+"# alias wwid\n" \
+"#\n"
+
+static const char bindings_file_header[] = BINDINGS_FILE_HEADER;
+
 int
 valid_alias(const char *alias)
 {
@@ -287,7 +298,7 @@ use_existing_alias (const char *wwid, const char *file, const char *alias_old,
 	char buff[WWID_SIZE];
 	FILE *f;
 
-	fd = open_file(file, &can_write, BINDINGS_FILE_HEADER);
+	fd = open_file(file, &can_write, bindings_file_header);
 	if (fd < 0)
 		return NULL;
 
@@ -361,7 +372,7 @@ get_user_friendly_alias(const char *wwid, const char *file, const char *prefix,
 		return NULL;
 	}
 
-	fd = open_file(file, &can_write, BINDINGS_FILE_HEADER);
+	fd = open_file(file, &can_write, bindings_file_header);
 	if (fd < 0)
 		return NULL;
 
@@ -406,7 +417,7 @@ get_user_friendly_wwid(const char *alias, char *buff, const char *file)
 		return -1;
 	}
 
-	fd = open_file(file, &unused, BINDINGS_FILE_HEADER);
+	fd = open_file(file, &unused, bindings_file_header);
 	if (fd < 0)
 		return -1;
 
diff --git a/libmultipath/alias.h b/libmultipath/alias.h
index 7c4b302..236b3ba 100644
--- a/libmultipath/alias.h
+++ b/libmultipath/alias.h
@@ -1,11 +1,5 @@
-#define BINDINGS_FILE_HEADER \
-"# Multipath bindings, Version : 1.0\n" \
-"# NOTE: this file is automatically maintained by the multipath program.\n" \
-"# You should not need to edit this file in normal circumstances.\n" \
-"#\n" \
-"# Format:\n" \
-"# alias wwid\n" \
-"#\n"
+#ifndef _ALIAS_H
+#define _ALIAS_H
 
 int valid_alias(const char *alias);
 char *get_user_friendly_alias(const char *wwid, const char *file,
@@ -15,3 +9,5 @@ int get_user_friendly_wwid(const char *alias, char *buff, const char *file);
 char *use_existing_alias (const char *wwid, const char *file,
 			  const char *alias_old,
 			  const char *prefix, int bindings_read_only);
+
+#endif /* _ALIAS_H */
-- 
2.28.0

Re: [PATCH v2 83/84] libmultipath: alias: static const variable for BINDINGS_FILE_HEADER

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:36:00PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> ... and fixup the header file.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c | 17 ++++++++++++++---
>  libmultipath/alias.h | 12 ++++--------
>  2 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 35f1beb..0759c4e 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -37,6 +37,17 @@
>   * See the file COPYING included with this distribution for more details.
>   */
>  
> +#define BINDINGS_FILE_HEADER		\
> +"# Multipath bindings, Version : 1.0\n" \
> +"# NOTE: this file is automatically maintained by the multipath program.\n" \
> +"# You should not need to edit this file in normal circumstances.\n" \
> +"#\n" \
> +"# Format:\n" \
> +"# alias wwid\n" \
> +"#\n"
> +
> +static const char bindings_file_header[] = BINDINGS_FILE_HEADER;
> +
>  int
>  valid_alias(const char *alias)
>  {
> @@ -287,7 +298,7 @@ use_existing_alias (const char *wwid, const char *file, const char *alias_old,
>  	char buff[WWID_SIZE];
>  	FILE *f;
>  
> -	fd = open_file(file, &can_write, BINDINGS_FILE_HEADER);
> +	fd = open_file(file, &can_write, bindings_file_header);
>  	if (fd < 0)
>  		return NULL;
>  
> @@ -361,7 +372,7 @@ get_user_friendly_alias(const char *wwid, const char *file, const char *prefix,
>  		return NULL;
>  	}
>  
> -	fd = open_file(file, &can_write, BINDINGS_FILE_HEADER);
> +	fd = open_file(file, &can_write, bindings_file_header);
>  	if (fd < 0)
>  		return NULL;
>  
> @@ -406,7 +417,7 @@ get_user_friendly_wwid(const char *alias, char *buff, const char *file)
>  		return -1;
>  	}
>  
> -	fd = open_file(file, &unused, BINDINGS_FILE_HEADER);
> +	fd = open_file(file, &unused, bindings_file_header);
>  	if (fd < 0)
>  		return -1;
>  
> diff --git a/libmultipath/alias.h b/libmultipath/alias.h
> index 7c4b302..236b3ba 100644
> --- a/libmultipath/alias.h
> +++ b/libmultipath/alias.h
> @@ -1,11 +1,5 @@
> -#define BINDINGS_FILE_HEADER \
> -"# Multipath bindings, Version : 1.0\n" \
> -"# NOTE: this file is automatically maintained by the multipath program.\n" \
> -"# You should not need to edit this file in normal circumstances.\n" \
> -"#\n" \
> -"# Format:\n" \
> -"# alias wwid\n" \
> -"#\n"
> +#ifndef _ALIAS_H
> +#define _ALIAS_H
>  
>  int valid_alias(const char *alias);
>  char *get_user_friendly_alias(const char *wwid, const char *file,
> @@ -15,3 +9,5 @@ int get_user_friendly_wwid(const char *alias, char *buff, const char *file);
>  char *use_existing_alias (const char *wwid, const char *file,
>  			  const char *alias_old,
>  			  const char *prefix, int bindings_read_only);
> +
> +#endif /* _ALIAS_H */
> -- 
> 2.28.0

[PATCH v2 84/84] 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 | 257 +++++++++++++++++++++++++++++++++++++++++++
 libmultipath/alias.h |   3 +
 multipath/main.c     |   3 +
 multipathd/main.c    |   3 +
 4 files changed, 266 insertions(+)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 0759c4e..d328f77 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,256 @@ 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 */
+	vector_foreach_slot(bindings, bdg, i) {
+		if ((cmp = strcmp(bdg->alias, alias)) >= 0)
+			break;
+	}
+
+	/* Check for exact match */
+	if (i < VECTOR_SIZE(bindings) && cmp == 0)
+		return strcmp(bdg->wwid, wwid) ?
+			BINDING_CONFLICT : BINDING_EXISTS;
+
+	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;
+	}
+
+	_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)
+{
+	FILE *file;
+	int can_write;
+	int rc = 0, i;
+	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);
+	file = fdopen(open_file(conf->bindings_file, &can_write,
+				BINDINGS_FILE_HEADER), "r");
+	if (file == NULL) {
+		if (errno != ENOENT)
+			condlog(1, "failed to fdopen %s: %m",
+				conf->bindings_file);
+	} else {
+		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 in bindings file %s",
+				conf->bindings_file);
+	}
+	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 7f95d46..1b89909 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"
@@ -2716,6 +2717,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 v2 84/84] libmultipath: add consistency check for alias settings

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:36:01PM +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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c | 257 +++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/alias.h |   3 +
>  multipath/main.c     |   3 +
>  multipathd/main.c    |   3 +
>  4 files changed, 266 insertions(+)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 0759c4e..d328f77 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,256 @@ 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 */
> +	vector_foreach_slot(bindings, bdg, i) {

I'm not sure that it makes much of a difference, but the bindings file
will usually be in order, so searching forwards will mean that youi will
usually check every item each time.  Search backwards would mean that
you usually only check one item.

> +		if ((cmp = strcmp(bdg->alias, alias)) >= 0)
> +			break;
> +	}
> +
> +	/* Check for exact match */
> +	if (i < VECTOR_SIZE(bindings) && cmp == 0)
> +		return strcmp(bdg->wwid, wwid) ?
> +			BINDING_CONFLICT : BINDING_EXISTS;
> +
> +	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;
> +	}
> +
> +	_free_binding(bdg);
if calloc() returns NULL this will end up dereferencing a NULL pointer.
> +	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");

I realize that your patch is just copying the existing code and I think
our locking will save us here, but strtok isn't thread safe. We should
consider changing all these to strtok_r().

> +		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)
> +{
> +	FILE *file;
> +	int can_write;
> +	int rc = 0, i;
> +	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);
> +	file = fdopen(open_file(conf->bindings_file, &can_write,
> +				BINDINGS_FILE_HEADER), "r");
> +	if (file == NULL) {
> +		if (errno != ENOENT)
> +			condlog(1, "failed to fdopen %s: %m",
> +				conf->bindings_file);

If open_file() succeeds, but fdopen() fails, I don't belive fdopen()
closes the file descriptor, so we need to do that here.

> +	} else {
> +		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 in bindings file %s",
> +				conf->bindings_file);
> +	}
> +	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 7f95d46..1b89909 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"
> @@ -2716,6 +2717,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 v2 84/84] libmultipath: add consistency check for alias settings

[Martin Wilck]

On Mon, 2020-08-17 at 19:30 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 12, 2020 at 01:36:01PM +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.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/alias.c | 257
> > +++++++++++++++++++++++++++++++++++++++++++
> >  libmultipath/alias.h |   3 +
> >  multipath/main.c     |   3 +
> >  multipathd/main.c    |   3 +
> >  4 files changed, 266 insertions(+)
> > 
> > diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> > index 0759c4e..d328f77 100644
> > --- a/libmultipath/alias.c
> > +++ b/libmultipath/alias.c
> > 
> > +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");
> 
> I realize that your patch is just copying the existing code and I
> think
> our locking will save us here, but strtok isn't thread safe. We
> should
> consider changing all these to strtok_r().

I'll change this to strtok_r. I'll send a cumulative patch fixing the
other users as well.

I guess we'll also need to protect access to the bindings file. All
current accesses seem to be protected by the vecs lock (even
check_alias_settings(), called from reconfigure()), but we shouldn't
rely only on that. Also, multipath and multipathd could access the file
in parallel. Let's consider that later.

Btw, the vector holding the bindings in memory could obviously also
be used as a cache for looking up aliases at runtime. That would be
another item for later improvement. Actually, I thought we might
replace the vector here by some sort of hash map for even quicker
lookup. But I wanted to keep the already large patch series within
limits.

I'll send a v3 with your issues fixed.

Thanks,
Martin