[PATCH 00/42] multipath-tools series part III: duplicate alias

[PATCH 00/42] multipath-tools series part III: duplicate alias

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part II of a larger patch series for multpath-tools I've been preparing.
It contains fixes for a customer issue where the same alias was set for
several maps with different WWIDs in the WWIDs file.

It's based on the previously submitted part II.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-2007

There are tags in that repo for each part of the series.
This part is tagged "submit-200709-3".

Regards,
Martin

Martin Wilck (3):
  libmultipath: refuse creating map with duplicate alias
  libmultipath: refuse reloading an existing map with different WWID
  libmultipath: dm_addmap(): refuse creating map with empty WWID

 libmultipath/configure.c   | 24 ++++++++++++++++++++----
 libmultipath/devmapper.c   | 26 +++++++++++++++-----------
 libmultipath/structs_vec.c | 13 +++++++++++++
 libmultipath/structs_vec.h |  1 +
 multipathd/main.c          |  6 +++++-
 5 files changed, 54 insertions(+), 16 deletions(-)

-- 
2.26.2

[PATCH 40/42] libmultipath: refuse creating map with duplicate alias

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If for some reason (e.g. user configuration error) the same alias
is used for different WWIDs, multipathd starts behaving very weirdly.
Sooner or later it "fixes" pp->wwid of some paths to match those of
the other WWID, and may eventually coalesce paths with different WWIDs
into one map, causing data corruption.

Fix this by refusing to add a map with an already existing alias.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   |  5 +++++
 libmultipath/structs_vec.c | 13 +++++++++++++
 libmultipath/structs_vec.h |  1 +
 multipathd/main.c          |  6 +++++-
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 315eb6a..c62807e 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1187,6 +1187,11 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 		}
 		verify_paths(mpp, vecs);
 
+		if (does_alias_exist(newmp, mpp)) {
+			remove_map(mpp, vecs, PURGE_VEC);
+			continue;
+		}
+
 		params[0] = '\0';
 		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
 			remove_map(mpp, vecs, 0);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8137ea2..c7dffb7 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -424,6 +424,19 @@ find_existing_alias (struct multipath * mpp,
 		}
 }
 
