Skip to contentSkip to conversation

Improve readbility of NVME "wwid" attribute

PATCH Applied Martin Wilck 6 participants 19 messages Jul 20, 2017 – Sep 6, 2017

Subsystems (3)
NVMe NVMe target Generic string library
lore
Share & tools
lore

Martin Wilck

author
#0 lore raw

v3 cover Improve readbility of NVME "wwid" attribute

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.

Changes wrt v2:
* 3/3: Make sure no underflow occurs (Joe Perches)

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(-)
Signature · 1 line
2.13.2
author
#1 lore raw

PATCH v3 1/3 string.h: add memcpy_and_pad()

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

Signed-off-by Martin Wilck <mwilck at suse.com>
---
 include/linux/string.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
include/linux/string.h +30 -0
index a467e617eeb08..0bec4151b0eb9 100644
@@ -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_ */
Signature · 1 line
2.13.2
build bot
#2 lore raw

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

url: https://github.com/0day-ci/linux/commits/Martin-Wilck/Improve-readbility-of-NVME-wwid-attribute/20170722-110309 config: x86_64-randconfig-x005-201729 (attached as .config) compiler: gcc-6…

Hi Martin,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.13-rc1 next-20170721]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Martin-Wilck/Improve-readbility-of-NVME-wwid-attribute/20170722-110309
config: x86_64-randconfig-x005-201729 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All warnings (new ones prefixed by >>):

In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4, from include/uapi/linux/posix_types.h:4, from include/uapi/linux/types.h:13, from include/linux/types.h:5, from include/linux/mod_devicetable.h:11, from scripts/mod/devicetable-offsets.c:2:

>> include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:421:2: note: in expansion of macro 'if'
     if (dest_len > count) {
     ^~
>> include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:419:2: note: in expansion of macro 'if'
     if (dest_size < dest_len)
     ^~
>> include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:416:8: note: in expansion of macro 'if'
      else if (src_size < dest_len && src_size < count)
           ^~
>> include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:414:3: note: in expansion of macro 'if'
      if (dest_size < dest_len && dest_size < count)
      ^~
>> include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:413:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(dest_len) && __builtin_constant_p(count)) {
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'strcpy' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:391:2: note: in expansion of macro 'if'
     if (p_size == (size_t)-1 && q_size == (size_t)-1)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:381:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:379:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:370:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:368:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:359:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:357:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:349:2: note: in expansion of macro 'if'
     if (p_size < size || q_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:346:3: note: in expansion of macro 'if'
      if (q_size < size)
      ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:344:3: note: in expansion of macro 'if'
      if (p_size < size)
      ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \

vim +162 include/linux/compiler.h

2bcd521a Steven Rostedt 2008-11-21  148  
2bcd521a Steven Rostedt 2008-11-21  149  #ifdef CONFIG_PROFILE_ALL_BRANCHES
2bcd521a Steven Rostedt 2008-11-21  150  /*
2bcd521a Steven Rostedt 2008-11-21  151   * "Define 'is'", Bill Clinton
2bcd521a Steven Rostedt 2008-11-21  152   * "Define 'if'", Steven Rostedt
2bcd521a Steven Rostedt 2008-11-21  153   */
ab3c9c68 Linus Torvalds 2009-04-07  154  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
ab3c9c68 Linus Torvalds 2009-04-07  155  #define __trace_if(cond) \
b33c8ff4 Arnd Bergmann  2016-02-12  156  	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
2bcd521a Steven Rostedt 2008-11-21  157  	({								\
2bcd521a Steven Rostedt 2008-11-21  158  		int ______r;						\
2bcd521a Steven Rostedt 2008-11-21  159  		static struct ftrace_branch_data			\
2bcd521a Steven Rostedt 2008-11-21  160  			__attribute__((__aligned__(4)))			\
2bcd521a Steven Rostedt 2008-11-21  161  			__attribute__((section("_ftrace_branch")))	\
2bcd521a Steven Rostedt 2008-11-21 @162  			______f = {					\
2bcd521a Steven Rostedt 2008-11-21  163  				.func = __func__,			\
2bcd521a Steven Rostedt 2008-11-21  164  				.file = __FILE__,			\
2bcd521a Steven Rostedt 2008-11-21  165  				.line = __LINE__,			\
2bcd521a Steven Rostedt 2008-11-21  166  			};						\
2bcd521a Steven Rostedt 2008-11-21  167  		______r = !!(cond);					\
97e7e4f3 Witold Baryluk 2009-03-17  168  		______f.miss_hit[______r]++;					\
2bcd521a Steven Rostedt 2008-11-21  169  		______r;						\
2bcd521a Steven Rostedt 2008-11-21  170  	}))
2bcd521a Steven Rostedt 2008-11-21  171  #endif /* CONFIG_PROFILE_ALL_BRANCHES */

2bcd521a Steven Rostedt 2008-11-21 172

:::::: The code at line 162 was first introduced by commit
:::::: 2bcd521a684cc94befbe2ce7d5b613c841b0d304 trace: profile all if conditionals

:::::: TO: Steven Rostedt <srostedt at redhat.com> :::::: CC: Ingo Molnar <mingo at elte.hu>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 26326 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170722/7326d48c/attachment-0001.gz>

build bot
#3 lore raw

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

In reply to Martin Wilck
url: https://github.com/0day-ci/linux/commits/Martin-Wilck/Improve-readbility-of-NVME-wwid-attribute/20170722-110309 config: x86_64-randconfig-v0-07240033 (attached as .config) compiler: gcc-4.4…

Hi Martin,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc1 next-20170721]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Martin-Wilck/Improve-readbility-of-NVME-wwid-attribute/20170722-110309
config: x86_64-randconfig-v0-07240033 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

