[PATCH 0/4] libmultipath: Fixes for NVME / NVMEoF

[PATCH 0/4] libmultipath: Fixes for NVME / NVMEoF

[Martin Wilck]

Current code fails to set up multipath maps for NVME devices in a
Linux target/Linux host combination. This series enables at least
basic operation.

Patch 1/4 fixes a crash that happens if over-long WWIDs are encountered, and
is not specific to NVME as such. Patch 2/4 drops
the broken test uevent_can_discard_by_devpath(). Patch 3/4 compensates
for the additional event processing required by 2/4. Patch 4/4 mangles
overlong "nvme.*" WWIDs to make them usable for multipath (related discussion
in [1]; WWID_SIZE can't be simply increased because it has to match
device mapper's DM_NAME_LEN).

[1] http://lists.infradead.org/pipermail/linux-nvme/2017-July/011960.html

Martin Wilck (4):
  libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated
  libmultipath: drop uevent_can_discard_by_devpath
  libmultipath: only listen for uevents with DEVTYPE=disk
  libmultipath: fix over-long NVME WWIDs

 libmultipath/discovery.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/uevent.c    | 27 +---------------
 2 files changed, 81 insertions(+), 26 deletions(-)

-- 
2.13.2

[PATCH 0/4] libmultipath: Fixes for NVME / NVMEoF

[Martin Wilck]

Current code fails to set up multipath maps for NVME devices in a
Linux target/Linux host combination. This series enables at least
basic operation.

Patch 1/4 fixes a crash that happens if over-long WWIDs are encountered, and
is not specific to NVME as such. Patch 2/4 drops
the broken test uevent_can_discard_by_devpath(). Patch 3/4 compensates
for the additional event processing required by 2/4. Patch 4/4 mangles
overlong "nvme.*" WWIDs to make them usable for multipath (related discussion
in [1]; WWID_SIZE can't be simply increased because it has to match
device mapper's DM_NAME_LEN).

[1] http://lists.infradead.org/pipermail/linux-nvme/2017-July/011960.html

Martin Wilck (4):
  libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated
  libmultipath: drop uevent_can_discard_by_devpath
  libmultipath: only listen for uevents with DEVTYPE=disk
  libmultipath: fix over-long NVME WWIDs

 libmultipath/discovery.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/uevent.c    | 27 +---------------
 2 files changed, 81 insertions(+), 26 deletions(-)

-- 
2.13.2

[PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

If the first WWID_LEN bytes of the uuid_attribute do not contain
a 0 byte, pp->wwid may end up not properly terminated. Fix it.

Signed-off-by: Martin Wilck <mwilck at suse.com>
---
 libmultipath/discovery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 663c8eaa..9951af84 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 			len = strlen(value);
 		}
 		strncpy(pp->wwid, value, len);
+		pp->wwid[WWID_SIZE - 1] = '\0';
 	} else {
 		condlog(3, "%s: no %s attribute", pp->dev,
 			uid_attribute);
-- 
2.13.2

[PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

If the first WWID_LEN bytes of the uuid_attribute do not contain
a 0 byte, pp->wwid may end up not properly terminated. Fix it.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 663c8eaa..9951af84 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 			len = strlen(value);
 		}
 		strncpy(pp->wwid, value, len);
+		pp->wwid[WWID_SIZE - 1] = '\0';
 	} else {
 		condlog(3, "%s: no %s attribute", pp->dev,
 			uid_attribute);
-- 
2.13.2

[PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Martin Wilck]

This function is broken. Not all devices that matter for multipathd
follow the block/$DEVICE/$PARTITION convention (example: NVME)

Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.de>
---
 libmultipath/uevent.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 4fbd1dfb..b688ca03 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -143,36 +143,11 @@ uevent_need_merge(void)
 	return need_merge;
 }
 
-static bool
-uevent_can_discard_by_devpath(const char *devpath)
-{
-	static const char BLOCK[] = "/block/";
-	const char *tmp = strstr(devpath, BLOCK);
-
-	if (tmp == NULL) {
-		condlog(4, "no /block/ in '%s'", devpath);
-		return true;
-	}
-	tmp += sizeof(BLOCK) - 1;
-	if (*tmp == '\0')
-		/* just ".../block/" - discard */
-		return true;
-	/*
-	 * If there are more path elements after ".../block/xyz",
-	 * it's a partition - discard it; but don't discard ".../block/sda/".
-	 */
-	tmp = strchr(tmp, '/');
-	return tmp != NULL && *(tmp + 1) != '\0';
-}
-
 bool
 uevent_can_discard(struct uevent *uev)
 {
 	struct config * conf;
 
-	if (uevent_can_discard_by_devpath(uev->devpath))
-		return true;
-
 	/*
 	 * do not filter dm devices by devnode
 	 */
-- 
2.13.2

[PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Martin Wilck]

This function is broken. Not all devices that matter for multipathd
follow the block/$DEVICE/$PARTITION convention (example: NVME)

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/uevent.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 4fbd1dfb..b688ca03 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -143,36 +143,11 @@ uevent_need_merge(void)
 	return need_merge;
 }
 
-static bool
-uevent_can_discard_by_devpath(const char *devpath)
-{
-	static const char BLOCK[] = "/block/";
-	const char *tmp = strstr(devpath, BLOCK);
-
-	if (tmp == NULL) {
-		condlog(4, "no /block/ in '%s'", devpath);
-		return true;
-	}
-	tmp += sizeof(BLOCK) - 1;
-	if (*tmp == '\0')
-		/* just ".../block/" - discard */
-		return true;
-	/*
-	 * If there are more path elements after ".../block/xyz",
-	 * it's a partition - discard it; but don't discard ".../block/sda/".
-	 */
-	tmp = strchr(tmp, '/');
-	return tmp != NULL && *(tmp + 1) != '\0';
-}
-
 bool
 uevent_can_discard(struct uevent *uev)
 {
 	struct config * conf;
 
-	if (uevent_can_discard_by_devpath(uev->devpath))
-		return true;
-
 	/*
 	 * do not filter dm devices by devnode
 	 */
-- 
2.13.2

[PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Martin Wilck]

We are only interested in disks anyway. This saves us checking
for devtype in the uevent processing queue, and is more powerful
than the previous test in the dropped function
uevent_can_discard_by_devpath().

Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.de>
---
 libmultipath/uevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index b688ca03..eb44da56 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
 		goto out;
 	}
 	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
-							      NULL);
+							      "disk");
 	if (err)
 		condlog(2, "failed to create filter : %s", strerror(-err));
 	err = udev_monitor_enable_receiving(monitor);
