[PATCH 0/9] multipath-tools: more small fixes

[PATCH 0/9] multipath-tools: more small fixes

[Martin Wilck]

1 and 2 are minor cleanups. 

Patch 3-5 are follow-ups of the structure-freeing cleanup of my
previous patch series "Multipath-tools: various bug fixes".
The previous series tried to make sure that no freed "struct multipath"
objects are ever referenced by any "struct path" objects. To achieve
that, it nullified pp->mpp links from all paths in a struct multipath
when the latter was discarded in free_multipath(). This is only safe
if we make sure that the path referenced by a struct multipath (either
via mpp->paths or mpp->pg[i]->paths for some i), aren't freed before
the struct multipath itself. Ideally, the orphaning in free_multipath()
wouldn't need to happen if we'd make sure that pp->mpp is set *if and
only if* pp was referenced by mpp as explained above. I think we do
a decent job at that, but it's hard to tell with 100% confidence.

In order to improve confidence, patch 4 and 5 add some logging. It's now an
error if a path which is freed still links to a multipath map. Patch 3/5
eliminates a few cases where this could still happen, even though the path was
not member of the map. Patch 5 is mainly meant to debug cases pp->mpp was
either NULL or pointing to a different map. This shouldn't happen and I
haven't observed it in my testing so far.

Patch 6 avoids a strange behavior that multipathd has had forever - after
grouping paths, mpp->paths was freed. In most of our code, we assume that
mpp->paths and mpp->pg should reference the same set of path devices, and
we have update_mpp_paths() and sync_paths() to take care of it. But right
after path grouping, this is not the case. Subsequent invocations of
update_mpp_path() (usually via refresh_multipath()) will re-allocate and
fill the paths vector. This causes a lot of unnecessary heap operations.

Patch 7 and 8 are unrelated bug fixes which I came across lately.

Patch 9 is motivated by the recent patch "libmpathutil: use union for bitfield".
I did some further experiments with strict aliasing and found that the
multipath-tools code base just should not use it at all. Like the kernel,
we use between low-level data structures that don't comply with strict
aliasing rules. So like the kernel, we should disable strict aliasing.
That basically obsoletes "libmpathutil: use union for bitfield", but having
it doesn't hurt, either.

Interestingly, switching to -fno-strict-aliasing seems to have fixed [1],
which I'd unsuccessfully tried to fix with various methods before.

[1] https://github.com/opensvc/multipath-tools/issues/130

Martin Wilck (9):
  libmultipath: reset pgindex in uninitialize_path()
  libmultipath: clarify code in set_path_removed()
  libmultipath: nullify pp->mpp before calling free_path()
  libmultipath: log error when freeing path that refers to a map
  libmultipath: sync_paths(): print message when fixing pp->mpp
  libmultipath: don't free mpp->paths in group_paths()
  libmultipath: warn only once in scsi_tmo_error_msg()
  libmultipath: find_hwe(): fix gcc errors at high optimization levels
  multipath-tools: compile with -fno-strict-aliasing

 Makefile.inc               |  2 +-
 libmultipath/config.c      | 13 ++++++++++---
 libmultipath/discovery.c   |  3 +++
 libmultipath/pgpolicies.c  |  2 --
 libmultipath/structs.c     |  4 ++++
 libmultipath/structs_vec.c | 26 +++++++++++++++++---------
 tests/pgpolicy.c           |  7 +++++--
 7 files changed, 40 insertions(+), 17 deletions(-)

-- 
2.52.0

[PATCH 1/9] libmultipath: reset pgindex in uninitialize_path()

[Martin Wilck]

This is where this operation logically belongs.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.c     | 1 +
 libmultipath/structs_vec.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 4d993be..29c0008 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -150,6 +150,7 @@ uninitialize_path(struct path *pp)
 	pp->uid_attribute = NULL;
 	pp->checker_timeout = 0;
 	pp->pending_ticks = 0;
+	pp->pgindex = 0;
 
 	if (checker_selected(&pp->checker))
 		checker_put(&pp->checker);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 75b6d5b..434b57c 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -373,7 +373,6 @@ void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
 	pp->mpp = NULL;
-	pp->pgindex = 0;
 	uninitialize_path(pp);
 }
 
-- 
2.52.0

[PATCH 2/9] libmultipath: clarify code in set_path_removed()

[Martin Wilck]

