[PATCH v2 0/3] Improve readbility of NVME "wwid" attribute

[PATCH v2 0/3] Improve readbility of NVME "wwid" attribute

[Martin Wilck]

With the current implementation, the default "fallback" WWID generation
code (if no nguid, euid etc. are defined) for Linux NVME host and target
results in the following WWID format:

nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002

This is not only hard to read, it poses real problems e.g. for multipath
(dm WWIDs are limited to 128 characters).

With this patch series, the WWID on a Linux host connected to a Linux target
looks like this:

nvme.0000-65613435333665653738613464363961-4c696e7578-00000001

Changes wrt v1:
 * 1/3: new, moved helper to include/linux/string.h (Christoph Hellwig)
        (you suggested kernel.h, but I think this matches string.h better)      
 * Dropped the last patch from the v1 series that would have changed valid WWIDs for
   HW NVME controllers.

Martin Wilck (3):
  string.h: add memcpy_and_pad()
  nvmet: identify controller: improve standard compliance
  nvme: wwid_show: strip trailing 0-bytes

 drivers/nvme/host/core.c        |  6 ++++--
 drivers/nvme/target/admin-cmd.c | 13 ++++++-------
 include/linux/string.h          | 30 ++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.13.2

[PATCH v2 1/3] string.h: add memcpy_and_pad()

[Martin Wilck]

This helper function is useful for the nvme subsystem, and maybe
others.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 include/linux/string.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index a467e617eeb08..0bec4151b0eb9 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -200,6 +200,7 @@ static inline const char *kbasename(const char *path)
 void fortify_panic(const char *name) __noreturn __cold;
 void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
 void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
+void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
 void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
 
 #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
@@ -395,4 +396,33 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 
 #endif
 
+/**
+ * memcpy_and_pad - Copy one buffer to another with padding
+ * @dest: Where to copy to
+ * @dest_len: The destination buffer size
+ * @src: Where to copy from
+ * @count: The number of bytes to copy
+ * @pad: Character to use for padding if space is left in destination.
+ */
+__FORTIFY_INLINE void memcpy_and_pad(void *dest, size_t dest_len,
+				     const void *src, size_t count, int pad)
+{
+	size_t dest_size = __builtin_object_size(dest, 0);
+	size_t src_size = __builtin_object_size(src, 0);
+
+	if (__builtin_constant_p(dest_len) && __builtin_constant_p(count)) {
+		if (dest_size < dest_len && dest_size < count)
+			__write_overflow();
+		else if (src_size < dest_len && src_size < count)
+			__read_overflow3();
+	}
+	if (dest_size < dest_len)
+		fortify_panic(__func__);
+	if (dest_len > count) {
+		memcpy(dest, src, count);
+		memset(dest + count, pad,  dest_len - count);
+	} else
+		memcpy(dest, src, dest_len);
+}
+
 #endif /* _LINUX_STRING_H_ */
-- 
2.13.2

[PATCH v2 2/3] nvmet: identify controller: improve standard compliance

[Martin Wilck]

The NVME standard mandates that the SN, MN, and FR fields of
the Indentify Controller Data Structure be "ASCII strings".
That means that they may not contain 0-bytes, not even string
terminators.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/nvme/target/admin-cmd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 35f930db3c02c..bd040ae32528d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -173,6 +173,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
+	const char MODEL[] = "Linux";
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -184,14 +185,12 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->vid = 0;
 	id->ssvid = 0;
 
-	memset(id->sn, ' ', sizeof(id->sn));
-	snprintf(id->sn, sizeof(id->sn), "%llx", ctrl->serial);
+	bin2hex(id->sn, &ctrl->serial, min(sizeof(ctrl->serial),
+					   sizeof(id->sn) / 2));
 
-	memset(id->mn, ' ', sizeof(id->mn));
-	strncpy((char *)id->mn, "Linux", sizeof(id->mn));
-
-	memset(id->fr, ' ', sizeof(id->fr));
-	strncpy((char *)id->fr, UTS_RELEASE, sizeof(id->fr));
+	memcpy_and_pad(id->mn, sizeof(id->mn), MODEL, sizeof(MODEL) - 1, ' ');
+	memcpy_and_pad(id->fr, sizeof(id->fr),
+		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
 	id->rab = 6;
 
-- 
2.13.2

[PATCH v2 3/3] nvme: wwid_show: strip trailing 0-bytes

[Martin Wilck]

Some broken targets (such as the current Linux target) pad
model or serial fields with 0-bytes rather than spaces. The
NVME spec disallows 0 bytes in "ASCII" fields.
Thus strip trailing 0-bytes, too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Acked-by: Christoph Hellwig <hch@lst.de>

---
 drivers/nvme/host/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cb96f4a7ae3a9..30b87600157d7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2001,9 +2001,11 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 	if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
 		return sprintf(buf, "eui.%8phN\n", ns->eui);
 
-	while (ctrl->serial[serial_len - 1] == ' ')
+	while (ctrl->serial[serial_len - 1] == ' ' ||
+	       ctrl->serial[serial_len - 1] == '\0')
 		serial_len--;
-	while (ctrl->model[model_len - 1] == ' ')
+	while (ctrl->model[model_len - 1] == ' ' ||
+	       ctrl->model[model_len - 1] == '\0')
 		model_len--;
 
 	return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid,
-- 
2.13.2

Re: [PATCH v2 3/3] nvme: wwid_show: strip trailing 0-bytes

[Joe Perches]

On Thu, 2017-07-20 at 17:27 +0200, Martin Wilck wrote:
> Some broken targets (such as the current Linux target) pad
> model or serial fields with 0-bytes rather than spaces. The
> NVME spec disallows 0 bytes in "ASCII" fields.
> Thus strip trailing 0-bytes, too.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Acked-by: Christoph Hellwig <hch@lst.de>
> 
> ---
>  drivers/nvme/host/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
[]
> @@ -2001,9 +2001,11 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
>  	if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
>  		return sprintf(buf, "eui.%8phN\n", ns->eui);
>  
> -	while (ctrl->serial[serial_len - 1] == ' ')
> +	while (ctrl->serial[serial_len - 1] == ' ' ||
> +	       ctrl->serial[serial_len - 1] == '\0')
>  		serial_len--;
> -	while (ctrl->model[model_len - 1] == ' ')
> +	while (ctrl->model[model_len - 1] == ' ' ||
> +	       ctrl->model[model_len - 1] == '\0')
>  		model_len--;

Please add a <foo>_len > 0 to the while loops too