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

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

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part III 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.

Note that this does *not* break the scenario Ben described in his response to
v1 of the part of the series. Details in the commit message of patch 41/42.

This part based on the previously submitted part II (which is completely
unchanged from the v1 submission and not re-posted).

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

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

 * 40/42 "libmultipath: refuse creating map with duplicate alias"
    - dropped, as this breaks valid scenarios
 * 40/42 "libmultipath: free_multipath(): remove mpp references"
    - new patch added, required to avoid segfaults; inserted here to preserve
      numbering
 * 41/42 "libmultipath: refuse reloading an existing map with different WWID"
    - unchanged, but not reviewed yet. Commit message improved.

NOTE: part VI and VII of the the series contain patches related to the topic
of this part, improving the handling of inconsistent alias settings further:

  80/80 "libmultipath: select_action(): don't drop map if alias clashes"
  84/84 "libmultipath: add consistency check for alias settings"

Regards,
Martin

Martin Wilck (3):
  libmultipath: free_multipath(): remove mpp references
  libmultipath: refuse reloading an existing map with different WWID
  libmultipath: dm_addmap(): refuse creating map with empty WWID

 libmultipath/configure.c | 19 +++++++++++++++----
 libmultipath/devmapper.c | 26 +++++++++++++++-----------
 libmultipath/structs.c   | 15 +++++++++++++++
 3 files changed, 45 insertions(+), 15 deletions(-)

-- 
2.28.0

[PATCH v2 40/42] libmultipath: free_multipath(): remove mpp references

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If free_paths is false, make sure no references to the dropped
multipath remain. Otherwise multipathd may crash later when
trying to access these.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index da898b7..d227ec0 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -258,6 +258,21 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
 		mpp->dmi = NULL;
 	}
 
+	if (!free_paths && mpp->pg) {
+		struct pathgroup *pgp;
+		struct path *pp;
+		int i, j;
+
+		/*
+		 * Make sure paths carry no reference to this mpp any more
+		 */
+		vector_foreach_slot(mpp->pg, pgp, i) {
+			vector_foreach_slot(pgp->paths, pp, j)
+				if (pp->mpp == mpp)
+					pp->mpp = NULL;
+		}
+	}
+
 	free_pathvec(mpp->paths, free_paths);
 	free_pgvec(mpp->pg, free_paths);
 	FREE_PTR(mpp->mpcontext);
-- 
2.28.0

Re: [PATCH v2 40/42] libmultipath: free_multipath(): remove mpp references

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:34:04PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If free_paths is false, make sure no references to the dropped
> multipath remain. Otherwise multipathd may crash later when
> trying to access these.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index da898b7..d227ec0 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -258,6 +258,21 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
>  		mpp->dmi = NULL;
>  	}
>  
> +	if (!free_paths && mpp->pg) {
> +		struct pathgroup *pgp;
> +		struct path *pp;
> +		int i, j;
> +
> +		/*
> +		 * Make sure paths carry no reference to this mpp any more
> +		 */
> +		vector_foreach_slot(mpp->pg, pgp, i) {
> +			vector_foreach_slot(pgp->paths, pp, j)
> +				if (pp->mpp == mpp)
> +					pp->mpp = NULL;
> +		}
> +	}
> +
>  	free_pathvec(mpp->paths, free_paths);
>  	free_pgvec(mpp->pg, free_paths);
>  	FREE_PTR(mpp->mpcontext);
> -- 
> 2.28.0

Re: [PATCH v2 40/42] libmultipath: free_multipath(): remove mpp references

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:34:04PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If free_paths is false, make sure no references to the dropped
> multipath remain. Otherwise multipathd may crash later when
> trying to access these.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index da898b7..d227ec0 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -258,6 +258,21 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
>  		mpp->dmi = NULL;
>  	}
>  
> +	if (!free_paths && mpp->pg) {
> +		struct pathgroup *pgp;
> +		struct path *pp;
> +		int i, j;
> +
> +		/*
> +		 * Make sure paths carry no reference to this mpp any more
> +		 */
> +		vector_foreach_slot(mpp->pg, pgp, i) {
> +			vector_foreach_slot(pgp->paths, pp, j)
> +				if (pp->mpp == mpp)
> +					pp->mpp = NULL;
> +		}
> +	}
> +
>  	free_pathvec(mpp->paths, free_paths);
>  	free_pgvec(mpp->pg, free_paths);
>  	FREE_PTR(mpp->mpcontext);
> -- 
> 2.28.0