cc1: warnings being treated as errors In file included from include/linux/bitmap.h:8,

                    from include/linux/cpumask.h:11,
                    from arch/x86/include/asm/cpumask.h:4,
                    from arch/x86/include/asm/msr.h:10,
                    from arch/x86/include/asm/processor.h:20,
                    from arch/x86/include/asm/cpufeature.h:4,
                    from arch/x86/include/asm/thread_info.h:52,
                    from include/linux/thread_info.h:37,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:80,
                    from include/linux/spinlock.h:50,
                    from include/linux/mmzone.h:7,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:14,
                    from include/linux/resource_ext.h:19,
                    from include/linux/acpi.h:26,
                    from drivers/gpu//drm/i915/i915_drv.c:30:
   include/linux/string.h: In function 'memcpy_and_pad':
>> include/linux/string.h:413: error: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static

include/linux/string.h:414: error: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static include/linux/string.h:416: error: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static include/linux/string.h:419: error: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static include/linux/string.h:421: error: '______f' is static but declared in inline function 'memcpy_and_pad' which is not static

vim +413 include/linux/string.h

398
399 /**
400 * memcpy_and_pad - Copy one buffer to another with padding
401 * @dest: Where to copy to
402 * @dest_len: The destination buffer size
403 * @src: Where to copy from
404 * @count: The number of bytes to copy
405 * @pad: Character to use for padding if space is left in destination.
406 */
407 __FORTIFY_INLINE void memcpy_and_pad(void *dest, size_t dest_len,
408 const void *src, size_t count, int pad)
409 {
410 size_t dest_size = __builtin_object_size(dest, 0);
411 size_t src_size = __builtin_object_size(src, 0);
412

 > 413		if (__builtin_constant_p(dest_len) && __builtin_constant_p(count)) {

414 if (dest_size < dest_len && dest_size < count)
415 __write_overflow();
416 else if (src_size < dest_len && src_size < count)
417 __read_overflow3();
418 }
419 if (dest_size < dest_len)
420 fortify_panic(__func__);
421 if (dest_len > count) {
422 memcpy(dest, src, count);
423 memset(dest + count, pad, dest_len - count);
424 } else
425 memcpy(dest, src, dest_len);
426 }
427

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 29271 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170724/b929cb26/attachment-0001.gz>

author
#4 lore raw

PATCH v3 2/3 nvmet: identify controller: improve standard compliance

In reply to 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 at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/nvme/target/admin-cmd.c +6 -7
index 35f930db3c02c..bd040ae32528d 100644
@@ -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;
Signature · 1 line
2.13.2
author
#5 lore raw

PATCH v3 3/3 nvme: wwid_show: strip trailing 0-bytes

In reply to 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. Also make sure that we get no underflow for pathological input.