Instead of remembering pp->mpp and setting it back after orphan_path(),
create a variant orphan_path__() that keeps pp->mpp set. The name
of this function is kind of misleading, but keep it as the main caller
is still orphan_path(), and we want to keep the well-known message.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 434b57c..8d4fc69 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -369,13 +369,18 @@ err:
 	return 1;
 }
 
-void orphan_path(struct path *pp, const char *reason)
+static void orphan_path__(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
-	pp->mpp = NULL;
 	uninitialize_path(pp);
 }
 
+void orphan_path(struct path *pp, const char *reason)
+{
+	pp->mpp = NULL;
+	orphan_path__(pp, reason);
+}
+
 static void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 {
 	int i;
@@ -393,16 +398,13 @@ static void orphan_paths(vector pathvec, struct multipath *mpp, const char *reas
 
 void set_path_removed(struct path *pp)
 {
-	struct multipath *mpp = pp->mpp;
-
-	orphan_path(pp, "removed");
 	/*
 	 * Keep link to mpp. It will be removed when the path
 	 * is successfully removed from the map.
 	 */
-	if (!mpp)
+	if (!pp->mpp)
 		condlog(0, "%s: internal error: mpp == NULL", pp->dev);
-	pp->mpp = mpp;
+	orphan_path__(pp, "removed");
 	pp->initialized = INIT_REMOVED;
 }
 
-- 
2.52.0

[PATCH 3/9] libmultipath: nullify pp->mpp before calling free_path()

[Martin Wilck]

This is a preparation for the next patch.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8d4fc69..7185038 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -181,6 +181,7 @@ static void update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 					condlog(2, "%s: discarding non-existing path %s",
 						mpp->alias, pp->dev_t);
 					vector_del_slot(pgp->paths, j--);
+					pp->mpp = NULL;
 					free_path(pp);
 					must_reload = true;
 					continue;
@@ -201,6 +202,7 @@ static void update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 						condlog(1, "%s: error %d in pathinfo, discarding path",
 							pp->dev, rc);
 						vector_del_slot(pgp->paths, j--);
+						pp->mpp = NULL;
 						free_path(pp);
 						must_reload = true;
 						continue;
@@ -605,6 +607,7 @@ static void check_removed_paths(const struct multipath *mpp, vector pathvec)
 				pp->initialized == INIT_REMOVED ?
 				"removed" : "partial");
 			vector_del_slot(pathvec, i--);
+			pp->mpp = NULL;
 			free_path(pp);
 		}
 	}
-- 
2.52.0

[PATCH 4/9] libmultipath: log error when freeing path that refers to a map

[Martin Wilck]

We want to make sure that we never free a path that still references
a map. This error message will hopefully never be printed.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 29c0008..f441312 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -170,6 +170,9 @@ free_path (struct path * pp)
 	if (!pp)
 		return;
 
+	if (pp->mpp)
+		condlog(0, "%s: INTERNAL ERROR: path %s references a map",
+			__func__, pp->dev_t);
 	uninitialize_path(pp);
 
 	if (pp->udev) {
-- 
2.52.0

[PATCH 5/9] libmultipath: sync_paths(): print message when fixing pp->mpp

[Martin Wilck]

This should help us find cases where pp->mpp isn't properly set
to begin with.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 7185038..9384de0 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -639,7 +639,11 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 	check_removed_paths(mpp, pathvec);
 	update_mpp_paths(mpp, pathvec);
 	vector_foreach_slot (mpp->paths, pp, i)
-		pp->mpp = mpp;
+		if (pp->mpp != mpp) {
+			condlog(2, "%s: %s: mpp of %s was %p, fixing to %p",
+				__func__, mpp->alias, pp->dev_t, pp->mpp, mpp);
+			pp->mpp = mpp;
+		};
 }
 
 /* This function may free paths. See check_removed_paths(). */
-- 
2.52.0

[PATCH 6/9] libmultipath: don't free mpp->paths in group_paths()

[Martin Wilck]

I'm not sure why we ever did this. We have update_mpp_paths() and
sync_paths() now, which make sure that mpp->paths and mpp->pg are
as synced as possible.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/pgpolicies.c | 2 --
 tests/pgpolicy.c          | 7 +++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 57b7485..92f84c7 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -158,8 +158,6 @@ int group_paths(struct multipath *mp, int marginal_pathgroups)
 	}
 	sort_pathgroups(mp);
 out:
-	vector_free(mp->paths);
-	mp->paths = NULL;
 	return 0;
 fail_marginal:
 	vector_free(normal);
diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index aec16ec..a37aa8e 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -131,6 +131,8 @@ static int setup_null(void **state)
 
 static int teardownX(struct multipath *mp)
 {
+	vector_free(mp->paths);
+	mp->paths = NULL;
 	free_pgvec(mp->pg);
 	mp->pg = NULL;
 	return 0;
@@ -165,10 +167,9 @@ static void
 verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
 		  int *group_size, int *marginal, int size)
 {
-	int i, j;
+	int i, j, sum = 0;
 	struct pathgroup *pgp;
 
-	assert_null(mp->paths);
 	assert_non_null(mp->pg);
 	assert_int_equal(VECTOR_SIZE(mp->pg), size);
 	for (i = 0; i < size; i++) {
@@ -176,6 +177,7 @@ verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
 		assert_non_null(pgp);
 		assert_non_null(pgp->paths);
 		assert_int_equal(VECTOR_SIZE(pgp->paths), group_size[i]);
+		sum += group_size[i];
 		if (marginal)
 			assert_int_equal(pgp->marginal, marginal[i]);
 		else
@@ -192,6 +194,7 @@ verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
 			assert_ptr_equal(pgp_path, pp_path);
 		}
 	}
+	assert_int_equal(sum, VECTOR_SIZE(mp->paths));
 }
 
 static void test_one_group8(void **state)
-- 
2.52.0

[PATCH 7/9] libmultipath: warn only once in scsi_tmo_error_msg()

[Martin Wilck]

The intention of this code was to warn only once for each unsupported
protocol. But the condition statement is missing.

Fixes: 6ad77db ("libmultipath: Set the scsi timeout parameters by path")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 9d0f6d9..f8b514b 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -888,6 +888,9 @@ scsi_tmo_error_msg(struct path *pp)
 
 	/* make sure the bitfield is large enough */
 	BUILD_BUG_ON((LAST_BUS_PROTOCOL_ID + 1) > bits_per_slot);
+
+	if (is_bit_set_in_bitfield(proto_id, bf))
+		return;
 	snprint_path_protocol(&proto_buf, pp);
 	condlog(2, "%s: setting scsi timeouts is unsupported for protocol %s",
 		pp->dev, get_strbuf_str(&proto_buf));
-- 
2.52.0

[PATCH 8/9] libmultipath: find_hwe(): fix gcc errors at high optimization levels

[Martin Wilck]

With -O6, gcc complains that we might pass NULL strings to the "%s"
format specifier. Make sure this can't happen.

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

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 0717c83..d0a14c9 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -168,13 +168,19 @@ out:
 	return retval;
 }
 
+static const char *avoid_null(const char *s)
+{
+	return s ? s : "(unset)";
+}
+
 static void _log_match(const char *fn, const struct hwentry *h,
 		       const char *vendor, const char *product,
 		       const char *revision)
 {
 	condlog(4, "%s: found match /%s:%s:%s/ for '%s:%s:%s'", fn,
-		h->vendor, h->product, h->revision,
-		vendor, product, revision);
+		avoid_null(h->vendor), avoid_null(h->product),
+		avoid_null(h->revision), avoid_null(vendor),
+		avoid_null(product), avoid_null(revision));
 }
 #define log_match(h, v, p, r) _log_match(__func__, (h), (v), (p), (r))
 
@@ -203,7 +209,8 @@ find_hwe (const struct vector_s *hwtable,
 		log_match(tmp, vendor, product, revision);
 	}
 	condlog(n > 1 ? 3 : 4, "%s: found %d hwtable matches for %s:%s:%s",
-		__func__, n, vendor, product, revision);
+		__func__, n, avoid_null(vendor), avoid_null(product),
+		avoid_null(revision));
 	return n;
 }
 
-- 
2.52.0

[PATCH 9/9] multipath-tools: compile with -fno-strict-aliasing

[Martin Wilck]

multipath-tools uses container_of() and similar macros, which imply
casts between different types, which isn't stricly compliant with
strict aliasing rules. The issue that lead to the previous commit
"libmpathutil: use union for bitfield" was one example where this
can fail. While that one could be fixed relatively easily, it shows
that surprises can happen any time when we compile our code with
strict aliasing enabled. This can be seen clearly when we compile
with "-fstrict-aliasing -Wstrict-aliasing=1" (note that the bitfield
problem is only reported by gcc with "-Wstrict-aliasing=1", other
levels of aliasing detection miss it with gcc 15).