+bool does_alias_exist(const struct _vector *mpvec, const struct multipath *mpp)
+{
+	const struct multipath *other;
+
+	other = find_mp_by_alias(mpvec, mpp->alias);
+	if (other == NULL || !strcmp(other->wwid, mpp->wwid))
+		return false;
+	condlog(0, "%s: alias \"%s\" already present with WWID %s, skipping",
+		mpp->wwid, mpp->alias, other->wwid);
+	condlog(0, "please check alias settings in config and bindings file");
+	return true;
+}
+
 struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
 				    int add_vec)
 {
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 2a5e3d6..4b3b8b7 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -33,6 +33,7 @@ void remove_map_by_alias(const char *alias, struct vectors * vecs,
 void remove_maps (struct vectors * vecs);
 
 void sync_map_state (struct multipath *);
+bool does_alias_exist(const struct _vector *mpvec, const struct multipath *mpp);
 struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
 void update_queue_mode_del_path(struct multipath *mpp);
diff --git a/multipathd/main.c b/multipathd/main.c
index 40c050b..cd0e29b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -966,8 +966,12 @@ rescan:
 			orphan_path(pp, "only one path");
 			return 0;
 		}
-		condlog(4,"%s: creating new map", pp->dev);
 		if ((mpp = add_map_with_path(vecs, pp, 1))) {
+			if (does_alias_exist(vecs->mpvec, mpp)) {
+				remove_map(mpp, vecs, PURGE_VEC);
+				return 0;
+			}
+			condlog(4,"%s: creating new map", pp->dev);
 			mpp->action = ACT_CREATE;
 			/*
 			 * We don't depend on ACT_CREATE, as domap will
-- 
2.26.2

[PATCH 41/42] libmultipath: refuse reloading an existing map with different WWID

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If a map with given name (alias) already exists with a different WWID,
reloading it with a new WWID will be harmful. The existing paths would
be replaced by other, unrelated ones. The WWIDs of the new paths would
not match the map WWID, and thus sooner or later overwritten by
disassemble_map with the map's wwid.

Refuse reloading a map under such circumstances. This makes it impossible
to "swap" multipath names in a single reconfigure run, but avoiding
data corruption should be worth it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index c62807e..75e11fd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -897,10 +897,21 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		return DOMAP_DRY;
 	}
 
-	if (mpp->action == ACT_CREATE &&
-	    dm_map_present(mpp->alias)) {
-		condlog(3, "%s: map already present", mpp->alias);
-		mpp->action = ACT_RELOAD;
+	if (mpp->action == ACT_CREATE && dm_map_present(mpp->alias)) {
+		char wwid[WWID_SIZE];
+
+		if (dm_get_uuid(mpp->alias, wwid, sizeof(wwid)) == 0) {
+			if (!strncmp(mpp->wwid, wwid, sizeof(wwid))) {
+				condlog(3, "%s: map already present",
+					mpp->alias);
+				mpp->action = ACT_RELOAD;
+			} else {
+				condlog(0, "%s: map \"%s\" already present with WWID %s, skipping",
+					mpp->wwid, mpp->alias, wwid);
+				condlog(0, "please check alias settings in config and bindings file");
+				mpp->action = ACT_REJECT;
+			}
+		}
 	}
 
 	switch (mpp->action) {
-- 
2.26.2

[PATCH 42/42] libmultipath: dm_addmap(): refuse creating map with empty WWID

[mwilck]

From: Martin Wilck <mwilck@suse.com>

We already avoid creating maps with empty WWID in coalesce_paths()
as well as in ev_add_path(). The only code path where it's difficult
to prove (although extremely unlikely) that we can't call
dm_addmap(ACT_CREATE) with an empty WWID is update_path_groups()->
reload_map(). To make the code easier to review and avoid ugly
corner cases, simply refuse to create maps with a zero-length
WWID.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index a177a54..fb7675c 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -352,6 +352,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	char *prefixed_uuid = NULL;
 	uint32_t cookie = 0;
 
+	if (task == DM_DEVICE_CREATE && strlen(mpp->wwid) == 0) {
+		condlog(1, "%s: refusing to create map with empty WWID",
+			mpp->alias);
+		return 0;
+	}
+
 	/* Need to add this here to allow 0 to be passed in udev_flags */
 	udev_flags |= DM_UDEV_DISABLE_LIBRARY_FALLBACK;
 
@@ -368,18 +374,16 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 		dm_task_set_ro(dmt);
 
 	if (task == DM_DEVICE_CREATE) {
-		if (strlen(mpp->wwid) > 0) {
-			prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
-					       strlen(mpp->wwid) + 1);
-			if (!prefixed_uuid) {
-				condlog(0, "cannot create prefixed uuid : %s",
-					strerror(errno));
-				goto addout;
-			}
-			sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
-			if (!dm_task_set_uuid(dmt, prefixed_uuid))
-				goto freeout;
+		prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
+				       strlen(mpp->wwid) + 1);
+		if (!prefixed_uuid) {
+			condlog(0, "cannot create prefixed uuid : %s",
+				strerror(errno));
+			goto addout;
 		}
+		sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
+		if (!dm_task_set_uuid(dmt, prefixed_uuid))
+			goto freeout;
 		dm_task_skip_lockfs(dmt);
 #ifdef LIBDM_API_FLUSH
 		dm_task_no_flush(dmt);
-- 
2.26.2

Re: [PATCH 00/42] multipath-tools series part III: duplicate alias

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:35:10PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> This is part II of a larger patch series for multpath-tools I've been preparing.
> It contains fixes for a customer issue where the same alias was set for
> several maps with different WWIDs in the WWIDs file.
> 
> It's based on the previously submitted part II.
> 
> The full series will also be available here:
> https://github.com/mwilck/multipath-tools/tree/ups/submit-2007
> 
> There are tags in that repo for each part of the series.
> This part is tagged "submit-200709-3".

With the first two of these patches applied , multipathd fails a
real-world situation in different, arguably worse, way than it currently
does, and I think it could do better.  If user_friendly_names is set,
but two (or more) devices weren't in the initramfs bindings file, they
will get random aliases during boot.  Assuming the devices are in the
regular filesystem bindings file, it's not super uncommon for these
devices to pick from the same alias as they use in the regular bindings
file, but with different wwids matching to different aliases.

Without these patches, multipath will rename them if possible, but if
not, they will still exist, but with the wrong alias. Existing with the
wrong alias isn't great, since things could be checking for the devices
by name, which could cause corruption. But in reality, usually they are
referenced by Labels, which will still work (since most things are
designed to not expect persistent naming of devices).  However, with
this patches, some of the devices will be deleted, which avoids the
possiblity of corruption, but in practice usually is worse because
referencing devices by label already avoids the corruption problem. 

A better idea might simply be to fallback to using the WWID as an alias,
on the offending map.  This should avoid corruption, since unless
someone manually set the WWID as an alias in multipath.conf it should be
unique. But it won't cause an existing, and possibly necessary, device
to get deleted.

-Ben
 
> Regards,
> Martin
> 
> Martin Wilck (3):
>   libmultipath: refuse creating map with duplicate alias
>   libmultipath: refuse reloading an existing map with different WWID
>   libmultipath: dm_addmap(): refuse creating map with empty WWID
> 
>  libmultipath/configure.c   | 24 ++++++++++++++++++++----
>  libmultipath/devmapper.c   | 26 +++++++++++++++-----------
>  libmultipath/structs_vec.c | 13 +++++++++++++
>  libmultipath/structs_vec.h |  1 +
>  multipathd/main.c          |  6 +++++-
>  5 files changed, 54 insertions(+), 16 deletions(-)
> 
> -- 
> 2.26.2

Re: [PATCH 42/42] libmultipath: dm_addmap(): refuse creating map with empty WWID

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:35:13PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We already avoid creating maps with empty WWID in coalesce_paths()
> as well as in ev_add_path(). The only code path where it's difficult
> to prove (although extremely unlikely) that we can't call
> dm_addmap(ACT_CREATE) with an empty WWID is update_path_groups()->
> reload_map(). To make the code easier to review and avoid ugly
> corner cases, simply refuse to create maps with a zero-length
> WWID.
> 

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

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/devmapper.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index a177a54..fb7675c 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -352,6 +352,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	char *prefixed_uuid = NULL;
>  	uint32_t cookie = 0;
>  
> +	if (task == DM_DEVICE_CREATE && strlen(mpp->wwid) == 0) {
> +		condlog(1, "%s: refusing to create map with empty WWID",
> +			mpp->alias);
> +		return 0;
> +	}
> +
>  	/* Need to add this here to allow 0 to be passed in udev_flags */
>  	udev_flags |= DM_UDEV_DISABLE_LIBRARY_FALLBACK;
>  
> @@ -368,18 +374,16 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  		dm_task_set_ro(dmt);
>  
>  	if (task == DM_DEVICE_CREATE) {
> -		if (strlen(mpp->wwid) > 0) {
> -			prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
> -					       strlen(mpp->wwid) + 1);
> -			if (!prefixed_uuid) {
> -				condlog(0, "cannot create prefixed uuid : %s",
> -					strerror(errno));
> -				goto addout;
> -			}
> -			sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
> -			if (!dm_task_set_uuid(dmt, prefixed_uuid))
> -				goto freeout;
> +		prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
> +				       strlen(mpp->wwid) + 1);
> +		if (!prefixed_uuid) {
> +			condlog(0, "cannot create prefixed uuid : %s",
> +				strerror(errno));
> +			goto addout;
>  		}
> +		sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
> +		if (!dm_task_set_uuid(dmt, prefixed_uuid))
> +			goto freeout;
>  		dm_task_skip_lockfs(dmt);
>  #ifdef LIBDM_API_FLUSH
>  		dm_task_no_flush(dmt);
> -- 
> 2.26.2

Re: [PATCH 00/42] multipath-tools series part III: duplicate alias

[Martin Wilck]

On Mon, 2020-07-20 at 15:30 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:35:10PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Hi Christophe, hi Ben,
> > 
> > This is part II of a larger patch series for multpath-tools I've
> > been preparing.
> > It contains fixes for a customer issue where the same alias was set
> > for
> > several maps with different WWIDs in the WWIDs file.
> > 
> > It's based on the previously submitted part II.
> > 
> > The full series will also be available here:
> > https://github.com/mwilck/multipath-tools/tree/ups/submit-2007
> > 
> > There are tags in that repo for each part of the series.
> > This part is tagged "submit-200709-3".
> 
> With the first two of these patches applied , multipathd fails a
> real-world situation in different, arguably worse, way than it
> currently
> does, and I think it could do better.  If user_friendly_names is set,
> but two (or more) devices weren't in the initramfs bindings file,
> they
> will get random aliases during boot.  Assuming the devices are in the
> regular filesystem bindings file, it's not super uncommon for these
> devices to pick from the same alias as they use in the regular
> bindings
> file, but with different wwids matching to different aliases.
> 
> Without these patches, multipath will rename them if possible, but if
> not, they will still exist, but with the wrong alias. Existing with
> the
> wrong alias isn't great, since things could be checking for the
> devices
> by name, which could cause corruption. 

Indeed I didn't think of this scenario.

The problem our customer faced was actually an evil case of data
corruption which must be avoided. But you're right, my attempt to fix
it was wrong.

> But in reality, usually they are
> referenced by Labels, which will still work (since most things are
> designed to not expect persistent naming of devices).  However, with
> this patches, some of the devices will be deleted, which avoids the
> possiblity of corruption, but in practice usually is worse because
> referencing devices by label already avoids the corruption problem. 
> 
> A better idea might simply be to fallback to using the WWID as an
> alias,
> on the offending map.  This should avoid corruption, since unless
> someone manually set the WWID as an alias in multipath.conf it should
> be
> unique. But it won't cause an existing, and possibly necessary,
> device
> to get deleted.

Ok, I'm going to try to come up with a version that doesn't suffer from
the problem you describe. I'll have to ponder about it some more.

Thanks for pointing this out,
Martin