-- 
2.13.2

[PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Martin Wilck]

We are only interested in disks anyway. This saves us checking
for devtype in the uevent processing queue, and is more powerful
than the previous test in the dropped function
uevent_can_discard_by_devpath().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/uevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index b688ca03..eb44da56 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
 		goto out;
 	}
 	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
-							      NULL);
+							      "disk");
 	if (err)
 		condlog(2, "failed to create filter : %s", strerror(-err));
 	err = udev_monitor_enable_receiving(monitor);
-- 
2.13.2

[PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Martin Wilck]

Fix for NVME wwids looking like this:
nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
which are encountered in some combinations of Linux NVME host and target,
where the 2nd field represents the serial number (SN) and the 3rd the
model number (MN).

The device mapper allows map names only up to 128 characters.
Strip the "00" sequences at the end of the serial and model field,
they are hex-encoded 0-bytes which are forbidden by the NVME spec
anyway.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 9951af84..f46b9b17 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1598,6 +1598,82 @@ get_prio (struct path * pp)
 	return 0;
 }
 
+/*
+ * Mangle string of length *len starting at start
+ * by removing character sequence "00" (hex for a 0 byte),
+ * starting at end, backwards.
+ * Changes the value of *len if characters were removed.
+ * Returns a pointer to the position where "end" was moved to.
+ */
+static char
+*skip_zeroes_backward(char* start, int *len, char *end)
+{
+	char *p = end;
+
+	while (p >= start + 2 && *(p - 1) == '0' && *(p - 2) == '0')
+		p -= 2;
+
+	if (p == end)
+		return p;
+
+	memmove(p, end, start + *len + 1 - end);
+	*len -= end - p;
+
+	return p;
+}
+
+/*
+ * Fix for NVME wwids looking like this:
+ * nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
+ * which are encountered in some combinations of Linux NVME host and target.
+ * The '00' are hex-encoded 0-bytes which are forbidden in the serial (SN)
+ * and model (MN) fields. Discard them.
+ * If a WWID of the above type is found, sets pp->wwid and returns a value > 0.
+ * Otherwise, returns 0.
+ */
+static int
+fix_broken_nvme_wwid(struct path *pp, const char *value, int size)
+{
+	static const char _nvme[] = "nvme.";
+	int len, i;
+	char mangled[256];
+	char *p;
+
+	len = strlen(value);
+	if (len >= sizeof(mangled) || len + 1 < size)
+		return 0;
+
+	/* Check that value starts with "nvme.%04x-" */
+	if (memcmp(value, _nvme, sizeof(_nvme) - 1) || value[9] != '-')
+		return 0;
+	for (i = 5; i < 9; i++)
+		if (!isxdigit(value[i]))
+			return 0;
+
+	memcpy(mangled, value, len + 1);
+
+	/* search end of "model" part and strip trailing '00' */
+	p = memrchr(mangled, '-', len);
+	if (p == NULL)
+		return 0;
+
+	p = skip_zeroes_backward(mangled, &len, p);
+
+	/* search end of "serial" part */
+	p = memrchr(mangled, '-', p - mangled);
+	if (p == NULL || memrchr(mangled, '-', p - mangled) != mangled + 9)
+	    /* We expect exactly 3 '-' in the value */
+		return 0;
+
+	p = skip_zeroes_backward(mangled, &len, p);
+	if (len >= size)
+		return 0;
+
+	memcpy(pp->wwid, mangled, len + 1);
+	condlog(2, "%s: over-long WWID shortened to %s", pp->dev, pp->wwid);
+	return len;
+}
+
 static int
 get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 {
@@ -1609,6 +1685,9 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 		value = getenv(uid_attribute);
 	if (value && strlen(value)) {
 		if (strlen(value) + 1 > WWID_SIZE) {
+			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
+			if (len > 0)
+				return len;
 			condlog(0, "%s: wwid overflow", pp->dev);
 			len = WWID_SIZE;
 		} else {
-- 
2.13.2

[PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Martin Wilck]

Fix for NVME wwids looking like this:
nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
which are encountered in some combinations of Linux NVME host and target,
where the 2nd field represents the serial number (SN) and the 3rd the
model number (MN).

The device mapper allows map names only up to 128 characters.
Strip the "00" sequences at the end of the serial and model field,
they are hex-encoded 0-bytes which are forbidden by the NVME spec
anyway.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 9951af84..f46b9b17 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1598,6 +1598,82 @@ get_prio (struct path * pp)
 	return 0;
 }
 
+/*
+ * Mangle string of length *len starting at start
+ * by removing character sequence "00" (hex for a 0 byte),
+ * starting at end, backwards.
+ * Changes the value of *len if characters were removed.
+ * Returns a pointer to the position where "end" was moved to.
+ */
+static char
+*skip_zeroes_backward(char* start, int *len, char *end)
+{
+	char *p = end;
+
+	while (p >= start + 2 && *(p - 1) == '0' && *(p - 2) == '0')
+		p -= 2;
+
+	if (p == end)
+		return p;
+
+	memmove(p, end, start + *len + 1 - end);
+	*len -= end - p;
+
+	return p;
+}
+
+/*
+ * Fix for NVME wwids looking like this:
+ * nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
+ * which are encountered in some combinations of Linux NVME host and target.
+ * The '00' are hex-encoded 0-bytes which are forbidden in the serial (SN)
+ * and model (MN) fields. Discard them.
+ * If a WWID of the above type is found, sets pp->wwid and returns a value > 0.
+ * Otherwise, returns 0.
+ */
+static int
+fix_broken_nvme_wwid(struct path *pp, const char *value, int size)
+{
+	static const char _nvme[] = "nvme.";
+	int len, i;
+	char mangled[256];
+	char *p;
+
+	len = strlen(value);
+	if (len >= sizeof(mangled) || len + 1 < size)
+		return 0;
+
+	/* Check that value starts with "nvme.%04x-" */
+	if (memcmp(value, _nvme, sizeof(_nvme) - 1) || value[9] != '-')
+		return 0;
+	for (i = 5; i < 9; i++)
+		if (!isxdigit(value[i]))
+			return 0;
+
+	memcpy(mangled, value, len + 1);
+
+	/* search end of "model" part and strip trailing '00' */
+	p = memrchr(mangled, '-', len);
+	if (p == NULL)
+		return 0;
+
+	p = skip_zeroes_backward(mangled, &len, p);
+
+	/* search end of "serial" part */
+	p = memrchr(mangled, '-', p - mangled);
+	if (p == NULL || memrchr(mangled, '-', p - mangled) != mangled + 9)
+	    /* We expect exactly 3 '-' in the value */
+		return 0;
+
+	p = skip_zeroes_backward(mangled, &len, p);
+	if (len >= size)
+		return 0;
+
+	memcpy(pp->wwid, mangled, len + 1);
+	condlog(2, "%s: over-long WWID shortened to %s", pp->dev, pp->wwid);
+	return len;
+}
+
 static int
 get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 {
@@ -1609,6 +1685,9 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 		value = getenv(uid_attribute);
 	if (value && strlen(value)) {
 		if (strlen(value) + 1 > WWID_SIZE) {
+			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
+			if (len > 0)
+				return len;
 			condlog(0, "%s: wwid overflow", pp->dev);
 			len = WWID_SIZE;
 		} else {
-- 
2.13.2

[dm-devel] [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14@13:32 +0200, Martin Wilck wrote:
> If the first WWID_LEN bytes of the uuid_attribute do not contain
> a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/discovery.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 663c8eaa..9951af84 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  			len = strlen(value);
>  		}
>  		strncpy(pp->wwid, value, len);
> +		pp->wwid[WWID_SIZE - 1] = '\0';
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);

Hi Martin,

Your patch does not cause all overflows to be reported. How about using the
following (untested) alternative?

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index eca4ce97..80d962e6 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 	if (!value || strlen(value) == 0)
 		value = getenv(uid_attribute);
 	if (value && strlen(value)) {
-		if (strlen(value) + 1 > WWID_SIZE) {
+		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >= WWID_SIZE)
 			condlog(0, "%s: wwid overflow", pp->dev);
-			len = WWID_SIZE;
-		} else {
-			len = strlen(value);
-		}
-		strncpy(pp->wwid, value, len);
 	} else {
 		condlog(3, "%s: no %s attribute", pp->dev,
 			uid_attribute);
Bart.

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14 at 13:32 +0200, Martin Wilck wrote:
> If the first WWID_LEN bytes of the uuid_attribute do not contain
> a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 663c8eaa..9951af84 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  			len = strlen(value);
>  		}
>  		strncpy(pp->wwid, value, len);
> +		pp->wwid[WWID_SIZE - 1] = '\0';
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);

Hi Martin,

Your patch does not cause all overflows to be reported. How about using the
following (untested) alternative?

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index eca4ce97..80d962e6 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 	if (!value || strlen(value) == 0)
 		value = getenv(uid_attribute);
 	if (value && strlen(value)) {
-		if (strlen(value) + 1 > WWID_SIZE) {
+		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >= WWID_SIZE)
 			condlog(0, "%s: wwid overflow", pp->dev);
-			len = WWID_SIZE;
-		} else {
-			len = strlen(value);
-		}
-		strncpy(pp->wwid, value, len);
 	} else {
 		condlog(3, "%s: no %s attribute", pp->dev,
 			uid_attribute);
Bart.

[dm-devel] [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

Hi Bart,

On Fri, 2017-07-14@14:56 +0000, Bart Van Assche wrote:
> On Fri, 2017-07-14@13:32 +0200, Martin Wilck wrote:
> > If the first WWID_LEN bytes of the uuid_attribute do not contain
> > a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/discovery.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 663c8eaa..9951af84 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char
> > *uid_attribute, struct udev_device *udev)
> >  			len = strlen(value);
> >  		}
> >  		strncpy(pp->wwid, value, len);
> > +		pp->wwid[WWID_SIZE - 1] = '\0';
> >  	} else {
> >  		condlog(3, "%s: no %s attribute", pp->dev,
> >  			uid_attribute);
> 
> Hi Martin,
> 
> Your patch does not cause all overflows to be reported.

I'm not sure what you mean. The overflow message is printed if and only
if (strlen(value) + 1 > WWID_SIZE), which is correct, AFAICS. The point
of my patch is just to avoid that multipath crashes later due to an
unterminated string caused by this overflow.

>  How about using the
> following (untested) alternative?
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index eca4ce97..80d962e6 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char
> *uid_attribute, struct udev_device *udev)
>  	if (!value || strlen(value) == 0)
>  		value = getenv(uid_attribute);
>  	if (value && strlen(value)) {
> -		if (strlen(value) + 1 > WWID_SIZE) {
> +		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >=
> WWID_SIZE)
>  			condlog(0, "%s: wwid overflow", pp->dev);
> -			len = WWID_SIZE;
> -		} else {
> -			len = strlen(value);
> -		}
> -		strncpy(pp->wwid, value, len);
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);
> Bart.