Signed-off-by Martin Wilck <mwilck at suse.com>
Reviewed-by Hannes Reinecke <hare at suse.de>
Acked-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +4 -2
index cb96f4a7ae3a9..9c558ab485bbc 100644
@@ -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 (serial_len > 0 && (ctrl->serial[serial_len - 1] == ' ' ||
+				  ctrl->serial[serial_len - 1] == '\0'))
 		serial_len--;
-	while (ctrl->model[model_len - 1] == ' ')
+	while (model_len > 0 && (ctrl->model[model_len - 1] == ' ' ||
+				 ctrl->model[model_len - 1] == '\0'))
 		model_len--;
 
 	return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid,
Signature · 1 line
2.13.2
Reviewed-by on v3 3/3
#6 lore raw

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

Quoted 9 lines · 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. Also make sure that we get no underflow for pathological input.

Signed-off-by Martin Wilck <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Acked-by Christoph Hellwig <[email protected]>

Looks good.

Reviewed-by Keith Busch <[email protected]>
Applied
#7 lore raw

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

In reply to Martin Wilck

Thanks, applied to the nvme-4.13 tree.

Note that we already merged the earlier version of your target side changes, can you respin them against the latest 4.13-rc tree?

author
#8 lore raw

v4 cover Improve readbility of NVME "wwid" attribute (target side)

Hi Christoph,

I'm reposting the target-side of my patch rebased against 4.13-rc as requested.

NOTE: an error has occurred while merging the previous version of my patch. This is fixed by patch 1/3 in the series - that's an important fix for 4.13, please push forward. 2/3 and 3/3 move the "copy_and_pad" functionality to a generic helper, as requested. I've split this off in case the generic function meets criticism elsewhere.

Original cover letter:

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.

Changes wrt v2:
* 3/3: Make sure no underflow occurs (Joe Perches)

Changes wrt v3:
* Rebased on 4.13-rc3.
* Dropped client-side patch which was merged in nvme-4.13 already.
* Split off bug fix (patch 1/3).

Martin Wilck (3):
nvmet_execute_identify_ctrl: don't overwrite with 0-bytes string.h: add memcpy_and_pad() nvmet_execute_identify_ctrl: use memcpy_and_pad()

 drivers/nvme/target/admin-cmd.c | 20 +++-----------------
 include/linux/string.h          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 17 deletions(-)
Signature · 1 line
2.14.0
author
#9 lore raw

PATCH v4 1/3 nvmet_execute_identify_ctrl: don't overwrite with 0-bytes

The merged version of my patch "nvmet: don't report 0-bytes in serial number" fails to remove two lines which should have been replaced, so that the space-padded strings are overwritten again with 0-bytes. Fix it.

Fixes 42de82a8b544 nvmet: don't report 0-bytes in serial number
Signed-off-by Martin Wilck <mwilck at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/nvme/target/admin-cmd.c +0 -6
index 2d7a98ab53fbf..a53bb6635b837 100644
@@ -199,12 +199,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	copy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1);
 	copy_and_pad(id->fr, sizeof(id->fr), UTS_RELEASE, strlen(UTS_RELEASE));
 
-	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));
-
 	id->rab = 6;
 
 	/*
Signature · 1 line
2.14.0
author
#10 lore raw

PATCH v4 2/3 string.h: add memcpy_and_pad()

In reply to Martin Wilck

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

Note: the warnings reported by the kbuild test robot for this patch are actually generated by the use of CONFIG_PROFILE_ALL_BRANCHES together with __FORTIFY_INLINE.

Signed-off-by Martin Wilck <mwilck at suse.com>
---
 include/linux/string.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
include/linux/string.h +30 -0
index a467e617eeb08..0bec4151b0eb9 100644
@@ -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_ */
Signature · 1 line
2.14.0
#11 lore raw

Re: [PATCH v4 2/3] string.h: add memcpy_and_pad()

Quoted 16 lines · Martin Wilck

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

Note: the warnings reported by the kbuild test robot for this patch are actually generated by the use of CONFIG_PROFILE_ALL_BRANCHES together with __FORTIFY_INLINE.

Signed-off-by Martin Wilck <mwilck at suse.com>

---

#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)

include/linux/string.h +1 -0
@@ -395,4 +396,33 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 
 #endif
 