[PATCH v2 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 is harmful. The existing paths would first
be replaced by others with the new WWID. The WWIDs of the new paths wouldn't
match the map WWID; thus the next time the map is disassembled, the (correct)
path WWIDs would be overwritten by the different map WWID (with subsequent
patches from this series, they'd be orphaned instead, which is better but
still not ideal). When the map is reloaded later, paths with diffent WWIDs
may be mixed, causing data corruption.

Refuse reloading a map under such circumstances.

Note: this patch doesn't change multipath-tools behavior in the case
where valid map aliases are supposed to be swapped (typically if the bindings
file differs between initrd and root FS). In this case, select_action()
selects ACT_RENAME, which this patch doesn't affect. The user-visible behavior
in this case remains as-is: the map renames fail, and the aliases remain
unchanged, thus not matching the current bindings file, but art least
internally consistent.

To fully fix this use case, coalesce_paths() must cease setting up maps one
by one. Instead, we'd need to build up a full set of maps-to-be-set-up,
and create them in a single batch, figuring out a "rename strategy" beforehand
to avoid clashes between existing and to-be-created maps (for example,
a name swap A<->B would need to be carried out as B->tmp, A->B, tmp->A).
Implementing that is out of the scope of this patch series.

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 315eb6a..5f60f74 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.28.0

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

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:34:05PM +0200, mwilck@suse.com wrote:
> 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 is harmful. The existing paths would first
> be replaced by others with the new WWID. The WWIDs of the new paths wouldn't
> match the map WWID; thus the next time the map is disassembled, the (correct)
> path WWIDs would be overwritten by the different map WWID (with subsequent
> patches from this series, they'd be orphaned instead, which is better but
> still not ideal). When the map is reloaded later, paths with diffent WWIDs
> may be mixed, causing data corruption.
> 
> Refuse reloading a map under such circumstances.
> 
> Note: this patch doesn't change multipath-tools behavior in the case
> where valid map aliases are supposed to be swapped (typically if the bindings
> file differs between initrd and root FS). In this case, select_action()
> selects ACT_RENAME, which this patch doesn't affect. The user-visible behavior
> in this case remains as-is: the map renames fail, and the aliases remain
> unchanged, thus not matching the current bindings file, but art least
> internally consistent.
> 
> To fully fix this use case, coalesce_paths() must cease setting up maps one
> by one. Instead, we'd need to build up a full set of maps-to-be-set-up,
> and create them in a single batch, figuring out a "rename strategy" beforehand
> to avoid clashes between existing and to-be-created maps (for example,
> a name swap A<->B would need to be carried out as B->tmp, A->B, tmp->A).
> Implementing that is out of the scope of this patch series.
>

We need to do something about this, and this is an easy stop-gap, so I'm
fine with this patch.  However it might be better check if we can fall
back to using the wwid for the alias (we should always be able to,
unless people pick really bad aliases in /etc/multipath.conf). This will
allow us to create the device.  It can always get renamed later, and all
things being equal, it seems better to have a device with a wwid alias,
than to have no device at all.

Also, I do wonder if we can't still setup devices one at a time.  As
long as we agree that it is unsupported to configure multipath so that
two devices have the same alias or to configure a device with an alias
that matches another device's wwid, and that we can't guarantee what the
devices will be named in that case (or even that there names won't
change when commands are run), it seems doable.

If we are reloading all the devices, and there is an A<->B alias swap,
when we reload A (assume that's the first one we see, with a wwid of
WWID1) we notice that there already is an existing device named B (with
a wwid of WWID2). Before continuing with the reload of A, we reanme B to
WWID2 (which should work. If not we refuse the reload of A). When we
later get to the device now named WWID2, we can rename it to A.

If we are only reloading a single device, I'm fine with failing, with a
message telling the user to reload all devices to fix the issue. I'm
also open to convinced that it would be o.k. to rename a device that we
weren't planing on reloading, because it has a wrong alias, which messes
with the device that we were supposed to reload (I assume this makes the
code easier). Does that sound like a reasonable solution?
 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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 315eb6a..5f60f74 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.28.0

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

[Martin Wilck]

Hi Ben,

On Thu, 2020-08-13 at 17:23 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 12, 2020 at 01:34:05PM +0200, mwilck@suse.com wrote:
> > 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 is harmful. The existing paths would
> > first
> > be replaced by others with the new WWID. The WWIDs of the new paths
> > wouldn't
> > match the map WWID; thus the next time the map is disassembled, the
> > (correct)
> > path WWIDs would be overwritten by the different map WWID (with
> > subsequent
> > patches from this series, they'd be orphaned instead, which is
> > better but
> > still not ideal). When the map is reloaded later, paths with
> > diffent WWIDs
> > may be mixed, causing data corruption.
> > 
> > Refuse reloading a map under such circumstances.
> > 
> > Note: this patch doesn't change multipath-tools behavior in the
> > case
> > where valid map aliases are supposed to be swapped (typically if
> > the bindings
> > file differs between initrd and root FS). In this case,
> > select_action()
> > selects ACT_RENAME, which this patch doesn't affect. The user-
> > visible behavior
> > in this case remains as-is: the map renames fail, and the aliases
> > remain
> > unchanged, thus not matching the current bindings file, but art
> > least
> > internally consistent.
> > 
> > To fully fix this use case, coalesce_paths() must cease setting up
> > maps one
> > by one. Instead, we'd need to build up a full set of maps-to-be-
> > set-up,
> > and create them in a single batch, figuring out a "rename strategy"
> > beforehand
> > to avoid clashes between existing and to-be-created maps (for
> > example,
> > a name swap A<->B would need to be carried out as B->tmp, A->B,
> > tmp->A).
> > Implementing that is out of the scope of this patch series.
> > 
> 
> We need to do something about this, and this is an easy stop-gap, so
> I'm
> fine with this patch.  However it might be better check if we can
> fall
> back to using the wwid for the alias (we should always be able to,
> unless people pick really bad aliases in /etc/multipath.conf). This
> will
> allow us to create the device.  It can always get renamed later, and
> all
> things being equal, it seems better to have a device with a wwid
> alias,
> than to have no device at all.

Patch 80 from my series basically does that. There's currently no check
whether some other map mistakenly uses that WWID as alias, though.

> 
> Also, I do wonder if we can't still setup devices one at a time.  As
> long as we agree that it is unsupported to configure multipath so
> that
> two devices have the same alias or to configure a device with an
> alias
> that matches another device's wwid, and that we can't guarantee what
> the
> devices will be named in that case (or even that there names won't
> change when commands are run), it seems doable.

There are potential conflicts in two directions: 1) with the future,
to-be-set-up set of maps, and 2) with the current, existing maps. This
is problematic per se, because it creates more apparent conflicts than
actually exist. The current code looks almost exclusively at conflicts
of type 2), but I think what actually matters more is type 1).