Let's have a strncpy vs. strlcpy discussion :D !

I can do this if you insist, but I don't see a big benefit. We've
tested with the patch I submitted.

Thanks,
Martin

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

Hi Bart,

On Fri, 2017-07-14 at 14:56 +0000, Bart Van Assche wrote:
> On Fri, 2017-07-14 at 13:32 +0200, Martin Wilck wrote:
> > If the first WWID_LEN bytes of the uuid_attribute do not contain
> > a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/discovery.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 663c8eaa..9951af84 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char
> > *uid_attribute, struct udev_device *udev)
> >  			len = strlen(value);
> >  		}
> >  		strncpy(pp->wwid, value, len);
> > +		pp->wwid[WWID_SIZE - 1] = '\0';
> >  	} else {
> >  		condlog(3, "%s: no %s attribute", pp->dev,
> >  			uid_attribute);
> 
> Hi Martin,
> 
> Your patch does not cause all overflows to be reported.

I'm not sure what you mean. The overflow message is printed if and only
if (strlen(value) + 1 > WWID_SIZE), which is correct, AFAICS. The point
of my patch is just to avoid that multipath crashes later due to an
unterminated string caused by this overflow.

>  How about using the
> following (untested) alternative?
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index eca4ce97..80d962e6 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char
> *uid_attribute, struct udev_device *udev)
>  	if (!value || strlen(value) == 0)
>  		value = getenv(uid_attribute);
>  	if (value && strlen(value)) {
> -		if (strlen(value) + 1 > WWID_SIZE) {
> +		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >=
> WWID_SIZE)
>  			condlog(0, "%s: wwid overflow", pp->dev);
> -			len = WWID_SIZE;
> -		} else {
> -			len = strlen(value);
> -		}
> -		strncpy(pp->wwid, value, len);
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);
> Bart.