+/**

include/linux/string.h __FORTIFY_INLINE char *strcpy(char *p, const char *q) · line 409

Quoted 10 lines
include/linux/string.h +10 -0
+ * 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)
+{

This is causing compile-time warnings for me:

In file included from /git/arm-soc/arch/x86/include/asm/string.h:2:0,

                 from /git/arm-soc/include/linux/string.h:18,
                 from /git/arm-soc/arch/x86/include/asm/page_32.h:34,
                 from /git/arm-soc/arch/x86/include/asm/page.h:13,
                 from /git/arm-soc/arch/x86/include/asm/thread_info.h:11,
                 from /git/arm-soc/include/linux/thread_info.h:37,
                 from /git/arm-soc/arch/x86/include/asm/preempt.h:6,
                 from /git/arm-soc/include/linux/preempt.h:80,
                 from /git/arm-soc/include/linux/spinlock.h:50,
                 from /git/arm-soc/include/linux/seqlock.h:35,
                 from /git/arm-soc/include/linux/time.h:5,
                 from /git/arm-soc/include/linux/stat.h:18,
                 from /git/arm-soc/include/linux/module.h:10,
                 from /git/arm-soc/drivers/md/dm-integrity.c:9:
/git/arm-soc/arch/x86/include/asm/string_32.h:196:25: error:
'__memcpy' is static but used in inline function 'memcpy_and_pad'
which is not static [-Werror]
 #define memcpy(t, f, n) __memcpy((t), (f), (n))
                         ^~~~~~~~

/git/arm-soc/include/linux/string.h:466:3: note: in expansion of macro 'memcpy'

^
/git/arm-soc/arch/x86/include/asm/string_32.h:196:25: error:
'__memcpy' is static but used in inline function 'memcpy_and_pad' which is not static [-Werror]

 #define memcpy(t, f, n) __memcpy((t), (f), (n))
                         ^~~~~~~~

The problem is the use of __FORTIFY_INLINE outside of the #ifdef section above it.
I used an ugly local workaround, duplicating the function with a 'static inline' variant
in an #else block. Alternatively we could add an extern version in lib/string.c for the
non-fortified case.

Arnd

author
#12 lore raw

Re: [PATCH v4 2/3] string.h: add memcpy_and_pad()

Quoted 47 lines · Arnd Bergmann
Quoted 12 lines

+/**
+ * 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.
+ */

(quoted diff) +2 -0
+__FORTIFY_INLINE void memcpy_and_pad(void *dest, size_t dest_len,
+                                    const void *src, size_t count,
Quoted 2 lines

int pad)
+{

This is causing compile-time warnings for me:

In file included from /git/arm-soc/arch/x86/include/asm/string.h:2:0,

                 from /git/arm-soc/include/linux/string.h:18,
                 from /git/arm-soc/arch/x86/include/asm/page_32.h:34,
                 from /git/arm-soc/arch/x86/include/asm/page.h:13,
                 from /git/arm-
soc/arch/x86/include/asm/thread_info.h:11,
                 from /git/arm-soc/include/linux/thread_info.h:37,
                 from /git/arm-soc/arch/x86/include/asm/preempt.h:6,
                 from /git/arm-soc/include/linux/preempt.h:80,
                 from /git/arm-soc/include/linux/spinlock.h:50,
                 from /git/arm-soc/include/linux/seqlock.h:35,
                 from /git/arm-soc/include/linux/time.h:5,
                 from /git/arm-soc/include/linux/stat.h:18,
                 from /git/arm-soc/include/linux/module.h:10,
                 from /git/arm-soc/drivers/md/dm-integrity.c:9:
/git/arm-soc/arch/x86/include/asm/string_32.h:196:25: error:
'__memcpy' is static but used in inline function 'memcpy_and_pad'
which is not static [-Werror]
 #define memcpy(t, f, n) __memcpy((t), (f), (n))
                         ^~~~~~~~

/git/arm-soc/include/linux/string.h:466:3: note: in expansion of macro 'memcpy'

   ^
/git/arm-soc/arch/x86/include/asm/string_32.h:196:25: error:
'__memcpy' is static but used in inline function 'memcpy_and_pad'
which is not static [-Werror]
 #define memcpy(t, f, n) __memcpy((t), (f), (n))
                         ^~~~~~~~
Quoted 7 lines

The problem is the use of __FORTIFY_INLINE outside of the #ifdef section above it.
I used an ugly local workaround, duplicating the function with a 'static inline' variant
in an #else block. Alternatively we could add an extern version in lib/string.c for the
non-fortified case.

I'm sorry. It seems that I messed the code up by trying to do it right. I suggest to simply drop the fortification code from this function, which is not a "common str/mem function" anyway. Please tell me if that'd be ok for you.

I'll send a patch in a follow-up email.

Martin

Signature · 3 lines
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
author
#13 lore raw

PATCH 1/1 string.h: un-fortify memcpy_and_pad

In reply to Arnd Bergmann

The way I'd implemented the new helper memcpy_and_pad with __FORTIFY_INLINE caused compiler warnings for certain kernel configurations.

This helper is only used in a single place at this time, and thus doesn't benefit much from fortification. So simplify the code by dropping fortification support for now.

Fixes 3c5fa8cd18f8 "string.h: add memcpy_and_pad()"
Signed-off-by Martin Wilck <mwilck at suse.com>
---
 include/linux/string.h | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
include/linux/string.h +2 -13
index 0bec4151b0eb9..0495cd3c81689 100644
@@ -404,20 +404,9 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
  * @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)