Use -fno-strict-aliasing to disable it. The kernel does the same.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.inc b/Makefile.inc
index 3dbcdcf..539415d 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -122,7 +122,7 @@ CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) $(D_CMOCKA_VERSION) \
 		   -DWSTRINGOP_TRUNCATION=$(if $(WSTRINGOP_TRUNCATION),1,0) \
 		   -MMD -MP
 CFLAGS		:= -std=$(C_STD) $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
-		   -fexceptions
+		   -fexceptions -fno-strict-aliasing
 BIN_CFLAGS	:= -fPIE -DPIE
 LIB_CFLAGS	:= -fPIC
 SHARED_FLAGS	:= -shared
-- 
2.52.0

Re: [PATCH 0/9] multipath-tools: more small fixes

[Benjamin Marzinski]

On Fri, Jan 09, 2026 at 07:38:58PM +0100, Martin Wilck wrote:
> 1 and 2 are minor cleanups. 
> 
> Patch 3-5 are follow-ups of the structure-freeing cleanup of my
> previous patch series "Multipath-tools: various bug fixes".
> The previous series tried to make sure that no freed "struct multipath"
> objects are ever referenced by any "struct path" objects. To achieve
> that, it nullified pp->mpp links from all paths in a struct multipath
> when the latter was discarded in free_multipath(). This is only safe
> if we make sure that the path referenced by a struct multipath (either
> via mpp->paths or mpp->pg[i]->paths for some i), aren't freed before
> the struct multipath itself. Ideally, the orphaning in free_multipath()
> wouldn't need to happen if we'd make sure that pp->mpp is set *if and
> only if* pp was referenced by mpp as explained above. I think we do
> a decent job at that, but it's hard to tell with 100% confidence.
> 
> In order to improve confidence, patch 4 and 5 add some logging. It's now an
> error if a path which is freed still links to a multipath map. Patch 3/5
> eliminates a few cases where this could still happen, even though the path was
> not member of the map. Patch 5 is mainly meant to debug cases pp->mpp was
> either NULL or pointing to a different map. This shouldn't happen and I
> haven't observed it in my testing so far.
> 
> Patch 6 avoids a strange behavior that multipathd has had forever - after
> grouping paths, mpp->paths was freed. In most of our code, we assume that
> mpp->paths and mpp->pg should reference the same set of path devices, and
> we have update_mpp_paths() and sync_paths() to take care of it. But right
> after path grouping, this is not the case. Subsequent invocations of
> update_mpp_path() (usually via refresh_multipath()) will re-allocate and
> fill the paths vector. This causes a lot of unnecessary heap operations.
> 
> Patch 7 and 8 are unrelated bug fixes which I came across lately.
> 
> Patch 9 is motivated by the recent patch "libmpathutil: use union for bitfield".
> I did some further experiments with strict aliasing and found that the
> multipath-tools code base just should not use it at all. Like the kernel,
> we use between low-level data structures that don't comply with strict
> aliasing rules. So like the kernel, we should disable strict aliasing.
> That basically obsoletes "libmpathutil: use union for bitfield", but having
> it doesn't hurt, either.
> 
> Interestingly, switching to -fno-strict-aliasing seems to have fixed [1],
> which I'd unsuccessfully tried to fix with various methods before.

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

> 
> [1] https://github.com/opensvc/multipath-tools/issues/130
> 
> Martin Wilck (9):
>   libmultipath: reset pgindex in uninitialize_path()
>   libmultipath: clarify code in set_path_removed()
>   libmultipath: nullify pp->mpp before calling free_path()
>   libmultipath: log error when freeing path that refers to a map
>   libmultipath: sync_paths(): print message when fixing pp->mpp
>   libmultipath: don't free mpp->paths in group_paths()
>   libmultipath: warn only once in scsi_tmo_error_msg()
>   libmultipath: find_hwe(): fix gcc errors at high optimization levels
>   multipath-tools: compile with -fno-strict-aliasing
> 
>  Makefile.inc               |  2 +-
>  libmultipath/config.c      | 13 ++++++++++---
>  libmultipath/discovery.c   |  3 +++
>  libmultipath/pgpolicies.c  |  2 --
>  libmultipath/structs.c     |  4 ++++
>  libmultipath/structs_vec.c | 26 +++++++++++++++++---------
>  tests/pgpolicy.c           |  7 +++++--
>  7 files changed, 40 insertions(+), 17 deletions(-)
> 
> -- 
> 2.52.0