Let's have a strncpy vs. strlcpy discussion :D !

I can do this if you insist, but I don't see a big benefit. We've
tested with the patch I submitted.

Thanks,
Martin

[dm-devel] [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14@21:21 +0200, Martin Wilck wrote:
> On Fri, 2017-07-14@14:56 +0000, Bart Van Assche wrote:
> > How about using the following (untested) alternative?
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index eca4ce97..80d962e6 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char
> > *uid_attribute, struct udev_device *udev)
> >  	if (!value || strlen(value) == 0)
> >  		value = getenv(uid_attribute);
> >  	if (value && strlen(value)) {
> > -		if (strlen(value) + 1 > WWID_SIZE) {
> > +		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >=
> > WWID_SIZE)
> >  			condlog(0, "%s: wwid overflow", pp->dev);
> > -			len = WWID_SIZE;
> > -		} else {
> > -			len = strlen(value);
> > -		}
> > -		strncpy(pp->wwid, value, len);
> >  	} else {
> >  		condlog(3, "%s: no %s attribute", pp->dev,
> >  			uid_attribute);
> 
> Let's have a strncpy vs. strlcpy discussion :D !
>
> I can do this if you insist, but I don't see a big benefit. We've
> tested with the patch I submitted.

My comments were not intended as an invitation to open a strncpy() vs. strlcpy()
discussion. What I wanted to illustrate with the above patch is that when using
strlcpy() it is not necessary to explicitly zero-terminate a string because
strlcpy() guarantees zero-termination. Compact code that is as readable as more
verbose code is always better because compact code is easier to verify.

Bart.

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14 at 21:21 +0200, Martin Wilck wrote:
> On Fri, 2017-07-14 at 14:56 +0000, Bart Van Assche wrote:
> > How about using the following (untested) alternative?
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index eca4ce97..80d962e6 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1607,13 +1607,8 @@ get_udev_uid(struct path * pp, char
> > *uid_attribute, struct udev_device *udev)
> >  	if (!value || strlen(value) == 0)
> >  		value = getenv(uid_attribute);
> >  	if (value && strlen(value)) {
> > -		if (strlen(value) + 1 > WWID_SIZE) {
> > +		if (strlcpy(pp->wwid, value, sizeof(pp->wwid)) >=
> > WWID_SIZE)
> >  			condlog(0, "%s: wwid overflow", pp->dev);
> > -			len = WWID_SIZE;
> > -		} else {
> > -			len = strlen(value);
> > -		}
> > -		strncpy(pp->wwid, value, len);
> >  	} else {
> >  		condlog(3, "%s: no %s attribute", pp->dev,
> >  			uid_attribute);
> 
> Let's have a strncpy vs. strlcpy discussion :D !
>
> I can do this if you insist, but I don't see a big benefit. We've
> tested with the patch I submitted.

My comments were not intended as an invitation to open a strncpy() vs. strlcpy()
discussion. What I wanted to illustrate with the above patch is that when using
strlcpy() it is not necessary to explicitly zero-terminate a string because
strlcpy() guarantees zero-termination. Compact code that is as readable as more
verbose code is always better because compact code is easier to verify.

Bart.

[dm-devel] [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

On Fri, 2017-07-14@20:21 +0000, Bart Van Assche wrote:
> On Fri, 2017-07-14@21:21 +0200, Martin Wilck wrote:
> > 
> > Let's have a strncpy vs. strlcpy discussion :D !
> > 
> > I can do this if you insist, but I don't see a big benefit. We've
> > tested with the patch I submitted.
> 
> My comments were not intended as an invitation to open a strncpy()
> vs. strlcpy()
> discussion. What I wanted to illustrate with the above patch is that
> when using
> strlcpy() it is not necessary to explicitly zero-terminate a string
> because
> strlcpy() guarantees zero-termination. Compact code that is as
> readable as more
> verbose code is always better because compact code is easier to
> verify.

OK. I'll wait for comments on the other patches, and change v2 of this
patch to your version.

Martin

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Martin Wilck]

On Fri, 2017-07-14 at 20:21 +0000, Bart Van Assche wrote:
> On Fri, 2017-07-14 at 21:21 +0200, Martin Wilck wrote:
> > 
> > Let's have a strncpy vs. strlcpy discussion :D !
> > 
> > I can do this if you insist, but I don't see a big benefit. We've
> > tested with the patch I submitted.
> 
> My comments were not intended as an invitation to open a strncpy()
> vs. strlcpy()
> discussion. What I wanted to illustrate with the above patch is that
> when using
> strlcpy() it is not necessary to explicitly zero-terminate a string
> because
> strlcpy() guarantees zero-termination. Compact code that is as
> readable as more
> verbose code is always better because compact code is easier to
> verify.

OK. I'll wait for comments on the other patches, and change v2 of this
patch to your version.

Martin

[dm-devel] [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14@23:21 +0200, Martin Wilck wrote:
> OK. I'll wait for comments on the other patches, and change v2 of this
> patch to your version.

Hello Martin,

Patch 1/4 of this series is the only patch I wanted to comment on. The other
patches touch code that I'm not familiar enough with to review them.

Bart.

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Bart Van Assche]

On Fri, 2017-07-14 at 23:21 +0200, Martin Wilck wrote:
> OK. I'll wait for comments on the other patches, and change v2 of this
> patch to your version.

Hello Martin,

Patch 1/4 of this series is the only patch I wanted to comment on. The other
patches touch code that I'm not familiar enough with to review them.

Bart.

[dm-devel] [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Schremmer, Steven]

> -----Original Message-----
> From: dm-devel-bounces at redhat.com [mailto:dm-devel-bounces at redhat.com] On Behalf Of Martin Wilck
> Sent: Friday, July 14, 2017 6:32 AM
> To: Christophe Varoqui <christophe.varoqui at opensvc.com>; tang.junhui at zte.com.cn; Benjamin Marzinski <bmarzins at redhat.com>;
> Hannes Reinecke <hare at suse.de>; Guan Junxiong <guanjunxiong at huawei.com>
> Cc: Martin Wilck <mwilck at suse.de>; dm-devel at redhat.com; Xose Vazquez Perez <xose.vazquez at gmail.com>; linux-
> nvme at lists.infradead.org
> Subject: [dm-devel] [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk
> 
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---

Reviewed-by: Steve Schremmer <steve.schremmer at netapp.com>

Re: [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Schremmer, Steven]

> -----Original Message-----
> From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com] On Behalf Of Martin Wilck
> Sent: Friday, July 14, 2017 6:32 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>; tang.junhui@zte.com.cn; Benjamin Marzinski <bmarzins@redhat.com>;
> Hannes Reinecke <hare@suse.de>; Guan Junxiong <guanjunxiong@huawei.com>
> Cc: Martin Wilck <mwilck@suse.de>; dm-devel@redhat.com; Xose Vazquez Perez <xose.vazquez@gmail.com>; linux-
> nvme@lists.infradead.org
> Subject: [dm-devel] [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk
> 
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---

Reviewed-by: Steve Schremmer <steve.schremmer@netapp.com>

[PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Benjamin Marzinski]

ACK, but I'm fine with Bart's patch as well.

-Ben

On Fri, Jul 14, 2017@01:32:06PM +0200, Martin Wilck wrote:
> If the first WWID_LEN bytes of the uuid_attribute do not contain
> a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/discovery.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 663c8eaa..9951af84 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  			len = strlen(value);
>  		}
>  		strncpy(pp->wwid, value, len);
> +		pp->wwid[WWID_SIZE - 1] = '\0';
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);
> -- 
> 2.13.2

Re: [PATCH 1/4] libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated

[Benjamin Marzinski]

ACK, but I'm fine with Bart's patch as well.

-Ben

On Fri, Jul 14, 2017 at 01:32:06PM +0200, Martin Wilck wrote:
> If the first WWID_LEN bytes of the uuid_attribute do not contain
> a 0 byte, pp->wwid may end up not properly terminated. Fix it.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 663c8eaa..9951af84 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1615,6 +1615,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  			len = strlen(value);
>  		}
>  		strncpy(pp->wwid, value, len);
> +		pp->wwid[WWID_SIZE - 1] = '\0';
>  	} else {
>  		condlog(3, "%s: no %s attribute", pp->dev,
>  			uid_attribute);
> -- 
> 2.13.2

[dm-devel] [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Schremmer, Steven]

> From: dm-devel-bounces at redhat.com [mailto:dm-devel-bounces at redhat.com] On Behalf Of Martin Wilck
> Sent: Friday, July 14, 2017 6:32 AM
> To: Christophe Varoqui <christophe.varoqui at opensvc.com>; tang.junhui at zte.com.cn; Benjamin Marzinski <bmarzins at redhat.com>;
> Hannes Reinecke <hare at suse.de>; Guan Junxiong <guanjunxiong at huawei.com>
> Cc: Martin Wilck <mwilck at suse.de>; dm-devel at redhat.com; Xose Vazquez Perez <xose.vazquez at gmail.com>; linux-
> nvme at lists.infradead.org
> Subject: [dm-devel] [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath
> 
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---

Reviewed-by: Steve Schremmer <steve.schremmer at netapp.com>

Re: [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Schremmer, Steven]

> From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com] On Behalf Of Martin Wilck
> Sent: Friday, July 14, 2017 6:32 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>; tang.junhui@zte.com.cn; Benjamin Marzinski <bmarzins@redhat.com>;
> Hannes Reinecke <hare@suse.de>; Guan Junxiong <guanjunxiong@huawei.com>
> Cc: Martin Wilck <mwilck@suse.de>; dm-devel@redhat.com; Xose Vazquez Perez <xose.vazquez@gmail.com>; linux-
> nvme@lists.infradead.org
> Subject: [dm-devel] [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath
> 
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---

Reviewed-by: Steve Schremmer <steve.schremmer@netapp.com>

[PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Benjamin Marzinski]

ACK

-Ben

On Fri, Jul 14, 2017@01:32:07PM +0200, Martin Wilck wrote:
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---
>  libmultipath/uevent.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 4fbd1dfb..b688ca03 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -143,36 +143,11 @@ uevent_need_merge(void)
>  	return need_merge;
>  }
>  
> -static bool
> -uevent_can_discard_by_devpath(const char *devpath)
> -{
> -	static const char BLOCK[] = "/block/";
> -	const char *tmp = strstr(devpath, BLOCK);
> -
> -	if (tmp == NULL) {
> -		condlog(4, "no /block/ in '%s'", devpath);
> -		return true;
> -	}
> -	tmp += sizeof(BLOCK) - 1;
> -	if (*tmp == '\0')
> -		/* just ".../block/" - discard */
> -		return true;
> -	/*
> -	 * If there are more path elements after ".../block/xyz",
> -	 * it's a partition - discard it; but don't discard ".../block/sda/".
> -	 */
> -	tmp = strchr(tmp, '/');
> -	return tmp != NULL && *(tmp + 1) != '\0';
> -}
> -
>  bool
>  uevent_can_discard(struct uevent *uev)
>  {
>  	struct config * conf;
>  
> -	if (uevent_can_discard_by_devpath(uev->devpath))
> -		return true;
> -
>  	/*
>  	 * do not filter dm devices by devnode
>  	 */
> -- 
> 2.13.2

Re: [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Benjamin Marzinski]

ACK

-Ben

On Fri, Jul 14, 2017 at 01:32:07PM +0200, Martin Wilck wrote:
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/uevent.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 4fbd1dfb..b688ca03 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -143,36 +143,11 @@ uevent_need_merge(void)
>  	return need_merge;
>  }
>  
> -static bool
> -uevent_can_discard_by_devpath(const char *devpath)
> -{
> -	static const char BLOCK[] = "/block/";
> -	const char *tmp = strstr(devpath, BLOCK);
> -
> -	if (tmp == NULL) {
> -		condlog(4, "no /block/ in '%s'", devpath);
> -		return true;
> -	}
> -	tmp += sizeof(BLOCK) - 1;
> -	if (*tmp == '\0')
> -		/* just ".../block/" - discard */
> -		return true;
> -	/*
> -	 * If there are more path elements after ".../block/xyz",
> -	 * it's a partition - discard it; but don't discard ".../block/sda/".
> -	 */
> -	tmp = strchr(tmp, '/');
> -	return tmp != NULL && *(tmp + 1) != '\0';
> -}
> -
>  bool
>  uevent_can_discard(struct uevent *uev)
>  {
>  	struct config * conf;
>  
> -	if (uevent_can_discard_by_devpath(uev->devpath))
> -		return true;
> -
>  	/*
>  	 * do not filter dm devices by devnode
>  	 */
> -- 
> 2.13.2

[PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Benjamin Marzinski]

ACK

-Ben

On Fri, Jul 14, 2017@01:32:08PM +0200, Martin Wilck wrote:
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---
>  libmultipath/uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index b688ca03..eb44da56 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
>  		goto out;
>  	}
>  	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
> -							      NULL);
> +							      "disk");
>  	if (err)
>  		condlog(2, "failed to create filter : %s", strerror(-err));
>  	err = udev_monitor_enable_receiving(monitor);
> -- 
> 2.13.2

