[PATCH 0/4] multipath-tools: unmangle WWIDs if necessary

[PATCH 0/4] multipath-tools: unmangle WWIDs if necessary

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Recent changes in sg3_utils have changed the way certain WWIDs are presented to
the system. This may lead to maps not being set up or even being destroyed
or removed. Fix this by "unmangling" the WWIDs similar to libdevmapper.

The main part of this series is patch 2/4, it's commit message contains
detailed information. Patch 3 adds tests for the unmangling functionality.

Patch 1 is a small prerequisite. Patch 4 is an optimization that occured
to me while looking at wwids.c.

Comments and questions welcome!

Martin

Martin Wilck (4):
  libmultipath: constify functions in wwid.c
  libmultipath: unmangle WWIDs if necessary
  multipath-tools tests: add test for WWID unmangling
  libmultipath: use mmap in lookup_wwid()

 libmultipath/discovery.c | 103 ++++++++++-
 libmultipath/discovery.h |   1 +
 libmultipath/wwids.c     |  82 ++++-----
 libmultipath/wwids.h     |   6 +-
 tests/Makefile           |   5 +-
 tests/cmocka-compat.h    |   1 +
 tests/wwid-unmangle.c    | 375 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 523 insertions(+), 50 deletions(-)
 create mode 100644 tests/wwid-unmangle.c

-- 
2.55.0

[PATCH 1/4] libmultipath: constify functions in wwid.c

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Use `const char *wwid` where possible.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/wwids.c | 15 ++++++---------
 libmultipath/wwids.h |  6 +++---
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index aac18c0..c193b97 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -25,7 +25,7 @@
  */
 
 static int