+static 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);
Signature · 1 line
2.14.0
Acked-by on v1 1/1
#14 lore raw

Re: [PATCH] string.h: un-fortify memcpy_and_pad

Quoted 10 lines · Martin Wilck

The way I'd implemented the new helper memcpy_and_pad with __FORTIFY_INLINE caused compiler warnings for certain kernel configurations.

This helper is only used in a single place at this time, and thus doesn't benefit much from fortification. So simplify the code by dropping fortification support for now.

Fixes 3c5fa8cd18f8 "string.h: add memcpy_and_pad()"
Signed-off-by Martin Wilck <mwilck at suse.com>

Looks good to me,

Acked-by Arnd Bergmann <arnd at arndb.de>

I've added this to my randconfig testing tree, if you don't hear anything from me by tomorrow, you can assume that it caused no other failures.

Arnd

Tested-by
#15 lore raw

Re: [PATCH] string.h: un-fortify memcpy_and_pad

Quoted 12 lines · Arnd Bergmann
Quoted 10 lines · Martin Wilck

The way I'd implemented the new helper memcpy_and_pad with __FORTIFY_INLINE caused compiler warnings for certain kernel configurations.

This helper is only used in a single place at this time, and thus doesn't benefit much from fortification. So simplify the code by dropping fortification support for now.

Fixes 3c5fa8cd18f8 "string.h: add memcpy_and_pad()"
Signed-off-by Martin Wilck <mwilck at suse.com>
Quoted 6 lines

Looks good to me,

Acked-by Arnd Bergmann <arnd at arndb.de>

I've added this to my randconfig testing tree, if you don't hear anything from me by tomorrow, you can assume that it caused no other failures.

build-tested successfully.

Tested-by Arnd Bergmann <arnd at arndb.de>
author
#16 lore raw

PATCH v4 3/3 nvmet_execute_identify_ctrl: use memcpy_and_pad()

In reply to Martin Wilck

This changes the earlier patch "nvmet: don't report 0-bytes in serial number" to use the memcpy_and_pad() helper introduced in a previous patch.

Signed-off-by Martin Wilck <mwilck at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/nvme/target/admin-cmd.c +3 -11
index a53bb6635b837..7ccea863e0ab5 100644
@@ -168,15 +168,6 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void copy_and_pad(char *dst, int dst_len, const char *src, int src_len)
-{
-	int len = min(src_len, dst_len);
-
-	memcpy(dst, src, len);
-	if (dst_len > len)
-		memset(dst + len, ' ', dst_len - len);
-}
-
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;

@@ -196,8 +187,9 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	copy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1);
-	copy_and_pad(id->fr, sizeof(id->fr), UTS_RELEASE, strlen(UTS_RELEASE));
+	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;
Signature · 1 line
2.14.0
maintainer Reviewed-by on the v4 series
#17 lore raw

Re: [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side)

In reply to Martin Wilck

For the series,

Reviewed-by Sagi Grimberg <sagi at grimbeg.me>
Applied
#18 lore raw

Re: [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side)

In reply to Martin Wilck

Thanks, applied patch 1 to nvme-4.13, and the rest to nvme-4.14.

Btw, for future patches please use the driver name as prefix and not a function name.