Re: [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Benjamin Marzinski]

ACK

-Ben

On Fri, Jul 14, 2017 at 01:32:08PM +0200, Martin Wilck wrote:
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index b688ca03..eb44da56 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
>  		goto out;
>  	}
>  	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
> -							      NULL);
> +							      "disk");
>  	if (err)
>  		condlog(2, "failed to create filter : %s", strerror(-err));
>  	err = udev_monitor_enable_receiving(monitor);
> -- 
> 2.13.2

[PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Benjamin Marzinski]

ACK, with one small nit below.

-Ben

On Fri, Jul 14, 2017@01:32:09PM +0200, Martin Wilck wrote:
> Fix for NVME wwids looking like this:
> nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> which are encountered in some combinations of Linux NVME host and target,
> where the 2nd field represents the serial number (SN) and the 3rd the
> model number (MN).
> 
> The device mapper allows map names only up to 128 characters.
> Strip the "00" sequences at the end of the serial and model field,
> they are hex-encoded 0-bytes which are forbidden by the NVME spec
> anyway.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/discovery.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 9951af84..f46b9b17 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1598,6 +1598,82 @@ get_prio (struct path * pp)
>  	return 0;
>  }
>  
> +/*
> + * Mangle string of length *len starting at start
> + * by removing character sequence "00" (hex for a 0 byte),
> + * starting at end, backwards.
> + * Changes the value of *len if characters were removed.
> + * Returns a pointer to the position where "end" was moved to.
> + */
> +static char
> +*skip_zeroes_backward(char* start, int *len, char *end)
> +{
> +	char *p = end;
> +
> +	while (p >= start + 2 && *(p - 1) == '0' && *(p - 2) == '0')
> +		p -= 2;
> +
> +	if (p == end)
> +		return p;
> +
> +	memmove(p, end, start + *len + 1 - end);
> +	*len -= end - p;
> +
> +	return p;
> +}
> +
> +/*
> + * Fix for NVME wwids looking like this:
> + * nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> + * which are encountered in some combinations of Linux NVME host and target.
> + * The '00' are hex-encoded 0-bytes which are forbidden in the serial (SN)
> + * and model (MN) fields. Discard them.
> + * If a WWID of the above type is found, sets pp->wwid and returns a value > 0.
> + * Otherwise, returns 0.
> + */
> +static int
> +fix_broken_nvme_wwid(struct path *pp, const char *value, int size)
> +{
> +	static const char _nvme[] = "nvme.";
> +	int len, i;
> +	char mangled[256];
> +	char *p;
> +
> +	len = strlen(value);
> +	if (len >= sizeof(mangled) || len + 1 < size)
> +		return 0;

Don't we check (len + 1 < size) before calling this? It seems odd to
return that we can't do anything when in reality, we simply don't need
to do anything.

-Ben

> +
> +	/* Check that value starts with "nvme.%04x-" */
> +	if (memcmp(value, _nvme, sizeof(_nvme) - 1) || value[9] != '-')
> +		return 0;
> +	for (i = 5; i < 9; i++)
> +		if (!isxdigit(value[i]))
> +			return 0;
> +
> +	memcpy(mangled, value, len + 1);
> +
> +	/* search end of "model" part and strip trailing '00' */
> +	p = memrchr(mangled, '-', len);
> +	if (p == NULL)
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +
> +	/* search end of "serial" part */
> +	p = memrchr(mangled, '-', p - mangled);
> +	if (p == NULL || memrchr(mangled, '-', p - mangled) != mangled + 9)
> +	    /* We expect exactly 3 '-' in the value */
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +	if (len >= size)
> +		return 0;
> +
> +	memcpy(pp->wwid, mangled, len + 1);
> +	condlog(2, "%s: over-long WWID shortened to %s", pp->dev, pp->wwid);
> +	return len;
> +}
> +
>  static int
>  get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  {
> @@ -1609,6 +1685,9 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  		value = getenv(uid_attribute);
>  	if (value && strlen(value)) {
>  		if (strlen(value) + 1 > WWID_SIZE) {
> +			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
> +			if (len > 0)
> +				return len;
>  			condlog(0, "%s: wwid overflow", pp->dev);
>  			len = WWID_SIZE;
>  		} else {
> -- 
> 2.13.2

Re: [PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Benjamin Marzinski]

ACK, with one small nit below.

-Ben

On Fri, Jul 14, 2017 at 01:32:09PM +0200, Martin Wilck wrote:
> Fix for NVME wwids looking like this:
> nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> which are encountered in some combinations of Linux NVME host and target,
> where the 2nd field represents the serial number (SN) and the 3rd the
> model number (MN).
> 
> The device mapper allows map names only up to 128 characters.
> Strip the "00" sequences at the end of the serial and model field,
> they are hex-encoded 0-bytes which are forbidden by the NVME spec
> anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 9951af84..f46b9b17 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1598,6 +1598,82 @@ get_prio (struct path * pp)
>  	return 0;
>  }
>  
> +/*
> + * Mangle string of length *len starting at start
> + * by removing character sequence "00" (hex for a 0 byte),
> + * starting at end, backwards.
> + * Changes the value of *len if characters were removed.
> + * Returns a pointer to the position where "end" was moved to.
> + */
> +static char
> +*skip_zeroes_backward(char* start, int *len, char *end)
> +{
> +	char *p = end;
> +
> +	while (p >= start + 2 && *(p - 1) == '0' && *(p - 2) == '0')
> +		p -= 2;
> +
> +	if (p == end)
> +		return p;
> +
> +	memmove(p, end, start + *len + 1 - end);
> +	*len -= end - p;
> +
> +	return p;
> +}
> +
> +/*
> + * Fix for NVME wwids looking like this:
> + * nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002
> + * which are encountered in some combinations of Linux NVME host and target.
> + * The '00' are hex-encoded 0-bytes which are forbidden in the serial (SN)
> + * and model (MN) fields. Discard them.
> + * If a WWID of the above type is found, sets pp->wwid and returns a value > 0.
> + * Otherwise, returns 0.
> + */
> +static int
> +fix_broken_nvme_wwid(struct path *pp, const char *value, int size)
> +{
> +	static const char _nvme[] = "nvme.";
> +	int len, i;
> +	char mangled[256];
> +	char *p;
> +
> +	len = strlen(value);
> +	if (len >= sizeof(mangled) || len + 1 < size)
> +		return 0;

Don't we check (len + 1 < size) before calling this? It seems odd to
return that we can't do anything when in reality, we simply don't need
to do anything.

-Ben

> +
> +	/* Check that value starts with "nvme.%04x-" */
> +	if (memcmp(value, _nvme, sizeof(_nvme) - 1) || value[9] != '-')
> +		return 0;
> +	for (i = 5; i < 9; i++)
> +		if (!isxdigit(value[i]))
> +			return 0;
> +
> +	memcpy(mangled, value, len + 1);
> +
> +	/* search end of "model" part and strip trailing '00' */
> +	p = memrchr(mangled, '-', len);
> +	if (p == NULL)
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +
> +	/* search end of "serial" part */
> +	p = memrchr(mangled, '-', p - mangled);
> +	if (p == NULL || memrchr(mangled, '-', p - mangled) != mangled + 9)
> +	    /* We expect exactly 3 '-' in the value */
> +		return 0;
> +
> +	p = skip_zeroes_backward(mangled, &len, p);
> +	if (len >= size)
> +		return 0;
> +
> +	memcpy(pp->wwid, mangled, len + 1);
> +	condlog(2, "%s: over-long WWID shortened to %s", pp->dev, pp->wwid);
> +	return len;
> +}
> +
>  static int
>  get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  {
> @@ -1609,6 +1685,9 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
>  		value = getenv(uid_attribute);
>  	if (value && strlen(value)) {
>  		if (strlen(value) + 1 > WWID_SIZE) {
> +			len = fix_broken_nvme_wwid(pp, value, WWID_SIZE);
> +			if (len > 0)
> +				return len;
>  			condlog(0, "%s: wwid overflow", pp->dev);
>  			len = WWID_SIZE;
>  		} else {
> -- 
> 2.13.2

[PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Guan Junxiong]

This patch combined with PATCH 3/4 looks more elegant than my previous patch
and it works to fix the multipathd hotplug problem for NoF.
Thanks.

Reviewed-by: Guan Junxiong <guanjunxiong at huawei.com>

On 2017/7/14 19:32, Martin Wilck wrote:
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---
>  libmultipath/uevent.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 4fbd1dfb..b688ca03 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -143,36 +143,11 @@ uevent_need_merge(void)
>  	return need_merge;
>  }
>  
> -static bool
> -uevent_can_discard_by_devpath(const char *devpath)
> -{
> -	static const char BLOCK[] = "/block/";
> -	const char *tmp = strstr(devpath, BLOCK);
> -
> -	if (tmp == NULL) {
> -		condlog(4, "no /block/ in '%s'", devpath);
> -		return true;
> -	}
> -	tmp += sizeof(BLOCK) - 1;
> -	if (*tmp == '\0')
> -		/* just ".../block/" - discard */
> -		return true;
> -	/*
> -	 * If there are more path elements after ".../block/xyz",
> -	 * it's a partition - discard it; but don't discard ".../block/sda/".
> -	 */
> -	tmp = strchr(tmp, '/');
> -	return tmp != NULL && *(tmp + 1) != '\0';
> -}
> -
>  bool
>  uevent_can_discard(struct uevent *uev)
>  {
>  	struct config * conf;
>  
> -	if (uevent_can_discard_by_devpath(uev->devpath))
> -		return true;
> -
>  	/*
>  	 * do not filter dm devices by devnode
>  	 */
> 

Re: [PATCH 2/4] libmultipath: drop uevent_can_discard_by_devpath

[Guan Junxiong]

This patch combined with PATCH 3/4 looks more elegant than my previous patch
and it works to fix the multipathd hotplug problem for NoF.
Thanks.

Reviewed-by: Guan Junxiong <guanjunxiong@huawei.com>

On 2017/7/14 19:32, Martin Wilck wrote:
> This function is broken. Not all devices that matter for multipathd
> follow the block/$DEVICE/$PARTITION convention (example: NVME)
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/uevent.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 4fbd1dfb..b688ca03 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -143,36 +143,11 @@ uevent_need_merge(void)
>  	return need_merge;
>  }
>  
> -static bool
> -uevent_can_discard_by_devpath(const char *devpath)
> -{
> -	static const char BLOCK[] = "/block/";
> -	const char *tmp = strstr(devpath, BLOCK);
> -
> -	if (tmp == NULL) {
> -		condlog(4, "no /block/ in '%s'", devpath);
> -		return true;
> -	}
> -	tmp += sizeof(BLOCK) - 1;
> -	if (*tmp == '\0')
> -		/* just ".../block/" - discard */
> -		return true;
> -	/*
> -	 * If there are more path elements after ".../block/xyz",
> -	 * it's a partition - discard it; but don't discard ".../block/sda/".
> -	 */
> -	tmp = strchr(tmp, '/');
> -	return tmp != NULL && *(tmp + 1) != '\0';
> -}
> -
>  bool
>  uevent_can_discard(struct uevent *uev)
>  {
>  	struct config * conf;
>  
> -	if (uevent_can_discard_by_devpath(uev->devpath))
> -		return true;
> -
>  	/*
>  	 * do not filter dm devices by devnode
>  	 */
> 

[PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Guan Junxiong]

Looks fine.

Reviewed-by: Guan Junxiong <guanjunxiong at huawei.com>

On 2017/7/14 19:32, Martin Wilck wrote:
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> Reviewed-by: Hannes Reinecke <hare at suse.de>
> ---
>  libmultipath/uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index b688ca03..eb44da56 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
>  		goto out;
>  	}
>  	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
> -							      NULL);
> +							      "disk");
>  	if (err)
>  		condlog(2, "failed to create filter : %s", strerror(-err));
>  	err = udev_monitor_enable_receiving(monitor);
> 

Re: [PATCH 3/4] libmultipath: only listen for uevents with DEVTYPE=disk

[Guan Junxiong]

Looks fine.

Reviewed-by: Guan Junxiong <guanjunxiong@huawei.com>

On 2017/7/14 19:32, Martin Wilck wrote:
> We are only interested in disks anyway. This saves us checking
> for devtype in the uevent processing queue, and is more powerful
> than the previous test in the dropped function
> uevent_can_discard_by_devpath().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index b688ca03..eb44da56 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -770,7 +770,7 @@ int uevent_listen(struct udev *udev)
>  		goto out;
>  	}
>  	err = udev_monitor_filter_add_match_subsystem_devtype(monitor, "block",
> -							      NULL);
> +							      "disk");
>  	if (err)
>  		condlog(2, "failed to create filter : %s", strerror(-err));
>  	err = udev_monitor_enable_receiving(monitor);
> 

[PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Guan Junxiong]

Hi, Martin:

On 2017/7/14 19:32, Martin Wilck wrote:
> Fix for NVME wwids looking like this:
> nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002

I think the above line is too long and  it's better to split it into two lines.
Otherwise , this patch looks good for me.

Reviewed-by: Guan Junxiong <guanjunxiong at huawei.com>

> which are encountered in some combinations of Linux NVME host and target,
> where the 2nd field represents the serial number (SN) and the 3rd the
> model number (MN).

Re: [PATCH 4/4] libmultipath: fix over-long NVME WWIDs

[Guan Junxiong]

Hi, Martin:

On 2017/7/14 19:32, Martin Wilck wrote:
> Fix for NVME wwids looking like this:
> nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002

I think the above line is too long and  it's better to split it into two lines.
Otherwise , this patch looks good for me.

Reviewed-by: Guan Junxiong <guanjunxiong@huawei.com>

> which are encountered in some combinations of Linux NVME host and target,
> where the 2nd field represents the serial number (SN) and the 3rd the
> model number (MN).