If we didn't set up one by one, we could examine the "future" setup for
possible conflicts, and ignore conflicts with the existing setup
entirely, except for calculating a setup strategy / ordering that would
avoid conflicts "on the way".

When we set up one by one, it gets even worse because 1) is only
partially known while we set up the maps. When we set up a map, we
simply don't know which alias other maps we are yet going to set up
will be configured to have. Both false positives and false negatives
are possible. My attempt to take conflicts of type 1) into account in
the v1 series was incorrect and rightfully rejected by you.

> If we are reloading all the devices, and there is an A<->B alias
> swap,
> when we reload A (assume that's the first one we see, with a wwid of
> WWID1) we notice that there already is an existing device named B
> (with
> a wwid of WWID2). Before continuing with the reload of A, we reanme B
> to
> WWID2 (which should work. If not we refuse the reload of A). When we
> later get to the device now named WWID2, we can rename it to A.

This is one possible approach, yes. Indeed I considered a more radical
idea, renaming *all* maps to either WWID or random names before trying
to set up the final set of maps. That would avoid name clashes "on the
way" by definition, and avoid rename-specific error handling. But it
would issue a lot of unnecessary rename ioctls.

The other option would be bulk renaming with a pre-calculated renaming
strategy. I believe there two basic renaming sequences, one being a
"linear" sequence A->B->C->...->X-><N>, where <N> is a previously
unused alias, and one A->B->C->...->X->A (cycle; the swap is a special
case of the cycle). The linear case can be resolved easily be renaming
backwards. The cycle must first be "cut open" e.g. by renaming X->tmp;
then treated like linear, and finally tmp->A. The code would need to
identify these sequences, and figure out a sequence of renames that
would avoid conflicts with existing maps. This solution uses a minimal
number of renames, and would generally be the cleanest IMO. But it also
requires most thought and coding effort.

[ Something makes me feel we're tackling a solved problem here, I have
  a vague recollection of past issues with ethernet device renaming ...
  :-/ ]

> If we are only reloading a single device, I'm fine with failing, with
> a
> message telling the user to reload all devices to fix the issue. I'm
> also open to convinced that it would be o.k. to rename a device that
> we
> weren't planing on reloading, because it has a wrong alias, which
> messes
> with the device that we were supposed to reload (I assume this makes
> the
> code easier). Does that sound like a reasonable solution?

I sounds ok. But my experiments with the different patches have come
with various surprises to me, where the code behaved differently than I
had expected. The current logic is complex, split over different
functions (coalesce_paths(), select_alias(), select_action(), domap()),
and hard to follow for the reasons I described above. What you describe
here would add yet another special case with special treatment. Some
time in the future I'd like to see a clear and well-defined logic
executed in a reproducible manner.

Thanks for the review,
Martin

>  
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> > 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 315eb6a..5f60f74 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.28.0

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

[Martin Wilck]

On Fri, 2020-08-14 at 09:48 +0200, Martin Wilck wrote:
> Hi Ben,
> 
> On Thu, 2020-08-13 at 17:23 -0500, Benjamin Marzinski wrote:
> > 
> > I'm
> > also open to convinced that it would be o.k. to rename a device
> > that
> > we
> > weren't planing on reloading, because it has a wrong alias, which
> > messes
> > with the device that we were supposed to reload (I assume this
> > makes
> > the
> > code easier). Does that sound like a reasonable solution?
> 
> I sounds ok.

s/I/It/ - sorry.