linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Improve readbility of NVME "wwid" attribute
@ 2017-07-20 16:33 Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Martin Wilck @ 2017-07-20 16:33 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

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(-)

-- 
2.13.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3 1/3] string.h: add memcpy_and_pad()
  2017-07-20 16:33 [PATCH v3 0/3] Improve readbility of NVME "wwid" attribute Martin Wilck
@ 2017-07-20 16:34 ` Martin Wilck
  2017-07-22  3:45   ` kbuild test robot
  2017-07-23 18:18   ` kbuild test robot
  2017-07-20 16:34 ` [PATCH v3 2/3] nvmet: identify controller: improve standard compliance Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes Martin Wilck
  2 siblings, 2 replies; 19+ messages in thread
From: Martin Wilck @ 2017-07-20 16:34 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 2/3] nvmet: identify controller: improve standard compliance
  2017-07-20 16:33 [PATCH v3 0/3] Improve readbility of NVME "wwid" attribute Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
@ 2017-07-20 16:34 ` Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes Martin Wilck
  2 siblings, 0 replies; 19+ messages in thread
From: Martin Wilck @ 2017-07-20 16:34 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes
  2017-07-20 16:33 [PATCH v3 0/3] Improve readbility of NVME "wwid" attribute Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
  2017-07-20 16:34 ` [PATCH v3 2/3] nvmet: identify controller: improve standard compliance Martin Wilck
@ 2017-07-20 16:34 ` Martin Wilck
  2017-07-20 20:11   ` Keith Busch
  2017-08-10  8:45   ` Christoph Hellwig
  2 siblings, 2 replies; 19+ messages in thread
From: Martin Wilck @ 2017-07-20 16:34 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

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@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..9c558ab485bbc 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 (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,
-- 
2.13.2

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes
  2017-07-20 16:34 ` [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes Martin Wilck
@ 2017-07-20 20:11   ` Keith Busch
  2017-08-10  8:45   ` Christoph Hellwig
  1 sibling, 0 replies; 19+ messages in thread
From: Keith Busch @ 2017-07-20 20:11 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Sagi Grimberg, Martin Wilck,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme, linux-kernel,
	joe

On Thu, Jul 20, 2017 at 06:34:02PM +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. Also make sure that we get no
> underflow for pathological input.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Acked-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Keith Busch <keith.busch@intel.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 1/3] string.h: add memcpy_and_pad()
  2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
@ 2017-07-22  3:45   ` kbuild test robot
  2017-07-23 18:18   ` kbuild test robot
  1 sibling, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2017-07-22  3:45 UTC (permalink / raw)
  To: Martin Wilck
  Cc: kbuild-all, Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

[-- Attachment #1: Type: text/plain, Size: 10069 bytes --]

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@redhat.com>
:::::: CC: Ingo Molnar <mingo@elte.hu>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26326 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 1/3] string.h: add memcpy_and_pad()
  2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
  2017-07-22  3:45   ` kbuild test robot
@ 2017-07-23 18:18   ` kbuild test robot
  1 sibling, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2017-07-23 18:18 UTC (permalink / raw)
  To: Martin Wilck
  Cc: kbuild-all, Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel, joe

[-- Attachment #1: Type: text/plain, Size: 3557 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29271 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes
  2017-07-20 16:34 ` [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes Martin Wilck
  2017-07-20 20:11   ` Keith Busch
@ 2017-08-10  8:45   ` Christoph Hellwig
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
  1 sibling, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2017-08-10  8:45 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Martin Wilck,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme, linux-kernel,
	joe

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?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side)
  2017-08-10  8:45   ` Christoph Hellwig
@ 2017-08-14 20:12     ` Martin Wilck
  2017-08-14 20:12       ` [PATCH v4 1/3] nvmet_execute_identify_ctrl: don't overwrite with 0-bytes Martin Wilck
                         ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Martin Wilck @ 2017-08-14 20:12 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel

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(-)

-- 
2.14.0

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/3] nvmet_execute_identify_ctrl: don't overwrite with 0-bytes
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
@ 2017-08-14 20:12       ` Martin Wilck
  2017-08-14 20:12       ` [PATCH v4 2/3] string.h: add memcpy_and_pad() Martin Wilck
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 19+ messages in thread
From: Martin Wilck @ 2017-08-14 20:12 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel

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@suse.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 2d7a98ab53fbf..a53bb6635b837 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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;
 
 	/*
-- 
2.14.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 2/3] string.h: add memcpy_and_pad()
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
  2017-08-14 20:12       ` [PATCH v4 1/3] nvmet_execute_identify_ctrl: don't overwrite with 0-bytes Martin Wilck
@ 2017-08-14 20:12       ` Martin Wilck
  2017-09-05  7:28         ` Arnd Bergmann
  2017-08-14 20:12       ` [PATCH v4 3/3] nvmet_execute_identify_ctrl: use memcpy_and_pad() Martin Wilck
                         ` (2 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Martin Wilck @ 2017-08-14 20:12 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel

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@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.14.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 3/3] nvmet_execute_identify_ctrl: use memcpy_and_pad()
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
  2017-08-14 20:12       ` [PATCH v4 1/3] nvmet_execute_identify_ctrl: don't overwrite with 0-bytes Martin Wilck
  2017-08-14 20:12       ` [PATCH v4 2/3] string.h: add memcpy_and_pad() Martin Wilck
@ 2017-08-14 20:12       ` Martin Wilck
  2017-08-15  9:10       ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Sagi Grimberg
  2017-08-16  8:12       ` Christoph Hellwig
  4 siblings, 0 replies; 19+ messages in thread
From: Martin Wilck @ 2017-08-14 20:12 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel

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@suse.com>
---
 drivers/nvme/target/admin-cmd.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index a53bb6635b837..7ccea863e0ab5 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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;
 
-- 
2.14.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side)
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
                         ` (2 preceding siblings ...)
  2017-08-14 20:12       ` [PATCH v4 3/3] nvmet_execute_identify_ctrl: use memcpy_and_pad() Martin Wilck
@ 2017-08-15  9:10       ` Sagi Grimberg
  2017-08-16  8:12       ` Christoph Hellwig
  4 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2017-08-15  9:10 UTC (permalink / raw)
  To: Martin Wilck, Christoph Hellwig, Keith Busch
  Cc: Martin Wilck, Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	linux-kernel

For the series,

Reviewed-by: Sagi Grimberg <sagi@grimbeg.me>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side)
  2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
                         ` (3 preceding siblings ...)
  2017-08-15  9:10       ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Sagi Grimberg
@ 2017-08-16  8:12       ` Christoph Hellwig
  4 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2017-08-16  8:12 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Martin Wilck,
	linux-kernel, Hannes Reinecke, linux-nvme, Johannes Thumshirn

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] string.h: add memcpy_and_pad()
  2017-08-14 20:12       ` [PATCH v4 2/3] string.h: add memcpy_and_pad() Martin Wilck
@ 2017-09-05  7:28         ` Arnd Bergmann
  2017-09-05 18:18           ` Martin Wilck
  2017-09-05 18:23           ` [PATCH] string.h: un-fortify memcpy_and_pad Martin Wilck
  0 siblings, 2 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-09-05  7:28 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Martin Wilck,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	Linux Kernel Mailing List

On Mon, Aug 14, 2017 at 10:12 PM, Martin Wilck <mwilck@suse.com> wrote:
> 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@suse.com>
> ---

>  #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)
> +{

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/3] string.h: add memcpy_and_pad()
  2017-09-05  7:28         ` Arnd Bergmann
@ 2017-09-05 18:18           ` Martin Wilck
  2017-09-05 18:23           ` [PATCH] string.h: un-fortify memcpy_and_pad Martin Wilck
  1 sibling, 0 replies; 19+ messages in thread
From: Martin Wilck @ 2017-09-05 18:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	Linux Kernel Mailing List

On Tue, 2017-09-05 at 09:28 +0200, Arnd Bergmann wrote:
> 
> > +/**
> > + * 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.

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

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] string.h: un-fortify memcpy_and_pad
  2017-09-05  7:28         ` Arnd Bergmann
  2017-09-05 18:18           ` Martin Wilck
@ 2017-09-05 18:23           ` Martin Wilck
  2017-09-05 19:26             ` Arnd Bergmann
  1 sibling, 1 reply; 19+ messages in thread
From: Martin Wilck @ 2017-09-05 18:23 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: arndbergmann, Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme, linux-kernel

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@suse.com>
---
 include/linux/string.h | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 0bec4151b0eb9..0495cd3c81689 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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);
-- 
2.14.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] string.h: un-fortify memcpy_and_pad
  2017-09-05 18:23           ` [PATCH] string.h: un-fortify memcpy_and_pad Martin Wilck
@ 2017-09-05 19:26             ` Arnd Bergmann
  2017-09-06 13:02               ` Arnd Bergmann
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-09-05 19:26 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	Linux Kernel Mailing List

On Tue, Sep 5, 2017 at 8:23 PM, Martin Wilck <mwilck@suse.com> wrote:
> 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@suse.com>

Looks good to me,

Acked-by: Arnd Bergmann <arnd@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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] string.h: un-fortify memcpy_and_pad
  2017-09-05 19:26             ` Arnd Bergmann
@ 2017-09-06 13:02               ` Arnd Bergmann
  0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-09-06 13:02 UTC (permalink / raw)
  To: Martin Wilck
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Johannes Thumshirn, Hannes Reinecke, linux-nvme,
	Linux Kernel Mailing List

On Tue, Sep 5, 2017 at 9:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Sep 5, 2017 at 8:23 PM, Martin Wilck <mwilck@suse.com> wrote:
>> 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@suse.com>
>
> Looks good to me,
>
> Acked-by: Arnd Bergmann <arnd@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@arndb.de>

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-09-06 13:02 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-20 16:33 [PATCH v3 0/3] Improve readbility of NVME "wwid" attribute Martin Wilck
2017-07-20 16:34 ` [PATCH v3 1/3] string.h: add memcpy_and_pad() Martin Wilck
2017-07-22  3:45   ` kbuild test robot
2017-07-23 18:18   ` kbuild test robot
2017-07-20 16:34 ` [PATCH v3 2/3] nvmet: identify controller: improve standard compliance Martin Wilck
2017-07-20 16:34 ` [PATCH v3 3/3] nvme: wwid_show: strip trailing 0-bytes Martin Wilck
2017-07-20 20:11   ` Keith Busch
2017-08-10  8:45   ` Christoph Hellwig
2017-08-14 20:12     ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Martin Wilck
2017-08-14 20:12       ` [PATCH v4 1/3] nvmet_execute_identify_ctrl: don't overwrite with 0-bytes Martin Wilck
2017-08-14 20:12       ` [PATCH v4 2/3] string.h: add memcpy_and_pad() Martin Wilck
2017-09-05  7:28         ` Arnd Bergmann
2017-09-05 18:18           ` Martin Wilck
2017-09-05 18:23           ` [PATCH] string.h: un-fortify memcpy_and_pad Martin Wilck
2017-09-05 19:26             ` Arnd Bergmann
2017-09-06 13:02               ` Arnd Bergmann
2017-08-14 20:12       ` [PATCH v4 3/3] nvmet_execute_identify_ctrl: use memcpy_and_pad() Martin Wilck
2017-08-15  9:10       ` [PATCH v4 0/3] Improve readbility of NVME "wwid" attribute (target side) Sagi Grimberg
2017-08-16  8:12       ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).