-lookup_wwid(FILE *f, char *wwid) {
+lookup_wwid(FILE *f, const char *wwid) {
 	int c;
 	char buf[LINE_MAX];
 	int count;
@@ -58,7 +58,7 @@ next:
 }
 
 static int
-write_out_wwid(int fd, char *wwid) {
+write_out_wwid(int fd, const char *wwid) {
 	int ret;
 	off_t offset;
 	char buf[WWID_SIZE + 3];
@@ -189,9 +189,8 @@ do_remove_wwid(int fd, char *str) {
 	}
 }
 
-
-int
-remove_wwid(char *wwid) {
+int remove_wwid(const char *wwid)
+{
 	int fd = -1;
 	int len, can_write;
 	char *str;
@@ -231,8 +230,7 @@ out:
 	return ret;
 }
 
-int
-check_wwids_file(char *wwid, int write_wwid)
+int check_wwids_file(const char *wwid, int write_wwid)
 {
 	int fd, can_write, found, ret;
 	FILE *f;
@@ -321,8 +319,7 @@ should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 	return 1;
 }
 
-int
-remember_wwid(char *wwid)
+int remember_wwid(const char *wwid)
 {
 	int ret = check_wwids_file(wwid, 1);
 	if (ret < 0){
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index ee47ad9..5606ac6 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -13,9 +13,9 @@
 "# Valid WWIDs:\n"
 
 int should_multipath(struct path *pp, vector pathvec, vector mpvec);
-int remember_wwid(char *wwid);
-int check_wwids_file(char *wwid, int write_wwid);
-int remove_wwid(char *wwid);
+int remember_wwid(const char *wwid);
+int check_wwids_file(const char *wwid, int write_wwid);
+int remove_wwid(const char *wwid);
 int replace_wwids(vector mp);
 
 enum {
-- 
2.55.0

[PATCH 2/4] libmultipath: unmangle WWIDs if necessary

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Recent vulnerability reports have modified the output of sg_inq for
the the WWID in the udev property ID_SERIAL of certain devices [1], for
cases in which the device-provided WWID contains characters that are not
allowed in device nodes, such as comma or semicolon.

This causes 2 problems for multipath-tools:

1) The WWID is now different than before, and doesn't match existing
entries in /etc/multipath/wwids and /etc/multipath/bindings. This can
cause multipath maps not to be assembled at all, in particular if
"find_multipaths strict" is set multipath.conf, or alias mismatches.

2) In certain situations, multipath or multipathd determine the WWID from
the map (DM_UUID) and not from the path. This happens e.g. when an "add"
uevent is recevied for a map that has no known paths, or when "multipath
-U" is run from udev rules. libdevmapper automatically "mangles" device
UUIDs when it creates maps in order to avoid issues with illegal characters
in device nodes or udev symlinks. When it reads the WWIDs back, it
"unmangles" these strings by default, meaning that a WWID that actually
contains a mangled sequence such as `\x2c` will be returned with this
sequence replaced by a comma [2]. This can cause mismatches between map and
path WWID, with the effect that multipath or multipathd fail and remove the
paths from the map. This happens for every path, making the map unusable.

Problem 2) could be fixed by explicitly requesting the unmangled
device-mapper UUID from device-mapper when gathering information about a
map. But this leaves problem 1). Asking libdevmapper not to mangle names
during device creation is another possibility, but it causes issues with
udev symlink names etc. elsewhere, which we should strictly avoid.

Instead, solve the issue by unmangling WWIDs in libmultipath. This comes
with another problem: in libmultipath, we don't know for sure whether or
not given WWID had been mangled by by udev or similar tools like scsi_id or
sg_inq. We just query the ID_SERIAL udev property, which is set by udev
rules from device properties, some of which (like T10 vendor ID and SCSI
name string) are mangled by udev, and some not (like NAA designators). Both
`uid_attribute` and the udev rules can be customized by users, aggravating
the problem. Unmangling a WWID that hadn't been mangled in the first place
causes other issues, and while highly unusual, it is not forbidden that
e.g. the physical model ID contain `\xHH` character sequences.

This patch therefore adopts a multi-stage approach:

1. If the WWID contains a forbidden character, it can't be mangled by udev,
   use original WWID.
2. If it doesn't contain a `\xHH` sequence, it isn't mangled, use original.
3. If the unmangled WWID is known in the WWIDs file, use it.
4. If the original WWID is known in the WWIDs file, use it.
5. If in doubt, use the unmangled WWID.

IOW, option 5) is applied when a WWID is encountered that contains no
forbidden characters and at least one `\xHH` sequence, and which doesn't
appear in the WWIDs file in either unmangled or not-unmangled form. This
can still be wrong in unusual circumstances, but as the WWID appears
to be new, it shouldn't cause disruption as in case 2) above.

The unmangled WWID can contain characters which are not in udev's
whitelist. This patch again "mangles" only unprintable characters to `_`,
in order to avoid issues with symlink names. Whether or not that's correct
is debatable, but as it occurs before map creation, it's unlikely to cause
problems. In accordance with systemd code, multi-byte unicode (UTF-8)
characters are not mangled.

[1] sg3_utils commit 2fe1452 ("sg_inq: --export output conformance for SCSI
name string and ATA fields (github: PR #83); Apply udev-conforming
character encoding to VPD 0x83 designator type 8 (SCSI name string) and the
T10 vendor ID ATA subfield, matching the encoding already used by
designator types 0 and 1 since commit 4d770f1")

[2] See e.g. the documentation for the --manglename option in dmsetup(8).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 103 +++++++++++++++++++++++++++++++++++++--
 libmultipath/discovery.h |   1 +
 2 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 344da19..66af333 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -37,6 +37,7 @@
 #include "print.h"
 #include "strbuf.h"
 #include "pgpolicies.h"
+#include "wwids.h"
 
 #define VPD_BUFLEN 4096
 
@@ -45,6 +46,97 @@ struct vpd_vendor_page vpd_vendor_pages[VPD_VP_ARRAY_SIZE] = {
 	[VPD_VP_HP3PAR]	= { 0xc0, "hp3par" },
 };
 
+/* from libdm/libdm-common.c; license: LGPL-2.1-only */
+static bool _is_whitelisted_char(char c)
+{
+	/*
+	 * Actually, DM supports any character in a device name.
+	 * This whitelist is just for proper integration with udev.
+	 */
+	if ((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') ||
+	    (c >= 'a' && c <= 'z') || strchr("#+-.:=@_", c) != NULL)
+		return true;
+
+	return false;
+}
+
+size_t maybe_unmangle_wwid(char new_wwid[WWID_SIZE], const char *wwid)
+{
+	char buf[WWID_SIZE], *dst;
+	const char *src;
+	bool mangled = false, invalid = false;
+	size_t rc, len;
+
+	if (!wwid) {
+		*new_wwid = '\0';
+		return 0;
+	}
+
+	/* this might be > WWID_SIZE - 1 */
+	len = strlen(wwid);
+	/*
+	 * A correctly mangled string doesn't contain invalid characters.
+	 * In particular, if the original string contained `\xXY`, it will
+	 * itself be mangled to '\x5cxXY`.
+	 */
+	/* clang-format off */
+	for (src = wwid, dst = buf;
+	     dst - buf < WWID_SIZE - 1 && *src;
+	     src++, dst++) {
+		/*clang-format on */
+		if (_is_whitelisted_char(*src)) {
+			*dst = *src;
+			continue;
+		}
+		if (*src == '\\') {
+			char hex[3], c;
+
+			/*
+			 * udev mangling always outputs 2-digit hex numbers
+			 * 1-digit would be unprintable, anyway
+			 */
+			if (src + 4 > wwid + len || *(src + 1) != 'x' ||
+			    sscanf(src + 2, "%2[A-Fa-f0-9]", hex) == 0 ||
+			    strlen(hex) != 2) {
+				condlog(3, "%s: wwid \"%s\" contains invalid mangle sequence \"%s\"",
+					__func__, wwid, src);
+				invalid = true;
+				break;
+			}
+			mangled = true;
+			src += 1 + strlen(hex);
+			c = strtol(hex, NULL, 16);
+			/* Allow non-ascii unicode, but avoid non-printable characters */
+			*dst = (c & 0x80) || isprint(c) ? c : '_';
+		} else {
+			condlog(3, "%s: wwid \"%s\" contains illegal character '%02x'",
+				__func__, wwid, (unsigned char)*src);
+			invalid = true;
+			break;
+		}
+	}
+
+	/*
+	 * 1. If we found an invalid character, it can't be mangled by udev,
+	 *    use original WWID.
+	 * 2. If we didn't found a `\xHH`, it isn't mangled, use original.
+	 * 3. If the unmangled WWID is known, use it.
+	 * 4. If the original WWID is known, use it.
+	 * 5. If in doubt, use the unmangled WWID.
+	 */
+	if (!invalid && mangled &&
+	    (!check_wwids_file(new_wwid, 0) || check_wwids_file(wwid, 0))) {
+		*dst = '\0';
+		rc = strlcpy(new_wwid, buf, WWID_SIZE);
+		condlog(3, "%s: unmangled WWID \"%s\" -> \"%s\"", __func__,
+			wwid, new_wwid);
+	} else {
+		rc = strlcpy(new_wwid, wwid, WWID_SIZE);
+		condlog(4, "%s: kept WWID \"%s\"", __func__, new_wwid);
+	}
+	return rc;
+}
+
 int
 alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
 			  const char *wwid, int flag, struct path **pp_ptr)
@@ -65,8 +157,7 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
 	if (!pp)
 		return PATHINFO_FAILED;
 
-	if (wwid)
-		strlcpy(pp->wwid, wwid, sizeof(pp->wwid));
+	maybe_unmangle_wwid(pp->wwid, wwid);
 
 	if (safe_sprintf(pp->dev, "%s", devname)) {
 		condlog(0, "pp->dev too small");
@@ -2163,8 +2254,8 @@ get_udev_uid(struct path * pp, const char *uid_attribute, struct udev_device *ud
 	value = udev_device_get_property_value(udev, uid_attribute);
 	if ((!value || strlen(value) == 0) && pp->can_use_env_uid)
 		value = getenv(uid_attribute);
-	if (value && strlen(value)) {
-		len = strlcpy(pp->wwid, value, WWID_SIZE);
+	if (value && *value) {
+		len = maybe_unmangle_wwid(pp->wwid, value);
 		if (len >= WWID_SIZE) {
 			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
 			if (len > 0)
@@ -2224,7 +2315,7 @@ static ssize_t dasd_get_uid(struct path *pp)
 	if (p)
 		*p = '\0';
 
-	return strlcpy(pp->wwid, value, WWID_SIZE);
+	return maybe_unmangle_wwid(pp->wwid, value);
 }
 
 static ssize_t uid_fallback(struct path *pp, int path_state,
@@ -2254,6 +2345,8 @@ static ssize_t uid_fallback(struct path *pp, int path_state,
 					   sizeof(value));
 		if (!sysfs_attr_value_ok(len, sizeof(value)))
 			return -1;
+
+		/* WWID read from sysfs, unmangling is not necessary */
 		len = strlcpy(pp->wwid, value, WWID_SIZE);
 		if (len >= WWID_SIZE) {
 			len = fix_broken_nvme_wwid(pp, value,
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 2ef1d94..4e9c5ae 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -30,6 +30,7 @@
 
 struct config;
 
+size_t maybe_unmangle_wwid(char new_wwid[WWID_SIZE], const char *wwid);
 int path_discovery (vector pathvec, int flag);
 int path_get_tpgs(struct path *pp); /* This function never returns TPGS_UNDEF */
 int do_tur (char *);
-- 
2.55.0

[PATCH 3/4] multipath-tools tests: add test for WWID unmangling

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add tests for the logic of the code added in the previous commit.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/Makefile        |   5 +-
 tests/cmocka-compat.h |   1 +
 tests/wwid-unmangle.c | 375 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 379 insertions(+), 2 deletions(-)
 create mode 100644 tests/wwid-unmangle.c

diff --git a/tests/Makefile b/tests/Makefile
index 23f7d2b..5c7b276 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -10,7 +10,7 @@ LIBDEPS += -L. -L $(mpathutildir) -L$(mpathcmddir) -lmultipath -lmpathutil -lmpa
 
 TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
 	 alias directio valid devt mpathvalid strbuf sysfs features cli mapinfo runner \
-	 shared_ptr $(if $(MEMFD_SUPPORT),gpt)
+	 shared_ptr $(if $(MEMFD_SUPPORT),gpt) wwid-unmangle
 HELPERS := test-lib.o test-log.o
 
 .PRECIOUS: $(TESTS:%=%-test)
@@ -71,7 +71,8 @@ mapinfo-test_LIBDEPS = -lpthread -ldevmapper
 runner-test_LIBDEPS = -lpthread
 shared_ptr-test_LIBDEPS = -lpthread
 gpt-test_OBJDEPS := $(kpartxdir)/gpt.o $(kpartxdir)/crc32.o
-
+wwid-unmangle-test_OBJDEPS := $(multipathdir)/discovery.o
+wwid-unmangle-test_LIBDEPS := -lpthread
 
 %.o: %.c
 	@echo building $@ because of $?
diff --git a/tests/cmocka-compat.h b/tests/cmocka-compat.h
index 8ec6ed2..58d73ce 100644
--- a/tests/cmocka-compat.h
+++ b/tests/cmocka-compat.h
@@ -13,4 +13,5 @@
 #define expect_int_value(f, x, y) expect_value(f, x, y)
 #define expect_uint_value(f, x, y) expect_value(f, x, y)
 #define assert_int_in_set(x, vals, count) assert_in_set(x, vals, count)
+#define will_return_int(f, x) will_return(f, x)
 #endif
diff --git a/tests/wwid-unmangle.c b/tests/wwid-unmangle.c
new file mode 100644
index 0000000..5188706
--- /dev/null
+++ b/tests/wwid-unmangle.c
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 SUSE Linux GmbH
+ */
+
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include "cmocka-compat.h"
+#include "structs.h"
+#include "discovery.h"
+#include "util.h"
+
+#include "globals.c"
+
+// #include "globals.c"
+static char wwid_buf[WWID_SIZE];
+
+int __wrap_check_wwids_file(const char *wwid, int write)
+{
+	return mock_type(int);
+}
+
+static void test_wwid_null(void **state)
+{
+	size_t res = maybe_unmangle_wwid(wwid_buf, NULL);
+
+	assert_int_equal(res, 0);
+	assert_string_equal(wwid_buf, "");
+}
+
+static void test_wwid_zero(void **state)
+{
+	size_t res = maybe_unmangle_wwid(wwid_buf, "");
+
+	assert_int_equal(res, 0);
+	assert_string_equal(wwid_buf, "");
+}
+
+/* contains no mangled chars */
+static void test_wwid_no_unmangle_01(void **state)
+{
+	const char *const wwid = "simple";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid character, not unmangled */
+static void test_wwid_no_unmangle_02(void **state)
+{
+	const char *const wwid = "simple wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains unicode character, not unmangled */
+static void test_wwid_no_unmangle_03(void **state)
+{
+	const char *const wwid = "utf8→wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid character, not unmangled */
+static void test_wwid_no_unmangle_04(void **state)
+{
+	const char *const wwid = "simple;wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid character, not unmangled */
+static void test_wwid_no_unmangle_05(void **state)
+{
+	const char *const wwid = "simple\\wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains unicode character, not unmangled */
+static void test_wwid_no_unmangle_06(void **state)
+{
+	const char *const wwid = "utf8→wwid\x20space";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid character, not unmangled */
+static void test_wwid_no_unmangle_07(void **state)
+{
+	const char *const wwid = "invalid\\x20;wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid character, not unmangled */
+static void test_wwid_no_unmangle_08(void **state)
+{
+	const char *const wwid = "invalid,wwid\x20space";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains unicode character, not unmangled */
+static void test_wwid_no_unmangle_09(void **state)
+{
+	const char *const wwid = "utf8\\x20→wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* contains invalid mangled char */
+static void test_wwid_no_unmangle_10(void **state)
+{
+	const char *const wwid = "invalid\\x1wwid";
+	size_t res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* test that WWID_SIZE doesn't overflow */
+static void test_wwid_overlong_01(void **state)
+{
+	char wwid[WWID_SIZE + 2];
+	size_t res;
+	memset(wwid, 'A', sizeof(wwid) - 1);
+
+	wwid[sizeof(wwid) - 1] = '\0';
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+	assert_int_equal(res, strlen(wwid));
+	wwid[WWID_SIZE - 1] = '\0';
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* mangle sequence at the end, fits after mangling */
+static void test_wwid_overlong_02(void **state)
+{
+	char wwid[WWID_SIZE];
+	size_t res;
+	memset(wwid, 'A', sizeof(wwid) - 1);
+
+	wwid[sizeof(wwid) - 1] = '\0';
+	strlcpy(wwid + sizeof(wwid) - 5, "\\x20", 5);
+	memcpy(wwid, "\\x20", 4);
+
+	will_return_int(__wrap_check_wwids_file, 0);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	wwid[3] = ' ';
+	strlcpy(wwid + sizeof(wwid) - 5, " ", 5);
+	assert_int_equal(res, strlen(wwid + 3));
+	assert_string_equal(wwid_buf, wwid + 3);
+}
+
+/* truncated mangle sequence at the end, will no be unmangled */
+static void test_wwid_overlong_03(void **state)
+{
+	char wwid[WWID_SIZE + 5];
+	size_t res;
+	memset(wwid, 'A', sizeof(wwid) - 1);
+
+	wwid[sizeof(wwid) - 1] = '\0';
+	strlcpy(wwid + sizeof(wwid) - 4, "\\x20", 4);
+	memcpy(wwid, "\\x20", 4);
+
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+	assert_int_equal(res, strlen(wwid));
+	wwid[WWID_SIZE - 1] = '\0';
+	assert_string_equal(wwid_buf, wwid);
+}
+
+/* overlong string fits after unmangling */
+static void test_wwid_overlong_04(void **state)
+{
+	char wwid[WWID_SIZE + 3];
+	size_t res;
+	memset(wwid, 'A', sizeof(wwid) - 1);
+
+	wwid[sizeof(wwid) - 1] = '\0';
+	strlcpy(wwid, "\\x20", sizeof(wwid));
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	wwid[3] = ' ';
+	assert_int_equal(res, strlen(wwid + 3));
+	assert_string_equal(wwid_buf, wwid + 3);
+}
+
+static void test_wwid_unmangle_01(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x20spaces";
+	const char *const expected = "wwid with spaces";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_02(void **state)
+{
+	const char *const wwid = "wwid\\x2cwith\\x2ccommas";
+	const char *const expected = "wwid,with,commas";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_03(void **state)
+{
+	const char *const wwid = "\\x20\\x20wwid\\x20\\x20\\x20with\\x20spaces\\x20";
+	const char *const expected = "  wwid   with spaces ";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_04(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x5cbackslash";
+	const char *const expected = "wwid with\\backslash";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_05(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x5cx20mangled\\x20space";
+	const char *const expected = "wwid with\\x20mangled space";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_06(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x5cx5cmangled\\x20backslash";
+	const char *const expected = "wwid with\\x5cmangled backslash";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangle_07(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x09unprintable";
+	const char *const expected = "wwid with_unprintable";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 1);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_unmangled_exists(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x20spaces";
+	const char *const expected = "wwid with spaces";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 0);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(expected));
+	assert_string_equal(wwid_buf, expected);
+}
+
+static void test_wwid_orig_exists(void **state)
+{
+	const char *const wwid = "wwid\\x20with\\x20spaces";
+	size_t res;
+
+	will_return_int(__wrap_check_wwids_file, 1);
+	will_return_int(__wrap_check_wwids_file, 0);
+	res = maybe_unmangle_wwid(wwid_buf, wwid);
+
+	assert_int_equal(res, strlen(wwid));
+	assert_string_equal(wwid_buf, wwid);
+}
+
+static int test_uevent_wwid_mangle(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_wwid_null),
+		cmocka_unit_test(test_wwid_zero),
+		cmocka_unit_test(test_wwid_no_unmangle_01),
+		cmocka_unit_test(test_wwid_no_unmangle_02),
+		cmocka_unit_test(test_wwid_no_unmangle_03),
+		cmocka_unit_test(test_wwid_no_unmangle_04),
+		cmocka_unit_test(test_wwid_no_unmangle_05),
+		cmocka_unit_test(test_wwid_no_unmangle_06),
+		cmocka_unit_test(test_wwid_no_unmangle_07),
+		cmocka_unit_test(test_wwid_no_unmangle_08),
+		cmocka_unit_test(test_wwid_no_unmangle_09),
+		cmocka_unit_test(test_wwid_no_unmangle_10),
+		cmocka_unit_test(test_wwid_overlong_01),
+		cmocka_unit_test(test_wwid_overlong_02),
+		cmocka_unit_test(test_wwid_overlong_03),
+		cmocka_unit_test(test_wwid_overlong_04),
+		cmocka_unit_test(test_wwid_unmangle_01),
+		cmocka_unit_test(test_wwid_unmangle_02),
+		cmocka_unit_test(test_wwid_unmangle_03),
+		cmocka_unit_test(test_wwid_unmangle_04),
+		cmocka_unit_test(test_wwid_unmangle_05),
+		cmocka_unit_test(test_wwid_unmangle_06),
+		cmocka_unit_test(test_wwid_unmangle_07),
+		cmocka_unit_test(test_wwid_unmangled_exists),
+		cmocka_unit_test(test_wwid_orig_exists),
+	};
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	init_test_verbosity(-1);
+	ret += test_uevent_wwid_mangle();
+	return ret;
+}
-- 
2.55.0

[PATCH 4/4] libmultipath: use mmap in lookup_wwid()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Replace the line-by-line reading in lookup_wwid() with mmap()
and a simple loop over the in-memory structures.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/wwids.c | 71 ++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index c193b97..42455de 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -4,8 +4,10 @@
 #include <string.h>
 #include <limits.h>
 #include <stdio.h>
+#include <stdbool.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/mman.h>
 #include <fcntl.h>
 
 #include "util.h"
@@ -24,41 +26,44 @@
  * Copyright (c) 2010 Benjamin Marzinski, Redhat
  */
 
-static int
-lookup_wwid(FILE *f, const char *wwid) {
-	int c;
-	char buf[LINE_MAX];
-	int count;
+struct mem_map {
+	char *addr;
+	ssize_t len;
+};
 
-	while ((c = fgetc(f)) != EOF){
-		if (c != '/') {
-			if (fgets(buf, LINE_MAX, f) == NULL)
-				return 0;
-			else
-				continue;
-		}
-		count = 0;
-		while ((c = fgetc(f)) != '/') {
-			if (c == EOF)
-				return 0;
-			if (count >= WWID_SIZE - 1)
-				goto next;
-			if (wwid[count] == '\0')
-				goto next;
-			if (c != wwid[count++])
-				goto next;
-		}
-		if (wwid[count] == '\0')
-			return 1;
-next:
-		if (fgets(buf, LINE_MAX, f) == NULL)
-			return 0;
-	}
-	return 0;
+static void cleanup_mmap(struct mem_map *mm)
+{
+	if (mm->addr)
+		munmap(mm->addr, mm->len);
 }
 
-static int
-write_out_wwid(int fd, const char *wwid) {
+static bool lookup_wwid(int fd, const char *wwid)
+{
+	struct mem_map mm __attribute__((cleanup(cleanup_mmap))) = { .addr = 0 };
+	struct stat stb;
+	ssize_t wlen;
+	const char *line;
+
+	if (fstat(fd, &stb) != 0)
+		return false;
+
+	mm.len = stb.st_size;
+	mm.addr = mmap(NULL, stb.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (!mm.addr)
+		return false;
+
+	wlen = strlen(wwid);
+	for (line = mm.addr; line >= mm.addr && line < mm.addr + mm.len;
+	     line = memchr(line, '\n', mm.addr + mm.len - line) + 1)
+		if (line[0] == '/' && (line - mm.addr + 2 + wlen <= mm.len) &&
+		    line[wlen + 1] == '/' && !strncmp(wwid, line + 1, wlen))
+			return true;
+
+	return false;
+}
+
+static int write_out_wwid(int fd, const char *wwid)
+{
 	int ret;
 	off_t offset;
 	char buf[WWID_SIZE + 3];
@@ -245,7 +250,7 @@ int check_wwids_file(const char *wwid, int write_wwid)
 		close(fd);
 		return -1;
 	}
-	found = lookup_wwid(f, wwid);
+	found = lookup_wwid(fd, wwid);
 	if (found) {
 		ret = 0;
 		goto out;
-- 
2.55.0

Re: [PATCH 1/4] libmultipath: constify functions in wwid.c

[Benjamin Marzinski]

On Fri, Sep 25, 2026 at 11:40:06PM +0200, mwilck@suse.de wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Use `const char *wwid` where possible.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

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

Re: [PATCH 2/4] libmultipath: unmangle WWIDs if necessary

[Benjamin Marzinski]

On Fri, Sep 25, 2026 at 11:40:07PM +0200, mwilck@suse.de wrote:
> From: Martin Wilck <mwilck@suse.com>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 344da19..66af333 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -37,6 +37,7 @@
>  #include "print.h"
>  #include "strbuf.h"
>  #include "pgpolicies.h"
> +#include "wwids.h"
>  
>  #define VPD_BUFLEN 4096
>  
> @@ -45,6 +46,97 @@ struct vpd_vendor_page vpd_vendor_pages[VPD_VP_ARRAY_SIZE] = {
>  	[VPD_VP_HP3PAR]	= { 0xc0, "hp3par" },
>  };
>  
> +/* from libdm/libdm-common.c; license: LGPL-2.1-only */
> +static bool _is_whitelisted_char(char c)
> +{
> +	/*
> +	 * Actually, DM supports any character in a device name.
> +	 * This whitelist is just for proper integration with udev.
> +	 */
> +	if ((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') ||
> +	    (c >= 'a' && c <= 'z') || strchr("#+-.:=@_", c) != NULL)
> +		return true;
> +
> +	return false;
> +}
> +
> +size_t maybe_unmangle_wwid(char new_wwid[WWID_SIZE], const char *wwid)
> +{
> +	char buf[WWID_SIZE], *dst;
> +	const char *src;
> +	bool mangled = false, invalid = false;
> +	size_t rc, len;
> +
> +	if (!wwid) {
> +		*new_wwid = '\0';
> +		return 0;
> +	}
> +
> +	/* this might be > WWID_SIZE - 1 */
> +	len = strlen(wwid);
> +	/*
> +	 * A correctly mangled string doesn't contain invalid characters.
> +	 * In particular, if the original string contained `\xXY`, it will
> +	 * itself be mangled to '\x5cxXY`.
> +	 */
> +	/* clang-format off */
> +	for (src = wwid, dst = buf;
> +	     dst - buf < WWID_SIZE - 1 && *src;
> +	     src++, dst++) {
> +		/*clang-format on */
> +		if (_is_whitelisted_char(*src)) {
> +			*dst = *src;
> +			continue;
> +		}
> +		if (*src == '\\') {
> +			char hex[3], c;
> +
> +			/*
> +			 * udev mangling always outputs 2-digit hex numbers
> +			 * 1-digit would be unprintable, anyway
> +			 */
> +			if (src + 4 > wwid + len || *(src + 1) != 'x' ||
> +			    sscanf(src + 2, "%2[A-Fa-f0-9]", hex) == 0 ||
> +			    strlen(hex) != 2) {
> +				condlog(3, "%s: wwid \"%s\" contains invalid mangle sequence \"%s\"",
> +					__func__, wwid, src);
> +				invalid = true;
> +				break;
> +			}
> +			mangled = true;
> +			src += 1 + strlen(hex);
> +			c = strtol(hex, NULL, 16);
> +			/* Allow non-ascii unicode, but avoid non-printable characters */
> +			*dst = (c & 0x80) || isprint(c) ? c : '_';
> +		} else {
> +			condlog(3, "%s: wwid \"%s\" contains illegal character '%02x'",
> +				__func__, wwid, (unsigned char)*src);
> +			invalid = true;
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * 1. If we found an invalid character, it can't be mangled by udev,
> +	 *    use original WWID.
> +	 * 2. If we didn't found a `\xHH`, it isn't mangled, use original.
> +	 * 3. If the unmangled WWID is known, use it.
> +	 * 4. If the original WWID is known, use it.
> +	 * 5. If in doubt, use the unmangled WWID.
> +	 */
> +	if (!invalid && mangled &&

This check should use "buf" not "new_wwid".

> +	    (!check_wwids_file(new_wwid, 0) || check_wwids_file(wwid, 0))) {
> +		*dst = '\0';
> +		rc = strlcpy(new_wwid, buf, WWID_SIZE);
> +		condlog(3, "%s: unmangled WWID \"%s\" -> \"%s\"", __func__,
> +			wwid, new_wwid);
> +	} else {
> +		rc = strlcpy(new_wwid, wwid, WWID_SIZE);
> +		condlog(4, "%s: kept WWID \"%s\"", __func__, new_wwid);
> +	}

Nitpick: I think these conditionals would be easier to check against
rules as:

	if (invalid || !mangled || check_wwids_file(wwid, 0) == 0) {
		/* Keep original wwid */
	} else {
		/* Use unmangled wwid */
	}
> +	return rc;
> +}
> +
>  int
>  alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
>  			  const char *wwid, int flag, struct path **pp_ptr)
> @@ -65,8 +157,7 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
>  	if (!pp)
>  		return PATHINFO_FAILED;
>  
> -	if (wwid)
> -		strlcpy(pp->wwid, wwid, sizeof(pp->wwid));
> +	maybe_unmangle_wwid(pp->wwid, wwid);
>  
>  	if (safe_sprintf(pp->dev, "%s", devname)) {
>  		condlog(0, "pp->dev too small");
> @@ -2163,8 +2254,8 @@ get_udev_uid(struct path * pp, const char *uid_attribute, struct udev_device *ud
>  	value = udev_device_get_property_value(udev, uid_attribute);
>  	if ((!value || strlen(value) == 0) && pp->can_use_env_uid)
>  		value = getenv(uid_attribute);
> -	if (value && strlen(value)) {
> -		len = strlcpy(pp->wwid, value, WWID_SIZE);
> +	if (value && *value) {
> +		len = maybe_unmangle_wwid(pp->wwid, value);
>  		if (len >= WWID_SIZE) {
>  			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
>  			if (len > 0)
> @@ -2224,7 +2315,7 @@ static ssize_t dasd_get_uid(struct path *pp)
>  	if (p)
>  		*p = '\0';
>  
> -	return strlcpy(pp->wwid, value, WWID_SIZE);
> +	return maybe_unmangle_wwid(pp->wwid, value);

We're reading this from sysfs, so I don't think this one needs to be
unmangled.

>  }
>  
>  static ssize_t uid_fallback(struct path *pp, int path_state,
> @@ -2254,6 +2345,8 @@ static ssize_t uid_fallback(struct path *pp, int path_state,
>  					   sizeof(value));
>  		if (!sysfs_attr_value_ok(len, sizeof(value)))
>  			return -1;
> +
> +		/* WWID read from sysfs, unmangling is not necessary */

I'm pretty sure that unmangling is unnecessary in all of the methods in
uid_fallback, so this comment is confusing.

-Ben

Re: [PATCH 3/4] multipath-tools tests: add test for WWID unmangling

[Benjamin Marzinski]

On Fri, Sep 25, 2026 at 11:40:08PM +0200, mwilck@suse.de wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Add tests for the logic of the code added in the previous commit.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

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

Re: [PATCH 4/4] libmultipath: use mmap in lookup_wwid()

[Benjamin Marzinski]

On Fri, Sep 25, 2026 at 11:40:09PM +0200, mwilck@suse.de wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Replace the line-by-line reading in lookup_wwid() with mmap()
> and a simple loop over the in-memory structures.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/wwids.c | 71 ++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index c193b97..42455de 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c

[snip]

> @@ -245,7 +250,7 @@ int check_wwids_file(const char *wwid, int write_wwid)

Since lookup_wwid now takes a fd, check_wwids_file() doesn't need the
fdopen() anymore, right?

-Ben

>  		close(fd);
>  		return -1;
>  	}
> -	found = lookup_wwid(f, wwid);
> +	found = lookup_wwid(fd, wwid);
>  	if (found) {
>  		ret = 0;
>  		goto out;
> -- 
> 2.55.0
>