* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 2:09 ` [PATCH v2] " Nick Desaulniers
@ 2020-08-15 2:58 ` Arvind Sankar
2020-08-15 3:42 ` Joe Perches
` (2 subsequent siblings)
3 siblings, 0 replies; 31+ messages in thread
From: Arvind Sankar @ 2020-08-15 2:58 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Andrew Morton, Dávid Bolvanský,
Eli Friedman, stable, Arvind Sankar, Joe Perches, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Kees Cook, Ingo Molnar, Yury Norov,
Alexandru Ardelean, linux-kernel, clang-built-linux
On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> LLVM implemented a recent "libcall optimization" that lowers calls to
> `sprintf(dest, "%s", str)` where the return value is used to
> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> in parsing format strings. Calling `sprintf` with overlapping arguments
> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>
> `stpcpy` is just like `strcpy` except it returns the pointer to the new
> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> one statement.
>
> `stpcpy` was first standardized in POSIX.1-2008.
>
> Implement this so that we don't observe linkage failures due to missing
> symbol definitions for `stpcpy`.
>
> Similar to last year's fire drill with:
> commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
>
> This optimization was introduced into clang-12.
>
> Cc: stable@vger.kernel.org
> Link: https://bugs.llvm.org/show_bug.cgi?id=47162
> Link: https://github.com/ClangBuiltLinux/linux/issues/1126
> Link: https://man7.org/linux/man-pages/man3/stpcpy.3.html
> Link: https://pubs.opengroup.org/onlinepubs/9699919799/functions/stpcpy.html
> Link: https://reviews.llvm.org/D85963
> Suggested-by: Arvind Sankar <nivedita@alum.mit.edu>
> Suggested-by: Joe Perches <joe@perches.com>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Tested-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V2:
> * Added Sami's Tested by; though the patch changed implementation, the
> missing symbol at link time was the problem Sami was observing.
> * Fix __restrict -> __restrict__ typo as per Joe.
> * Drop note about restrict from commit message as per Arvind.
> * Fix NULL -> NUL as per Arvind; NUL is ASCII '\0'. TIL
> * Fix off by one error as per Arvind; I had another off by one error in
> my test program that was masking this.
>
> include/linux/string.h | 3 +++
> lib/string.c | 23 +++++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b1f3894a0a3e..7686dbca8582 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
> #ifndef __HAVE_ARCH_STRSCPY
> ssize_t strscpy(char *, const char *, size_t);
> #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +extern char *stpcpy(char *__restrict__, const char *__restrict__);
> +#endif
>
> /* Wraps calls to strscpy()/memset(), no arch specific code required */
> ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> diff --git a/lib/string.c b/lib/string.c
> index 6012c385fb31..68ddbffbbd58 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -272,6 +272,29 @@ ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> }
> EXPORT_SYMBOL(strscpy_pad);
>
> +#ifndef __HAVE_ARCH_STPCPY
> +/**
> + * stpcpy - copy a string from src to dest returning a pointer to the new end
> + * of dest, including src's NUL terminator. May overrun dest.
> + * @dest: pointer to end of string being copied into. Must be large enough
> + * to receive copy.
> + * @src: pointer to the beginning of string being copied from. Must not overlap
> + * dest.
> + *
> + * stpcpy differs from strcpy in two key ways:
> + * 1. inputs must not overlap.
Looks like you missed my second email: strcpy also does not allow inputs
to overlap. Couple typos below.
> + * 2. return value is the new NULL terminated character. (for strcpy, the
^^ NUL terminator.
> + * return value is a pointer to src.
^^ dest.)
> + */
> +#undef stpcpy
> +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
> +{
> + while ((*dest++ = *src++) != '\0')
> + /* nothing */;
> + return --dest;
> +}
> +#endif
> +
> #ifndef __HAVE_ARCH_STRCAT
> /**
> * strcat - Append one %NUL-terminated string to another
> --
> 2.28.0.220.ged08abb693-goog
>
The kernel-doc comments in string.c currently have a mix of %NUL and
NUL, but the former seems to be more common. %NUL-terminator appears to
be the preferred wording.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 2:09 ` [PATCH v2] " Nick Desaulniers
2020-08-15 2:58 ` Arvind Sankar
@ 2020-08-15 3:42 ` Joe Perches
2020-08-15 16:34 ` Kees Cook
2020-08-16 7:44 ` kernel test robot
3 siblings, 0 replies; 31+ messages in thread
From: Joe Perches @ 2020-08-15 3:42 UTC (permalink / raw)
To: Nick Desaulniers, Andrew Morton
Cc: Dávid Bolvanský,
Eli Friedman, stable, Arvind Sankar, Sami Tolvanen, Vishal Verma,
Dan Williams, Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Kees Cook, Ingo Molnar, Yury Norov,
Alexandru Ardelean, linux-kernel, clang-built-linux
On Fri, 2020-08-14 at 19:09 -0700, Nick Desaulniers wrote:
> LLVM implemented a recent "libcall optimization" that lowers calls to
> `sprintf(dest, "%s", str)` where the return value is used to
> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> in parsing format strings. Calling `sprintf` with overlapping arguments
> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>
> `stpcpy` is just like `strcpy` except it returns the pointer to the new
> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> one statement.
[]
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
> #ifndef __HAVE_ARCH_STRSCPY
> ssize_t strscpy(char *, const char *, size_t);
> #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +extern char *stpcpy(char *__restrict__, const char *__restrict__);
If __restrict__ is used, perhaps it should follow the
kernel style used by attributes like __iomem and __user
extern char *stpcpy(char __restrict *dest, const char __restrict *src);
(though I would lose the extern too)
char *stpcpy(char __restrict *dest, const char __restrict *src);
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 2:09 ` [PATCH v2] " Nick Desaulniers
2020-08-15 2:58 ` Arvind Sankar
2020-08-15 3:42 ` Joe Perches
@ 2020-08-15 16:34 ` Kees Cook
2020-08-15 17:38 ` Dávid Bolvanský
2020-08-15 20:47 ` Nick Desaulniers
2020-08-16 7:44 ` kernel test robot
3 siblings, 2 replies; 31+ messages in thread
From: Kees Cook @ 2020-08-15 16:34 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Andrew Morton, Dávid Bolvanský,
Eli Friedman, stable, Arvind Sankar, Joe Perches, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean,
linux-kernel, clang-built-linux
On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> LLVM implemented a recent "libcall optimization" that lowers calls to
> `sprintf(dest, "%s", str)` where the return value is used to
> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> in parsing format strings. Calling `sprintf` with overlapping arguments
> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>
> `stpcpy` is just like `strcpy` except it returns the pointer to the new
> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> one statement.
O_O What?
No; this is a _terrible_ API: there is no bounds checking, there are no
buffer sizes. Anything using the example sprintf() pattern is _already_
wrong and must be removed from the kernel. (Yes, I realize that the
kernel is *filled* with this bad assumption that "I'll never write more
than PAGE_SIZE bytes to this buffer", but that's both theoretically
wrong ("640k is enough for anybody") and has been known to be wrong in
practice too (e.g. when suddenly your writing routine is reachable by
splice(2) and you may not have a PAGE_SIZE buffer).
But we cannot _add_ another dangerous string API. We're already in a
terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
needs to be addressed up by removing the unbounded sprintf() uses. (And
to do so without introducing bugs related to using snprintf() when
scnprintf() is expected[4].)
-Kees
[1] https://github.com/KSPP/linux/issues/88
[2] https://github.com/KSPP/linux/issues/89
[3] https://github.com/KSPP/linux/issues/90
[4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/
--
Kees Cook
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 16:34 ` Kees Cook
@ 2020-08-15 17:38 ` Dávid Bolvanský
2020-08-15 20:47 ` Nick Desaulniers
1 sibling, 0 replies; 31+ messages in thread
From: Dávid Bolvanský @ 2020-08-15 17:38 UTC (permalink / raw)
To: Kees Cook
Cc: Nick Desaulniers, Andrew Morton, Eli Friedman, stable,
Arvind Sankar, Joe Perches, Sami Tolvanen, Vishal Verma,
Dan Williams, Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean,
linux-kernel, clang-built-linux
Yeah, sprintf calls should be replaced with something safer.
> Dňa 15. 8. 2020 o 18:34 užívateľ Kees Cook <keescook@chromium.org> napísal:
>
> On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
>> LLVM implemented a recent "libcall optimization" that lowers calls to
>> `sprintf(dest, "%s", str)` where the return value is used to
>> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
>> in parsing format strings. Calling `sprintf` with overlapping arguments
>> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>>
>> `stpcpy` is just like `strcpy` except it returns the pointer to the new
>> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
>> one statement.
>
> O_O What?
>
> No; this is a _terrible_ API: there is no bounds checking, there are no
> buffer sizes. Anything using the example sprintf() pattern is _already_
> wrong and must be removed from the kernel. (Yes, I realize that the
> kernel is *filled* with this bad assumption that "I'll never write more
> than PAGE_SIZE bytes to this buffer", but that's both theoretically
> wrong ("640k is enough for anybody") and has been known to be wrong in
> practice too (e.g. when suddenly your writing routine is reachable by
> splice(2) and you may not have a PAGE_SIZE buffer).
>
> But we cannot _add_ another dangerous string API. We're already in a
> terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> needs to be addressed up by removing the unbounded sprintf() uses. (And
> to do so without introducing bugs related to using snprintf() when
> scnprintf() is expected[4].)
>
> -Kees
>
> [1] https://github.com/KSPP/linux/issues/88
> [2] https://github.com/KSPP/linux/issues/89
> [3] https://github.com/KSPP/linux/issues/90
> [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 16:34 ` Kees Cook
2020-08-15 17:38 ` Dávid Bolvanský
@ 2020-08-15 20:47 ` Nick Desaulniers
2020-08-15 21:23 ` Joe Perches
1 sibling, 1 reply; 31+ messages in thread
From: Nick Desaulniers @ 2020-08-15 20:47 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Joe Perches, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > LLVM implemented a recent "libcall optimization" that lowers calls to
> > `sprintf(dest, "%s", str)` where the return value is used to
> > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > in parsing format strings. Calling `sprintf` with overlapping arguments
> > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> >
> > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > one statement.
>
> O_O What?
>
> No; this is a _terrible_ API: there is no bounds checking, there are no
> buffer sizes. Anything using the example sprintf() pattern is _already_
> wrong and must be removed from the kernel. (Yes, I realize that the
> kernel is *filled* with this bad assumption that "I'll never write more
> than PAGE_SIZE bytes to this buffer", but that's both theoretically
> wrong ("640k is enough for anybody") and has been known to be wrong in
> practice too (e.g. when suddenly your writing routine is reachable by
> splice(2) and you may not have a PAGE_SIZE buffer).
>
> But we cannot _add_ another dangerous string API. We're already in a
> terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> needs to be addressed up by removing the unbounded sprintf() uses. (And
> to do so without introducing bugs related to using snprintf() when
> scnprintf() is expected[4].)
Well, everything (-next, mainline, stable) is broken right now (with
ToT Clang) without providing this symbol. I'm not going to go clean
the entire kernel's use of sprintf to get our CI back to being green.
Thoughts on not exposing the declaration in the header, and changing
the comment to be to the effect of:
"Exists only for optimizing compilers to replace calls to sprintf
with; generally unsafe as bounds checks aren't performed, do not use."
It's still a worthwhile optimization to have, even if the use that
generated it didn't do any bounds checking.
>
> -Kees
>
> [1] https://github.com/KSPP/linux/issues/88
> [2] https://github.com/KSPP/linux/issues/89
> [3] https://github.com/KSPP/linux/issues/90
> [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/
>
> --
> Kees Cook
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 20:47 ` Nick Desaulniers
@ 2020-08-15 21:23 ` Joe Perches
2020-08-15 21:27 ` Dávid Bolvanský
2020-08-15 21:28 ` Nick Desaulniers
0 siblings, 2 replies; 31+ messages in thread
From: Joe Perches @ 2020-08-15 21:23 UTC (permalink / raw)
To: Nick Desaulniers, Kees Cook
Cc: Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
> On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > > LLVM implemented a recent "libcall optimization" that lowers calls to
> > > `sprintf(dest, "%s", str)` where the return value is used to
> > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > > in parsing format strings. Calling `sprintf` with overlapping arguments
> > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> > >
> > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > > one statement.
> >
> > O_O What?
> >
> > No; this is a _terrible_ API: there is no bounds checking, there are no
> > buffer sizes. Anything using the example sprintf() pattern is _already_
> > wrong and must be removed from the kernel. (Yes, I realize that the
> > kernel is *filled* with this bad assumption that "I'll never write more
> > than PAGE_SIZE bytes to this buffer", but that's both theoretically
> > wrong ("640k is enough for anybody") and has been known to be wrong in
> > practice too (e.g. when suddenly your writing routine is reachable by
> > splice(2) and you may not have a PAGE_SIZE buffer).
> >
> > But we cannot _add_ another dangerous string API. We're already in a
> > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> > needs to be addressed up by removing the unbounded sprintf() uses. (And
> > to do so without introducing bugs related to using snprintf() when
> > scnprintf() is expected[4].)
>
> Well, everything (-next, mainline, stable) is broken right now (with
> ToT Clang) without providing this symbol. I'm not going to go clean
> the entire kernel's use of sprintf to get our CI back to being green.
Maybe this should get place in compiler-clang.h so it isn't
generic and public.
Something like:
---
include/linux/compiler-clang.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index cee0c728d39a..6279f1904e39 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -61,3 +61,30 @@
#if __has_feature(shadow_call_stack)
# define __noscs __attribute__((__no_sanitize__("shadow-call-stack")))
#endif
+
+#ifndef __HAVE_ARCH_STPCPY
+/**
+ * stpcpy - copy a string from src to dest returning a pointer to the new end
+ * of dest, including src's NULL terminator. May overrun dest.
+ * @dest: pointer to buffer being copied into.
+ * Must be large enough to receive copy.
+ * @src: pointer to the beginning of string being copied from.
+ * Must not overlap dest.
+ *
+ * This function exists _only_ to support clang's possible conversion of
+ * sprintf calls to stpcpy.
+ *
+ * stpcpy differs from strcpy in two key ways:
+ * 1. inputs must not overlap.
+ * 2. return value is dest's NUL termination character after copy.
+ * (for strcpy, the return value is a pointer to src)
+ */
+
+static inline char *stpcpy(char __restrict *dest, const char __restrict *src)
+{
+ while ((*dest++ = *src++) != '\0') {
+ ; /* nothing */
+ }
+ return --dest;
+}
+#endif
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 21:23 ` Joe Perches
@ 2020-08-15 21:27 ` Dávid Bolvanský
2020-08-15 21:28 ` Nick Desaulniers
1 sibling, 0 replies; 31+ messages in thread
From: Dávid Bolvanský @ 2020-08-15 21:27 UTC (permalink / raw)
To: Joe Perches
Cc: Nick Desaulniers, Kees Cook, Andrew Morton, Eli Friedman,
# 3.4.x, Arvind Sankar, Sami Tolvanen, Vishal Verma,
Dan Williams, Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
-fno-builtin-stpcpy can be used to disable stpcpy but Nick at llvm bugzilla wrote that these flags are broken with LTO.
> Dňa 15. 8. 2020 o 23:24 užívateľ Joe Perches <joe@perches.com> napísal:
>
> On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
>>> On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
>>> On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
>>>> LLVM implemented a recent "libcall optimization" that lowers calls to
>>>> `sprintf(dest, "%s", str)` where the return value is used to
>>>> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
>>>> in parsing format strings. Calling `sprintf` with overlapping arguments
>>>> was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>>>>
>>>> `stpcpy` is just like `strcpy` except it returns the pointer to the new
>>>> tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
>>>> one statement.
>>>
>>> O_O What?
>>>
>>> No; this is a _terrible_ API: there is no bounds checking, there are no
>>> buffer sizes. Anything using the example sprintf() pattern is _already_
>>> wrong and must be removed from the kernel. (Yes, I realize that the
>>> kernel is *filled* with this bad assumption that "I'll never write more
>>> than PAGE_SIZE bytes to this buffer", but that's both theoretically
>>> wrong ("640k is enough for anybody") and has been known to be wrong in
>>> practice too (e.g. when suddenly your writing routine is reachable by
>>> splice(2) and you may not have a PAGE_SIZE buffer).
>>>
>>> But we cannot _add_ another dangerous string API. We're already in a
>>> terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
>>> needs to be addressed up by removing the unbounded sprintf() uses. (And
>>> to do so without introducing bugs related to using snprintf() when
>>> scnprintf() is expected[4].)
>>
>> Well, everything (-next, mainline, stable) is broken right now (with
>> ToT Clang) without providing this symbol. I'm not going to go clean
>> the entire kernel's use of sprintf to get our CI back to being green.
>
> Maybe this should get place in compiler-clang.h so it isn't
> generic and public.
>
> Something like:
>
> ---
> include/linux/compiler-clang.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index cee0c728d39a..6279f1904e39 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -61,3 +61,30 @@
> #if __has_feature(shadow_call_stack)
> # define __noscs __attribute__((__no_sanitize__("shadow-call-stack")))
> #endif
> +
> +#ifndef __HAVE_ARCH_STPCPY
> +/**
> + * stpcpy - copy a string from src to dest returning a pointer to the new end
> + * of dest, including src's NULL terminator. May overrun dest.
> + * @dest: pointer to buffer being copied into.
> + * Must be large enough to receive copy.
> + * @src: pointer to the beginning of string being copied from.
> + * Must not overlap dest.
> + *
> + * This function exists _only_ to support clang's possible conversion of
> + * sprintf calls to stpcpy.
> + *
> + * stpcpy differs from strcpy in two key ways:
> + * 1. inputs must not overlap.
> + * 2. return value is dest's NUL termination character after copy.
> + * (for strcpy, the return value is a pointer to src)
> + */
> +
> +static inline char *stpcpy(char __restrict *dest, const char __restrict *src)
> +{
> + while ((*dest++ = *src++) != '\0') {
> + ; /* nothing */
> + }
> + return --dest;
> +}
> +#endif
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 21:23 ` Joe Perches
2020-08-15 21:27 ` Dávid Bolvanský
@ 2020-08-15 21:28 ` Nick Desaulniers
2020-08-15 21:31 ` Joe Perches
1 sibling, 1 reply; 31+ messages in thread
From: Nick Desaulniers @ 2020-08-15 21:28 UTC (permalink / raw)
To: Joe Perches
Cc: Kees Cook, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
> > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > > > LLVM implemented a recent "libcall optimization" that lowers calls to
> > > > `sprintf(dest, "%s", str)` where the return value is used to
> > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > > > in parsing format strings. Calling `sprintf` with overlapping arguments
> > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> > > >
> > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > > > one statement.
> > >
> > > O_O What?
> > >
> > > No; this is a _terrible_ API: there is no bounds checking, there are no
> > > buffer sizes. Anything using the example sprintf() pattern is _already_
> > > wrong and must be removed from the kernel. (Yes, I realize that the
> > > kernel is *filled* with this bad assumption that "I'll never write more
> > > than PAGE_SIZE bytes to this buffer", but that's both theoretically
> > > wrong ("640k is enough for anybody") and has been known to be wrong in
> > > practice too (e.g. when suddenly your writing routine is reachable by
> > > splice(2) and you may not have a PAGE_SIZE buffer).
> > >
> > > But we cannot _add_ another dangerous string API. We're already in a
> > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> > > needs to be addressed up by removing the unbounded sprintf() uses. (And
> > > to do so without introducing bugs related to using snprintf() when
> > > scnprintf() is expected[4].)
> >
> > Well, everything (-next, mainline, stable) is broken right now (with
> > ToT Clang) without providing this symbol. I'm not going to go clean
> > the entire kernel's use of sprintf to get our CI back to being green.
>
> Maybe this should get place in compiler-clang.h so it isn't
> generic and public.
https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and
https://bugs.llvm.org/show_bug.cgi?id=47144
Seem to imply that Clang is not the only compiler that can lower a
sequence of libcalls to stpcpy. Do we want to wait until we have a
fire drill w/ GCC to move such an implementation from
include/linux/compiler-clang.h back in to lib/string.c?
>
> Something like:
>
> ---
> include/linux/compiler-clang.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index cee0c728d39a..6279f1904e39 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -61,3 +61,30 @@
> #if __has_feature(shadow_call_stack)
> # define __noscs __attribute__((__no_sanitize__("shadow-call-stack")))
> #endif
> +
> +#ifndef __HAVE_ARCH_STPCPY
> +/**
> + * stpcpy - copy a string from src to dest returning a pointer to the new end
> + * of dest, including src's NULL terminator. May overrun dest.
> + * @dest: pointer to buffer being copied into.
> + * Must be large enough to receive copy.
> + * @src: pointer to the beginning of string being copied from.
> + * Must not overlap dest.
> + *
> + * This function exists _only_ to support clang's possible conversion of
> + * sprintf calls to stpcpy.
> + *
> + * stpcpy differs from strcpy in two key ways:
> + * 1. inputs must not overlap.
> + * 2. return value is dest's NUL termination character after copy.
> + * (for strcpy, the return value is a pointer to src)
> + */
> +
> +static inline char *stpcpy(char __restrict *dest, const char __restrict *src)
> +{
> + while ((*dest++ = *src++) != '\0') {
> + ; /* nothing */
> + }
> + return --dest;
> +}
> +#endif
>
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 21:28 ` Nick Desaulniers
@ 2020-08-15 21:31 ` Joe Perches
2020-08-15 22:17 ` Nick Desaulniers
0 siblings, 1 reply; 31+ messages in thread
From: Joe Perches @ 2020-08-15 21:31 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Kees Cook, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote:
> On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote:
> > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
> > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > > > > LLVM implemented a recent "libcall optimization" that lowers calls to
> > > > > `sprintf(dest, "%s", str)` where the return value is used to
> > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > > > > in parsing format strings. Calling `sprintf` with overlapping arguments
> > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> > > > >
> > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > > > > one statement.
> > > >
> > > > O_O What?
> > > >
> > > > No; this is a _terrible_ API: there is no bounds checking, there are no
> > > > buffer sizes. Anything using the example sprintf() pattern is _already_
> > > > wrong and must be removed from the kernel. (Yes, I realize that the
> > > > kernel is *filled* with this bad assumption that "I'll never write more
> > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically
> > > > wrong ("640k is enough for anybody") and has been known to be wrong in
> > > > practice too (e.g. when suddenly your writing routine is reachable by
> > > > splice(2) and you may not have a PAGE_SIZE buffer).
> > > >
> > > > But we cannot _add_ another dangerous string API. We're already in a
> > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> > > > needs to be addressed up by removing the unbounded sprintf() uses. (And
> > > > to do so without introducing bugs related to using snprintf() when
> > > > scnprintf() is expected[4].)
> > >
> > > Well, everything (-next, mainline, stable) is broken right now (with
> > > ToT Clang) without providing this symbol. I'm not going to go clean
> > > the entire kernel's use of sprintf to get our CI back to being green.
> >
> > Maybe this should get place in compiler-clang.h so it isn't
> > generic and public.
>
> https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and
> https://bugs.llvm.org/show_bug.cgi?id=47144
> Seem to imply that Clang is not the only compiler that can lower a
> sequence of libcalls to stpcpy. Do we want to wait until we have a
> fire drill w/ GCC to move such an implementation from
> include/linux/compiler-clang.h back in to lib/string.c?
My guess is yes, wait until gcc, if ever, needs it.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 21:31 ` Joe Perches
@ 2020-08-15 22:17 ` Nick Desaulniers
2020-08-16 0:19 ` Fangrui Song
0 siblings, 1 reply; 31+ messages in thread
From: Nick Desaulniers @ 2020-08-15 22:17 UTC (permalink / raw)
To: Joe Perches
Cc: Kees Cook, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote:
> > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote:
> > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
> > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to
> > > > > > `sprintf(dest, "%s", str)` where the return value is used to
> > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments
> > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> > > > > >
> > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > > > > > one statement.
> > > > >
> > > > > O_O What?
> > > > >
> > > > > No; this is a _terrible_ API: there is no bounds checking, there are no
> > > > > buffer sizes. Anything using the example sprintf() pattern is _already_
> > > > > wrong and must be removed from the kernel. (Yes, I realize that the
> > > > > kernel is *filled* with this bad assumption that "I'll never write more
> > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically
> > > > > wrong ("640k is enough for anybody") and has been known to be wrong in
> > > > > practice too (e.g. when suddenly your writing routine is reachable by
> > > > > splice(2) and you may not have a PAGE_SIZE buffer).
> > > > >
> > > > > But we cannot _add_ another dangerous string API. We're already in a
> > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And
> > > > > to do so without introducing bugs related to using snprintf() when
> > > > > scnprintf() is expected[4].)
> > > >
> > > > Well, everything (-next, mainline, stable) is broken right now (with
> > > > ToT Clang) without providing this symbol. I'm not going to go clean
> > > > the entire kernel's use of sprintf to get our CI back to being green.
> > >
> > > Maybe this should get place in compiler-clang.h so it isn't
> > > generic and public.
> >
> > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and
> > https://bugs.llvm.org/show_bug.cgi?id=47144
> > Seem to imply that Clang is not the only compiler that can lower a
> > sequence of libcalls to stpcpy. Do we want to wait until we have a
> > fire drill w/ GCC to move such an implementation from
> > include/linux/compiler-clang.h back in to lib/string.c?
>
> My guess is yes, wait until gcc, if ever, needs it.
The suggestion to use static inline doesn't even make sense. The
compiler is lowering calls to other library routines; `stpcpy` isn't
being explicitly called. Even if it was, not sure we want it being
inlined. No symbol definition will be emitted; problem not solved.
And I refuse to add any more code using `extern inline`. Putting the
definition in lib/string.c is the most straightforward and avoids
revisiting this issue in the future for other toolchains. I'll limit
access by removing the declaration, and adding a comment to avoid its
use. But if you're going to use a gnu target triple without using
-ffreestanding because you *want* libcall optimizations, then you have
to provide symbols for all possible library routines!
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 22:17 ` Nick Desaulniers
@ 2020-08-16 0:19 ` Fangrui Song
2020-08-16 5:22 ` Sedat Dilek
0 siblings, 1 reply; 31+ messages in thread
From: Fangrui Song @ 2020-08-16 0:19 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Joe Perches, Kees Cook, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On 2020-08-15, 'Nick Desaulniers' via Clang Built Linux wrote:
>On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote:
>>
>> On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote:
>> > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote:
>> > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
>> > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
>> > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
>> > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to
>> > > > > > `sprintf(dest, "%s", str)` where the return value is used to
>> > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
>> > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments
>> > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
>> > > > > >
>> > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
>> > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
>> > > > > > one statement.
>> > > > >
>> > > > > O_O What?
>> > > > >
>> > > > > No; this is a _terrible_ API: there is no bounds checking, there are no
>> > > > > buffer sizes. Anything using the example sprintf() pattern is _already_
>> > > > > wrong and must be removed from the kernel. (Yes, I realize that the
>> > > > > kernel is *filled* with this bad assumption that "I'll never write more
>> > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically
>> > > > > wrong ("640k is enough for anybody") and has been known to be wrong in
>> > > > > practice too (e.g. when suddenly your writing routine is reachable by
>> > > > > splice(2) and you may not have a PAGE_SIZE buffer).
>> > > > >
>> > > > > But we cannot _add_ another dangerous string API. We're already in a
>> > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
>> > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And
>> > > > > to do so without introducing bugs related to using snprintf() when
>> > > > > scnprintf() is expected[4].)
>> > > >
>> > > > Well, everything (-next, mainline, stable) is broken right now (with
>> > > > ToT Clang) without providing this symbol. I'm not going to go clean
>> > > > the entire kernel's use of sprintf to get our CI back to being green.
>> > >
>> > > Maybe this should get place in compiler-clang.h so it isn't
>> > > generic and public.
>> >
>> > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and
>> > https://bugs.llvm.org/show_bug.cgi?id=47144
>> > Seem to imply that Clang is not the only compiler that can lower a
>> > sequence of libcalls to stpcpy. Do we want to wait until we have a
>> > fire drill w/ GCC to move such an implementation from
>> > include/linux/compiler-clang.h back in to lib/string.c?
>>
>> My guess is yes, wait until gcc, if ever, needs it.
>
>The suggestion to use static inline doesn't even make sense. The
>compiler is lowering calls to other library routines; `stpcpy` isn't
>being explicitly called. Even if it was, not sure we want it being
>inlined. No symbol definition will be emitted; problem not solved.
>And I refuse to add any more code using `extern inline`. Putting the
>definition in lib/string.c is the most straightforward and avoids
>revisiting this issue in the future for other toolchains. I'll limit
>access by removing the declaration, and adding a comment to avoid its
>use. But if you're going to use a gnu target triple without using
>-ffreestanding because you *want* libcall optimizations, then you have
>to provide symbols for all possible library routines!
Adding a definition without a declaration for stpcpy looks good.
Clang LTO will work.
(If the kernel does not want to provide these routines,
is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912
probably wrong? (why remove -ffreestanding from the main Makefile) )
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-16 0:19 ` Fangrui Song
@ 2020-08-16 5:22 ` Sedat Dilek
2020-08-16 15:02 ` Arvind Sankar
0 siblings, 1 reply; 31+ messages in thread
From: Sedat Dilek @ 2020-08-16 5:22 UTC (permalink / raw)
To: Fangrui Song
Cc: Nick Desaulniers, Joe Perches, Kees Cook, Andrew Morton,
Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> On 2020-08-15, 'Nick Desaulniers' via Clang Built Linux wrote:
> >On Sat, Aug 15, 2020 at 2:31 PM Joe Perches <joe@perches.com> wrote:
> >>
> >> On Sat, 2020-08-15 at 14:28 -0700, Nick Desaulniers wrote:
> >> > On Sat, Aug 15, 2020 at 2:24 PM Joe Perches <joe@perches.com> wrote:
> >> > > On Sat, 2020-08-15 at 13:47 -0700, Nick Desaulniers wrote:
> >> > > > On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> >> > > > > On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> >> > > > > > LLVM implemented a recent "libcall optimization" that lowers calls to
> >> > > > > > `sprintf(dest, "%s", str)` where the return value is used to
> >> > > > > > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> >> > > > > > in parsing format strings. Calling `sprintf` with overlapping arguments
> >> > > > > > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> >> > > > > >
> >> > > > > > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> >> > > > > > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> >> > > > > > one statement.
> >> > > > >
> >> > > > > O_O What?
> >> > > > >
> >> > > > > No; this is a _terrible_ API: there is no bounds checking, there are no
> >> > > > > buffer sizes. Anything using the example sprintf() pattern is _already_
> >> > > > > wrong and must be removed from the kernel. (Yes, I realize that the
> >> > > > > kernel is *filled* with this bad assumption that "I'll never write more
> >> > > > > than PAGE_SIZE bytes to this buffer", but that's both theoretically
> >> > > > > wrong ("640k is enough for anybody") and has been known to be wrong in
> >> > > > > practice too (e.g. when suddenly your writing routine is reachable by
> >> > > > > splice(2) and you may not have a PAGE_SIZE buffer).
> >> > > > >
> >> > > > > But we cannot _add_ another dangerous string API. We're already in a
> >> > > > > terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> >> > > > > needs to be addressed up by removing the unbounded sprintf() uses. (And
> >> > > > > to do so without introducing bugs related to using snprintf() when
> >> > > > > scnprintf() is expected[4].)
> >> > > >
> >> > > > Well, everything (-next, mainline, stable) is broken right now (with
> >> > > > ToT Clang) without providing this symbol. I'm not going to go clean
> >> > > > the entire kernel's use of sprintf to get our CI back to being green.
> >> > >
> >> > > Maybe this should get place in compiler-clang.h so it isn't
> >> > > generic and public.
> >> >
> >> > https://bugs.llvm.org/show_bug.cgi?id=47162#c7 and
> >> > https://bugs.llvm.org/show_bug.cgi?id=47144
> >> > Seem to imply that Clang is not the only compiler that can lower a
> >> > sequence of libcalls to stpcpy. Do we want to wait until we have a
> >> > fire drill w/ GCC to move such an implementation from
> >> > include/linux/compiler-clang.h back in to lib/string.c?
> >>
> >> My guess is yes, wait until gcc, if ever, needs it.
> >
> >The suggestion to use static inline doesn't even make sense. The
> >compiler is lowering calls to other library routines; `stpcpy` isn't
> >being explicitly called. Even if it was, not sure we want it being
> >inlined. No symbol definition will be emitted; problem not solved.
> >And I refuse to add any more code using `extern inline`. Putting the
> >definition in lib/string.c is the most straightforward and avoids
> >revisiting this issue in the future for other toolchains. I'll limit
> >access by removing the declaration, and adding a comment to avoid its
> >use. But if you're going to use a gnu target triple without using
> >-ffreestanding because you *want* libcall optimizations, then you have
> >to provide symbols for all possible library routines!
>
> Adding a definition without a declaration for stpcpy looks good.
> Clang LTO will work.
>
> (If the kernel does not want to provide these routines,
> is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912
> probably wrong? (why remove -ffreestanding from the main Makefile) )
>
We had some many issues in arch/x86 where *FLAGS were removed or used
differently and had to re-add them :-(.
So if -ffreestanding is used in arch/x86 and was! used in top-level
Makefile - it makes sense to re-add it back?
( I cannot speak for archs other than x86. )
- Sedat -
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-16 5:22 ` Sedat Dilek
@ 2020-08-16 15:02 ` Arvind Sankar
2020-08-17 17:14 ` Sami Tolvanen
0 siblings, 1 reply; 31+ messages in thread
From: Arvind Sankar @ 2020-08-16 15:02 UTC (permalink / raw)
To: Sedat Dilek
Cc: Fangrui Song, Nick Desaulniers, Joe Perches, Kees Cook,
Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Arvind Sankar, Sami Tolvanen,
Vishal Verma, Dan Williams, Andy Shevchenko,
Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote:
> On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux
> <clang-built-linux@googlegroups.com> wrote:
> >
> > Adding a definition without a declaration for stpcpy looks good.
> > Clang LTO will work.
> >
> > (If the kernel does not want to provide these routines,
> > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912
> > probably wrong? (why remove -ffreestanding from the main Makefile) )
> >
>
> We had some many issues in arch/x86 where *FLAGS were removed or used
> differently and had to re-add them :-(.
>
> So if -ffreestanding is used in arch/x86 and was! used in top-level
> Makefile - it makes sense to re-add it back?
> ( I cannot speak for archs other than x86. )
>
> - Sedat -
-ffreestanding disables _all_ builtins and libcall optimizations, which
is probably not desirable. If we added it back, we'd need to also go
back to #define various string functions to the __builtin versions.
Though I don't understand the original issue, with -ffreestanding,
sprintf shouldn't have been turned into strcpy in the first place.
32-bit still has -ffreestanding -- I wonder if that's actually necessary
any more?
Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a
compiler bug?
Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-16 15:02 ` Arvind Sankar
@ 2020-08-17 17:14 ` Sami Tolvanen
2020-08-17 18:36 ` Nick Desaulniers
2020-08-17 19:16 ` Kees Cook
0 siblings, 2 replies; 31+ messages in thread
From: Sami Tolvanen @ 2020-08-17 17:14 UTC (permalink / raw)
To: Arvind Sankar
Cc: Sedat Dilek, Fangrui Song, Nick Desaulniers, Joe Perches,
Kees Cook, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Sun, Aug 16, 2020 at 8:02 AM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote:
> > On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux
> > <clang-built-linux@googlegroups.com> wrote:
> > >
> > > Adding a definition without a declaration for stpcpy looks good.
> > > Clang LTO will work.
> > >
> > > (If the kernel does not want to provide these routines,
> > > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912
> > > probably wrong? (why remove -ffreestanding from the main Makefile) )
> > >
> >
> > We had some many issues in arch/x86 where *FLAGS were removed or used
> > differently and had to re-add them :-(.
> >
> > So if -ffreestanding is used in arch/x86 and was! used in top-level
> > Makefile - it makes sense to re-add it back?
> > ( I cannot speak for archs other than x86. )
> >
> > - Sedat -
>
> -ffreestanding disables _all_ builtins and libcall optimizations, which
> is probably not desirable. If we added it back, we'd need to also go
> back to #define various string functions to the __builtin versions.
>
> Though I don't understand the original issue, with -ffreestanding,
> sprintf shouldn't have been turned into strcpy in the first place.
>
> 32-bit still has -ffreestanding -- I wonder if that's actually necessary
> any more?
>
> Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a
> compiler bug?
I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does
work with LTO as well.
Sami
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-17 17:14 ` Sami Tolvanen
@ 2020-08-17 18:36 ` Nick Desaulniers
2020-08-17 19:15 ` Kees Cook
` (2 more replies)
2020-08-17 19:16 ` Kees Cook
1 sibling, 3 replies; 31+ messages in thread
From: Nick Desaulniers @ 2020-08-17 18:36 UTC (permalink / raw)
To: Sami Tolvanen, Arvind Sankar, Kees Cook
Cc: Sedat Dilek, Fangrui Song, Joe Perches, Andrew Morton,
Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes, Nathan Chancellor
On Mon, Aug 17, 2020 at 10:14 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Sun, Aug 16, 2020 at 8:02 AM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sun, Aug 16, 2020 at 07:22:35AM +0200, Sedat Dilek wrote:
> > > On Sun, Aug 16, 2020 at 2:19 AM 'Fangrui Song' via Clang Built Linux
> > > <clang-built-linux@googlegroups.com> wrote:
> > > >
> > > > Adding a definition without a declaration for stpcpy looks good.
> > > > Clang LTO will work.
> > > >
> > > > (If the kernel does not want to provide these routines,
> > > > is http://git.kernel.org/linus/6edfba1b33c701108717f4e036320fc39abe1912
> > > > probably wrong? (why remove -ffreestanding from the main Makefile) )
> > > >
> > >
> > > We had some many issues in arch/x86 where *FLAGS were removed or used
> > > differently and had to re-add them :-(.
> > >
> > > So if -ffreestanding is used in arch/x86 and was! used in top-level
> > > Makefile - it makes sense to re-add it back?
> > > ( I cannot speak for archs other than x86. )
> > >
> > > - Sedat -
> >
> > -ffreestanding disables _all_ builtins and libcall optimizations, which
> > is probably not desirable. If we added it back, we'd need to also go
I agree.
> > back to #define various string functions to the __builtin versions.
> >
> > Though I don't understand the original issue, with -ffreestanding,
> > sprintf shouldn't have been turned into strcpy in the first place.
Huh? The original issue for this thread is because `-ffreestanding`
*isn't* being used for most targets (oh boy, actually mixed usage by
ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and
I'm not suggesting it be used.
> > 32-bit still has -ffreestanding -- I wonder if that's actually necessary
> > any more?
Fair question. Someone will have to go chase git history, since
0a6ef376d4ba covers it up. If anyone has any tricks to do so quickly;
I'd love to know. I generally checkout the commit prior, then use vim
fugitive to get git blame.
> > Why does -fno-builtin-stpcpy not work with clang LTO? Isn't that a
> > compiler bug?
Yes; Sami found a recent patch that looks to me like it may have
recently solved that bug.
https://reviews.llvm.org/D71193 which landed Dec 12 2019. The bug
report was based on
https://github.com/ClangBuiltLinux/linux/issues/416#issuecomment-472231304
(Issue reported March 8 2019). And I do recall being able to
reproduce the bug when I sent
commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
Now that that is fixed as reported by Sami below, I don't mind sending
a revert for 5f074f3e192f that adds -fno-builtin-bcmp, because the
current implementation of bcmp isn't useful.
That said, this libcall optimization/transformation (sprintf->stpcpy)
does look useful to me. Kees, do you have thoughts on me providing
the implementation without exposing it in a header vs using
-fno-builtin-stpcpy? (I would need to add the missing EXPORT_SYMBOL,
as pointed out by 0day bot and on the github thread). I don't care
either way; I'd just like your input before sending a V+1. Maybe
better to just not implement it and never implement it?
>
> I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does
> work with LTO as well.
>
> Sami
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-17 18:36 ` Nick Desaulniers
@ 2020-08-17 19:15 ` Kees Cook
2020-08-17 20:13 ` Arvind Sankar
[not found] ` <77557c29286140dea726cc334b4f59fc@AcuMS.aculab.com>
2 siblings, 0 replies; 31+ messages in thread
From: Kees Cook @ 2020-08-17 19:15 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Sami Tolvanen, Arvind Sankar, Sedat Dilek, Fangrui Song,
Joe Perches, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes, Nathan Chancellor
On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote:
> That said, this libcall optimization/transformation (sprintf->stpcpy)
> does look useful to me. Kees, do you have thoughts on me providing
> the implementation without exposing it in a header vs using
> -fno-builtin-stpcpy? (I would need to add the missing EXPORT_SYMBOL,
> as pointed out by 0day bot and on the github thread). I don't care
> either way; I'd just like your input before sending a V+1. Maybe
> better to just not implement it and never implement it?
I think I would ultimately prefer -fno-builtin-stpcpy, but for now,
sure, an implementation without a header (and a biiig comment above it
detailing why and a reference to the bug) would be fine by me.
--
Kees Cook
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-17 18:36 ` Nick Desaulniers
2020-08-17 19:15 ` Kees Cook
@ 2020-08-17 20:13 ` Arvind Sankar
2020-08-17 21:45 ` Nick Desaulniers
[not found] ` <77557c29286140dea726cc334b4f59fc@AcuMS.aculab.com>
2 siblings, 1 reply; 31+ messages in thread
From: Arvind Sankar @ 2020-08-17 20:13 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Sami Tolvanen, Arvind Sankar, Kees Cook, Sedat Dilek,
Fangrui Song, Joe Perches, Andrew Morton,
Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes, Nathan Chancellor
On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote:
> > > Though I don't understand the original issue, with -ffreestanding,
> > > sprintf shouldn't have been turned into strcpy in the first place.
>
> Huh? The original issue for this thread is because `-ffreestanding`
> *isn't* being used for most targets (oh boy, actually mixed usage by
> ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and
> I'm not suggesting it be used.
>
Sorry, I meant the issue mentioned in the commit that removed
-ffreestanding, not the stpcpy one you're solving now. It says that
sprintf got converted into strcpy, which caused failures because back
then, strcpy was #define'd to __builtin_strcpy, and the default
implementation was actually of a function called __builtin_strcpy o_O,
not strcpy.
Anyway, that's water under the bridge now.
6edfba1b33c7 ("x86_64: Don't define string functions to builtin")
gcc should handle this anyways, and it causes problems when
sprintf is turned into strcpy by gcc behind our backs and
the C fallback version of strcpy is actually defining __builtin_strcpy
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-17 20:13 ` Arvind Sankar
@ 2020-08-17 21:45 ` Nick Desaulniers
0 siblings, 0 replies; 31+ messages in thread
From: Nick Desaulniers @ 2020-08-17 21:45 UTC (permalink / raw)
To: Arvind Sankar
Cc: Sami Tolvanen, Kees Cook, Sedat Dilek, Fangrui Song, Joe Perches,
Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes, Nathan Chancellor
On Mon, Aug 17, 2020 at 1:13 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Aug 17, 2020 at 11:36:49AM -0700, Nick Desaulniers wrote:
> > > > Though I don't understand the original issue, with -ffreestanding,
> > > > sprintf shouldn't have been turned into strcpy in the first place.
> >
> > Huh? The original issue for this thread is because `-ffreestanding`
> > *isn't* being used for most targets (oh boy, actually mixed usage by
> > ARCH. Looks like MIPS, m68k, superH, xtensa, and 32b x86 use it?); and
> > I'm not suggesting it be used.
> >
>
> Sorry, I meant the issue mentioned in the commit that removed
> -ffreestanding, not the stpcpy one you're solving now. It says that
> sprintf got converted into strcpy, which caused failures because back
> then, strcpy was #define'd to __builtin_strcpy, and the default
> implementation was actually of a function called __builtin_strcpy o_O,
> not strcpy.
>
> Anyway, that's water under the bridge now.
>
> 6edfba1b33c7 ("x86_64: Don't define string functions to builtin")
> gcc should handle this anyways, and it causes problems when
> sprintf is turned into strcpy by gcc behind our backs and
> the C fallback version of strcpy is actually defining __builtin_strcpy
For fun, I tried removing `-ffreestanding` from arch/x86/Makefile;
both gcc and clang can compile+boot the i386 defconfig just fine. Why
don't I send a patch removing it with your suggested by in a series of
fixes for stpcpy and bcmp?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <77557c29286140dea726cc334b4f59fc@AcuMS.aculab.com>]
* Re: [PATCH v2] lib/string.c: implement stpcpy
[not found] ` <77557c29286140dea726cc334b4f59fc@AcuMS.aculab.com>
@ 2020-08-18 8:32 ` Joe Perches
0 siblings, 0 replies; 31+ messages in thread
From: Joe Perches @ 2020-08-18 8:32 UTC (permalink / raw)
To: David Laight, 'Nick Desaulniers',
Sami Tolvanen, Arvind Sankar, Kees Cook
Cc: Sedat Dilek, Fangrui Song, Andrew Morton,
Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes, Nathan Chancellor
On Tue, 2020-08-18 at 08:24 +0000, David Laight wrote:
> From: Nick Desaulniers
> > Sent: 17 August 2020 19:37
> ..
> > That said, this libcall optimization/transformation (sprintf->stpcpy)
> > does look useful to me.
>
> I'd rather get a cow if I ask for a cow...
>
> Maybe checkpatch (etc) could report places where snprintf()
> could be replaced by a simpler function.
You mean sprintf no?
Reminds me of seq_printf vs seq_puts...
https://lkml.org/lkml/2013/3/16/79
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-17 17:14 ` Sami Tolvanen
2020-08-17 18:36 ` Nick Desaulniers
@ 2020-08-17 19:16 ` Kees Cook
1 sibling, 0 replies; 31+ messages in thread
From: Kees Cook @ 2020-08-17 19:16 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Arvind Sankar, Sedat Dilek, Fangrui Song, Nick Desaulniers,
Joe Perches, Andrew Morton, Dávid Bolvanský,
Eli Friedman, # 3.4.x, Vishal Verma, Dan Williams,
Andy Shevchenko, Joel Fernandes (Google),
Daniel Axtens, Ingo Molnar, Yury Norov, Alexandru Ardelean, LKML,
clang-built-linux, Rasmus Villemoes
On Mon, Aug 17, 2020 at 10:14:43AM -0700, Sami Tolvanen wrote:
> I just confirmed that adding -fno-builtin-stpcpy to KBUILD_CFLAGS does
> work with LTO as well.
Oh, I read this out of order; sorry! Yes, if -fno-builtin-stpcpy works,
let's use that instead. Doesn't that solve it too?
--
Kees Cook
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] lib/string.c: implement stpcpy
2020-08-15 2:09 ` [PATCH v2] " Nick Desaulniers
@ 2020-08-16 7:44 ` kernel test robot
2020-08-15 3:42 ` Joe Perches
` (2 subsequent siblings)
3 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2020-08-16 7:44 UTC (permalink / raw)
To: Nick Desaulniers, Andrew Morton
Cc: kbuild-all, Linux Memory Management List,
Dávid Bolvanský,
Eli Friedman, Nick Desaulniers, stable, Arvind Sankar,
Joe Perches, Sami Tolvanen, Vishal Verma
[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]
Hi Nick,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on hnaz-linux-mm/master]
[also build test ERROR on kees/for-next/pstore linus/master v5.8 next-20200814]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/lib-string-c-implement-stpcpy/20200816-090542
base: https://github.com/hnaz/linux-mm master
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "stpcpy" [sound/pci/ac97/snd-ac97-codec.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20246 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread