linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] string: uninline memcpy_and_pad
@ 2021-11-02  4:30 Guenter Roeck
  2021-11-02  7:56 ` Geert Uytterhoeven
  2021-11-02 12:36 ` Andy Shevchenko
  0 siblings, 2 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-11-02  4:30 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, Andrew Morton, Guenter Roeck, Linus Torvalds,
	Geert Uytterhoeven

When building m68k:allmodconfig, recent versions of gcc generate the
following error if the length of UTS_RELEASE is less than 8 bytes.

In function 'memcpy_and_pad',
    inlined from 'nvmet_execute_disc_identify' at
    	drivers/nvme/target/discovery.c:268:2:
arch/m68k/include/asm/string.h:72:25: error:
	'__builtin_memcpy' reading 8 bytes from a region of size 7

Discussions around the problem suggest that this only happens if an
architecture does not provide strlen(), if -ffreestanding is provided as
compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
for m68k. The exact reasons are unknown, but seem to be related to the
ability of the compiler to evaluate the return value of strlen() and
the resulting execution flow in memcpy_and_pad(). It would be possible
to work around the problem by using sizeof(UTS_RELEASE) instead of
strlen(UTS_RELEASE), but that would only postpone the problem until the
function is called in a similar way. Uninline memcpy_and_pad() instead
to solve the problem for good.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/string.h | 19 ++-----------------
 lib/string.c           | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 5e96d656be7a..d68097b4f600 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -262,23 +262,8 @@ void __write_overflow(void) __compiletime_error("detected write beyond size of o
 #include <linux/fortify-string.h>
 #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.
- */
-static inline void memcpy_and_pad(void *dest, size_t dest_len,
-				  const void *src, size_t count, int pad)
-{
-	if (dest_len > count) {
-		memcpy(dest, src, count);
-		memset(dest + count, pad,  dest_len - count);
-	} else
-		memcpy(dest, src, dest_len);
-}
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad);
 
 /**
  * str_has_prefix - Test if a string has a given prefix
diff --git a/lib/string.c b/lib/string.c
index b2de45a581f4..ff13d6d77a05 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -1165,3 +1165,22 @@ void fortify_panic(const char *name)
 	BUG();
 }
 EXPORT_SYMBOL(fortify_panic);
+
+/**
+ * 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.
+ */
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad)
+{
+	if (dest_len > count) {
+		memcpy(dest, src, count);
+		memset(dest + count, pad,  dest_len - count);
+	} else
+		memcpy(dest, src, dest_len);
+}
+EXPORT_SYMBOL(memcpy_and_pad);
-- 
2.33.0


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

* Re: [PATCH] string: uninline memcpy_and_pad
  2021-11-02  4:30 [PATCH] string: uninline memcpy_and_pad Guenter Roeck
@ 2021-11-02  7:56 ` Geert Uytterhoeven
  2021-11-02 14:20   ` Guenter Roeck
  2021-11-02 12:36 ` Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2021-11-02  7:56 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andy Shevchenko, Linux Kernel Mailing List, Andrew Morton,
	Linus Torvalds, Kees Cook

Hi Günter,

Thanks for your patch!

On Tue, Nov 2, 2021 at 5:30 AM Guenter Roeck <linux@roeck-us.net> wrote:
> When building m68k:allmodconfig, recent versions of gcc generate the
> following error if the length of UTS_RELEASE is less than 8 bytes.
>
> In function 'memcpy_and_pad',
>     inlined from 'nvmet_execute_disc_identify' at
>         drivers/nvme/target/discovery.c:268:2:
> arch/m68k/include/asm/string.h:72:25: error:
>         '__builtin_memcpy' reading 8 bytes from a region of size 7
>
> Discussions around the problem suggest that this only happens if an
> architecture does not provide strlen(), if -ffreestanding is provided as
> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
> for m68k. The exact reasons are unknown, but seem to be related to the
> ability of the compiler to evaluate the return value of strlen() and
> the resulting execution flow in memcpy_and_pad(). It would be possible
> to work around the problem by using sizeof(UTS_RELEASE) instead of
> strlen(UTS_RELEASE), but that would only postpone the problem until the
> function is called in a similar way. Uninline memcpy_and_pad() instead
> to solve the problem for good.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

>  include/linux/string.h | 19 ++-----------------
>  lib/string.c           | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 17 deletions(-)

Given this now conflicts with commit cfecea6ead5f1588 ("lib/string:
Move helper functions out of string.c"). perhaps you should move this
to lib/string_helpers.c when respinning?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH] string: uninline memcpy_and_pad
  2021-11-02  4:30 [PATCH] string: uninline memcpy_and_pad Guenter Roeck
  2021-11-02  7:56 ` Geert Uytterhoeven
@ 2021-11-02 12:36 ` Andy Shevchenko
  1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-11-02 12:36 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andy Shevchenko, linux-kernel, Andrew Morton, Linus Torvalds,
	Geert Uytterhoeven

On Mon, Nov 01, 2021 at 09:30:24PM -0700, Guenter Roeck wrote:
> When building m68k:allmodconfig, recent versions of gcc generate the
> following error if the length of UTS_RELEASE is less than 8 bytes.
> 
> In function 'memcpy_and_pad',
>     inlined from 'nvmet_execute_disc_identify' at
>     	drivers/nvme/target/discovery.c:268:2:
> arch/m68k/include/asm/string.h:72:25: error:
> 	'__builtin_memcpy' reading 8 bytes from a region of size 7
> 
> Discussions around the problem suggest that this only happens if an
> architecture does not provide strlen(), if -ffreestanding is provided as
> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
> for m68k. The exact reasons are unknown, but seem to be related to the
> ability of the compiler to evaluate the return value of strlen() and
> the resulting execution flow in memcpy_and_pad(). It would be possible
> to work around the problem by using sizeof(UTS_RELEASE) instead of
> strlen(UTS_RELEASE), but that would only postpone the problem until the
> function is called in a similar way. Uninline memcpy_and_pad() instead
> to solve the problem for good.

Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  include/linux/string.h | 19 ++-----------------
>  lib/string.c           | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 5e96d656be7a..d68097b4f600 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -262,23 +262,8 @@ void __write_overflow(void) __compiletime_error("detected write beyond size of o
>  #include <linux/fortify-string.h>
>  #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.
> - */
> -static inline void memcpy_and_pad(void *dest, size_t dest_len,
> -				  const void *src, size_t count, int pad)
> -{
> -	if (dest_len > count) {
> -		memcpy(dest, src, count);
> -		memset(dest + count, pad,  dest_len - count);
> -	} else
> -		memcpy(dest, src, dest_len);
> -}
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad);
>  
>  /**
>   * str_has_prefix - Test if a string has a given prefix
> diff --git a/lib/string.c b/lib/string.c
> index b2de45a581f4..ff13d6d77a05 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -1165,3 +1165,22 @@ void fortify_panic(const char *name)
>  	BUG();
>  }
>  EXPORT_SYMBOL(fortify_panic);
> +
> +/**
> + * 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.
> + */
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad)
> +{
> +	if (dest_len > count) {
> +		memcpy(dest, src, count);
> +		memset(dest + count, pad,  dest_len - count);
> +	} else
> +		memcpy(dest, src, dest_len);
> +}
> +EXPORT_SYMBOL(memcpy_and_pad);
> -- 
> 2.33.0
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH] string: uninline memcpy_and_pad
  2021-11-02  7:56 ` Geert Uytterhoeven
@ 2021-11-02 14:20   ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-11-02 14:20 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Andy Shevchenko, Linux Kernel Mailing List, Andrew Morton,
	Linus Torvalds, Kees Cook

On 11/2/21 12:56 AM, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> Thanks for your patch!
> 
> On Tue, Nov 2, 2021 at 5:30 AM Guenter Roeck <linux@roeck-us.net> wrote:
>> When building m68k:allmodconfig, recent versions of gcc generate the
>> following error if the length of UTS_RELEASE is less than 8 bytes.
>>
>> In function 'memcpy_and_pad',
>>      inlined from 'nvmet_execute_disc_identify' at
>>          drivers/nvme/target/discovery.c:268:2:
>> arch/m68k/include/asm/string.h:72:25: error:
>>          '__builtin_memcpy' reading 8 bytes from a region of size 7
>>
>> Discussions around the problem suggest that this only happens if an
>> architecture does not provide strlen(), if -ffreestanding is provided as
>> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
>> for m68k. The exact reasons are unknown, but seem to be related to the
>> ability of the compiler to evaluate the return value of strlen() and
>> the resulting execution flow in memcpy_and_pad(). It would be possible
>> to work around the problem by using sizeof(UTS_RELEASE) instead of
>> strlen(UTS_RELEASE), but that would only postpone the problem until the
>> function is called in a similar way. Uninline memcpy_and_pad() instead
>> to solve the problem for good.
>>
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>>   include/linux/string.h | 19 ++-----------------
>>   lib/string.c           | 19 +++++++++++++++++++
>>   2 files changed, 21 insertions(+), 17 deletions(-)
> 
> Given this now conflicts with commit cfecea6ead5f1588 ("lib/string:
> Move helper functions out of string.c"). perhaps you should move this
> to lib/string_helpers.c when respinning?
> 

Will do.

Thanks,
Guenter


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

* Re: [PATCH] string: uninline memcpy_and_pad
  2021-11-02 14:24 Guenter Roeck
@ 2021-11-02 14:26 ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-11-02 14:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, Andrew Morton, Linus Torvalds, Geert Uytterhoeven,
	Andy Shevchenko

Scratch this one. Forgot v2 in subject. Resending, and sorry for the noise.

Guenter

On Tue, Nov 02, 2021 at 07:24:20AM -0700, Guenter Roeck wrote:
> When building m68k:allmodconfig, recent versions of gcc generate the
> following error if the length of UTS_RELEASE is less than 8 bytes.
> 
> In function 'memcpy_and_pad',
>     inlined from 'nvmet_execute_disc_identify' at
>     	drivers/nvme/target/discovery.c:268:2:
> arch/m68k/include/asm/string.h:72:25: error:
> 	'__builtin_memcpy' reading 8 bytes from a region of size 7
> 
> Discussions around the problem suggest that this only happens if an
> architecture does not provide strlen(), if -ffreestanding is provided as
> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
> for m68k. The exact reasons are unknown, but seem to be related to the
> ability of the compiler to evaluate the return value of strlen() and
> the resulting execution flow in memcpy_and_pad(). It would be possible
> to work around the problem by using sizeof(UTS_RELEASE) instead of
> strlen(UTS_RELEASE), but that would only postpone the problem until the
> function is called in a similar way. Uninline memcpy_and_pad() instead
> to solve the problem for good.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Moved to lib/string_helpers.c
>     Balanced { } in if/else statement to make checkpatch happy
>     Added Reviewed-by: /Acked-by: tags
> 
>  include/linux/string.h | 19 ++-----------------
>  lib/string_helpers.c   | 20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 5a36608144a9..b6572aeca2f5 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -253,23 +253,8 @@ static inline const char *kbasename(const char *path)
>  #include <linux/fortify-string.h>
>  #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.
> - */
> -static inline void memcpy_and_pad(void *dest, size_t dest_len,
> -				  const void *src, size_t count, int pad)
> -{
> -	if (dest_len > count) {
> -		memcpy(dest, src, count);
> -		memset(dest + count, pad,  dest_len - count);
> -	} else
> -		memcpy(dest, src, dest_len);
> -}
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad);
>  
>  /**
>   * memset_after - Set a value after a struct member to the end of a struct
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index faa9d8e4e2c5..d5d008f5b1d9 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -883,6 +883,26 @@ char *strreplace(char *s, char old, char new)
>  }
>  EXPORT_SYMBOL(strreplace);
>  
> +/**
> + * 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.
> + */
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad)
> +{
> +	if (dest_len > count) {
> +		memcpy(dest, src, count);
> +		memset(dest + count, pad,  dest_len - count);
> +	} else {
> +		memcpy(dest, src, dest_len);
> +	}
> +}
> +EXPORT_SYMBOL(memcpy_and_pad);
> +
>  #ifdef CONFIG_FORTIFY_SOURCE
>  void fortify_panic(const char *name)
>  {
> -- 
> 2.33.0
> 

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

* [PATCH] string: uninline memcpy_and_pad
@ 2021-11-02 14:24 Guenter Roeck
  2021-11-02 14:26 ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-11-02 14:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, Andrew Morton, Guenter Roeck, Linus Torvalds,
	Geert Uytterhoeven, Andy Shevchenko

When building m68k:allmodconfig, recent versions of gcc generate the
following error if the length of UTS_RELEASE is less than 8 bytes.

In function 'memcpy_and_pad',
    inlined from 'nvmet_execute_disc_identify' at
    	drivers/nvme/target/discovery.c:268:2:
arch/m68k/include/asm/string.h:72:25: error:
	'__builtin_memcpy' reading 8 bytes from a region of size 7

Discussions around the problem suggest that this only happens if an
architecture does not provide strlen(), if -ffreestanding is provided as
compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
for m68k. The exact reasons are unknown, but seem to be related to the
ability of the compiler to evaluate the return value of strlen() and
the resulting execution flow in memcpy_and_pad(). It would be possible
to work around the problem by using sizeof(UTS_RELEASE) instead of
strlen(UTS_RELEASE), but that would only postpone the problem until the
function is called in a similar way. Uninline memcpy_and_pad() instead
to solve the problem for good.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Moved to lib/string_helpers.c
    Balanced { } in if/else statement to make checkpatch happy
    Added Reviewed-by: /Acked-by: tags

 include/linux/string.h | 19 ++-----------------
 lib/string_helpers.c   | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 5a36608144a9..b6572aeca2f5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -253,23 +253,8 @@ static inline const char *kbasename(const char *path)
 #include <linux/fortify-string.h>
 #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.
- */
-static inline void memcpy_and_pad(void *dest, size_t dest_len,
-				  const void *src, size_t count, int pad)
-{
-	if (dest_len > count) {
-		memcpy(dest, src, count);
-		memset(dest + count, pad,  dest_len - count);
-	} else
-		memcpy(dest, src, dest_len);
-}
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad);
 
 /**
  * memset_after - Set a value after a struct member to the end of a struct
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index faa9d8e4e2c5..d5d008f5b1d9 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -883,6 +883,26 @@ char *strreplace(char *s, char old, char new)
 }
 EXPORT_SYMBOL(strreplace);
 
+/**
+ * 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.
+ */
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad)
+{
+	if (dest_len > count) {
+		memcpy(dest, src, count);
+		memset(dest + count, pad,  dest_len - count);
+	} else {
+		memcpy(dest, src, dest_len);
+	}
+}
+EXPORT_SYMBOL(memcpy_and_pad);
+
 #ifdef CONFIG_FORTIFY_SOURCE
 void fortify_panic(const char *name)
 {
-- 
2.33.0


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

end of thread, other threads:[~2021-11-02 14:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-02  4:30 [PATCH] string: uninline memcpy_and_pad Guenter Roeck
2021-11-02  7:56 ` Geert Uytterhoeven
2021-11-02 14:20   ` Guenter Roeck
2021-11-02 12:36 ` Andy Shevchenko
2021-11-02 14:24 Guenter Roeck
2021-11-02 14:26 ` Guenter Roeck

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