[PATCH] x86_64: Change .weak to SYM_FUNC_START_WEAK for arch/x86/lib/mem*_64.S

[Fangrui Song]

Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
memset/memmove/memcpy functions") added .weak directives to
arch/x86/lib/mem*_64.S instead of changing the existing SYM_FUNC_START_*
macros. This can lead to the assembly snippet `.weak memcpy ... .globl
memcpy` which will produce a STB_WEAK memcpy with GNU as but STB_GLOBAL
memcpy with LLVM's integrated assembler before LLVM 12. LLVM 12 (since
https://reviews.llvm.org/D90108) will error on such an overridden symbol
binding.

Use the appropriate SYM_FUNC_START_WEAK instead.

Fixes: 393f203f5fd5 ("x86_64: kasan: add interceptors for memset/memmove/memcpy functions")
Reported-by: Sami Tolvanen <samito...@google.com>
Signed-off-by: Fangrui Song <mas...@google.com>
Cc: <sta...@vger.kernel.org>
---
arch/x86/lib/memcpy_64.S | 4 +---
arch/x86/lib/memmove_64.S | 4 +---
arch/x86/lib/memset_64.S | 4 +---
3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 037faac46b0c..1e299ac73c86 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -16,8 +16,6 @@
* to a jmp to memcpy_erms which does the REP; MOVSB mem copy.
*/

-.weak memcpy
-
/*
* memcpy - Copy a memory block.
*
@@ -30,7 +28,7 @@
* rax original destination
*/
SYM_FUNC_START_ALIAS(__memcpy)
-SYM_FUNC_START_LOCAL(memcpy)
+SYM_FUNC_START_WEAK(memcpy)
ALTERNATIVE_2 "jmp memcpy_orig", "", X86_FEATURE_REP_GOOD, \
"jmp memcpy_erms", X86_FEATURE_ERMS

diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S
index 7ff00ea64e4f..41902fe8b859 100644
--- a/arch/x86/lib/memmove_64.S
+++ b/arch/x86/lib/memmove_64.S
@@ -24,9 +24,7 @@
* Output:
* rax: dest
*/
-.weak memmove
-
-SYM_FUNC_START_ALIAS(memmove)
+SYM_FUNC_START_WEAK(memmove)
SYM_FUNC_START(__memmove)

mov %rdi, %rax
diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S
index 9ff15ee404a4..0bfd26e4ca9e 100644
--- a/arch/x86/lib/memset_64.S
+++ b/arch/x86/lib/memset_64.S
@@ -6,8 +6,6 @@
#include <asm/alternative-asm.h>
#include <asm/export.h>

-.weak memset
-
/*
* ISO C memset - set a memory block to a byte value. This function uses fast
* string to get better performance than the original function. The code is
@@ -19,7 +17,7 @@
*
* rax original destination
*/
-SYM_FUNC_START_ALIAS(memset)
+SYM_FUNC_START_WEAK(memset)
SYM_FUNC_START(__memset)
/*
* Some CPUs support enhanced REP MOVSB/STOSB feature. It is recommended
--
2.29.1.341.ge80a0c044ae-goog

[Sami Tolvanen]

On Thu, Oct 29, 2020 at 11:05 AM 'Fangrui Song' via Clang Built Linux
<clang-bu...@googlegroups.com> wrote:
>
> Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
> memset/memmove/memcpy functions") added .weak directives to
> arch/x86/lib/mem*_64.S instead of changing the existing SYM_FUNC_START_*
> macros. This can lead to the assembly snippet `.weak memcpy ... .globl
> memcpy` which will produce a STB_WEAK memcpy with GNU as but STB_GLOBAL
> memcpy with LLVM's integrated assembler before LLVM 12. LLVM 12 (since
> https://reviews.llvm.org/D90108) will error on such an overridden symbol
> binding.
>
> Use the appropriate SYM_FUNC_START_WEAK instead.
>
> Fixes: 393f203f5fd5 ("x86_64: kasan: add interceptors for memset/memmove/memcpy functions")
> Reported-by: Sami Tolvanen <samito...@google.com>
> Signed-off-by: Fangrui Song <mas...@google.com>
> Cc: <sta...@vger.kernel.org>
> ---
> arch/x86/lib/memcpy_64.S | 4 +---
> arch/x86/lib/memmove_64.S | 4 +---
> arch/x86/lib/memset_64.S | 4 +---
> 3 files changed, 3 insertions(+), 9 deletions(-)

Thank you for fixing this! I tested the patch with gcc 9.3 and clang
12, both with and without Clang's integrated assembler, and the kernel
builds and boots with all compilers again.

Tested-by: Sami Tolvanen <samito...@google.com>

Sami

[Nick Desaulniers]

On Thu, Oct 29, 2020 at 11:05 AM 'Fangrui Song' via Clang Built Linux
<clang-bu...@googlegroups.com> wrote:
>

Thanks for the patch. This exposes a lack of symmetry in the
assembler subroutine for memcpy; memmove and memset use SYM_FUNC_START
for their double underscore prefixed symbols, and SYM_FUNC_START_WEAK
for the non prefixed symbols, and no SYM_FUNC_START_ALIAS after your
patch. It's also curious to me why you removed SYM_FUNC_START_ALIAS
for memmove and memset, but not memcpy? Can we sort that out so that
they all follow the same convention?

Before your patch, with GNU `as` I see:
$ llvm-readelf -s arch/x86/lib/memcpy_64.o | grep -e memcpy -e __memcpy
16: 0000000000000000 26 FUNC GLOBAL DEFAULT 4 __memcpy
17: 0000000000000000 26 FUNC WEAK DEFAULT 4 memcpy
$ llvm-readelf -s arch/x86/lib/memmove_64.o| grep -e memmove -e __memmove
13: 0000000000000000 409 FUNC WEAK DEFAULT 1 memmove
14: 0000000000000000 409 FUNC GLOBAL DEFAULT 1 __memmove
$ llvm-readelf -s arch/x86/lib/memset_64.o| grep -e memset -e __memset
15: 0000000000000000 47 FUNC WEAK DEFAULT 1 memset
16: 0000000000000000 47 FUNC GLOBAL DEFAULT 1 __memset

After your patch, with GNU `as` I see:
$ llvm-readelf -s arch/x86/lib/memcpy_64.o | grep -e memcpy -e __memcpy
16: 0000000000000000 26 FUNC GLOBAL DEFAULT 4 __memcpy
17: 0000000000000000 26 FUNC WEAK DEFAULT 4 memcpy
$ llvm-readelf -s arch/x86/lib/memmove_64.o| grep -e memmove -e __memmove
13: 0000000000000000 409 FUNC WEAK DEFAULT 1 memmove
14: 0000000000000000 409 FUNC GLOBAL DEFAULT 1 __memmove
$ llvm-readelf -s arch/x86/lib/memset_64.o| grep -e memset -e __memset
15: 0000000000000000 47 FUNC WEAK DEFAULT 1 memset
16: 0000000000000000 47 FUNC GLOBAL DEFAULT 1 __memset

So in that sense, your patch is no functional change, and simply
resolves ambiguities in repeatedly defining a symbol with different
bindings. I guess we can save uncovering why memcpy doesn't follow
the same convention for another day. In that sense:

Reviewed-by: Nick Desaulniers <ndesau...@google.com>
Tested-by: Nick Desaulniers <ndesau...@google.com>

And thanks for the patch!

> --
--
Thanks,
~Nick Desaulniers

[Jiri Slaby]

On 29. 10. 20, 19:05, Fangrui Song wrote:
> Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
> memset/memmove/memcpy functions") added .weak directives to
> arch/x86/lib/mem*_64.S instead of changing the existing SYM_FUNC_START_*
> macros.

There were no SYM_FUNC_START_* macros in 2015.

> This can lead to the assembly snippet `.weak memcpy ... .globl
> memcpy`

SYM_FUNC_START_LOCAL(memcpy)
does not add .globl, so I don't understand the rationale.

js
suse labs

[Fāng-ruì Sòng]

On Fri, Oct 30, 2020 at 2:48 AM Jiri Slaby <jiri...@kernel.org> wrote:
>
> On 29. 10. 20, 19:05, Fangrui Song wrote:
> > Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
> > memset/memmove/memcpy functions") added .weak directives to
> > arch/x86/lib/mem*_64.S instead of changing the existing SYM_FUNC_START_*
> > macros.
>
> There were no SYM_FUNC_START_* macros in 2015.

include/linux/linkage.h had WEAK in 2015 and WEAK should have been
used instead. Do I just need to fix the description?

> > This can lead to the assembly snippet `.weak memcpy ... .globl
> > memcpy`
>
> SYM_FUNC_START_LOCAL(memcpy)
> does not add .globl, so I don't understand the rationale.

There is no problem using
.weak
SYM_FUNC_START_LOCAL
just looks suspicious so I changed it, too.

--
宋方睿

[Jiri Slaby]

On 30. 10. 20, 17:08, Fāng-ruì Sòng wrote:
> On Fri, Oct 30, 2020 at 2:48 AM Jiri Slaby <jiri...@kernel.org> wrote:
>>
>> On 29. 10. 20, 19:05, Fangrui Song wrote:
>>> Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
>>> memset/memmove/memcpy functions") added .weak directives to
>>> arch/x86/lib/mem*_64.S instead of changing the existing SYM_FUNC_START_*
>>> macros.
>>
>> There were no SYM_FUNC_START_* macros in 2015.
>
> include/linux/linkage.h had WEAK in 2015 and WEAK should have been
> used instead. Do I just need to fix the description?
>
>>> This can lead to the assembly snippet `.weak memcpy ... .globl
>>> memcpy`
>>
>> SYM_FUNC_START_LOCAL(memcpy)
>> does not add .globl, so I don't understand the rationale.
>
> There is no problem using
> .weak
> SYM_FUNC_START_LOCAL
> just looks suspicious so I changed it, too.

So the commit log doesn't correspond to your changes ;). You need to
update it and describe the difference from memmove/memset properly.

js

[Fangrui Song]

Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
memset/memmove/memcpy functions") added .weak directives to

arch/x86/lib/mem*_64.S instead of changing the existing ENTRY macros to
WEAK. This can lead to the assembly snippet `.weak memcpy ... .globl
memcpy` which will produce a STB_WEAK memcpy with GNU as but STB_GLOBAL

memcpy with LLVM's integrated assembler before LLVM 12. LLVM 12 (since
https://reviews.llvm.org/D90108) will error on such an overridden symbol
binding.

Commit ef1e03152cb0 ("x86/asm: Make some functions local") changed ENTRY in
arch/x86/lib/memcpy_64.S to SYM_FUNC_START_LOCAL, which was ineffective due to
the preceding .weak directive.

Use the appropriate SYM_FUNC_START_WEAK instead.

Fixes: 393f203f5fd5 ("x86_64: kasan: add interceptors for memset/memmove/memcpy functions")

Fixes: ef1e03152cb0 ("x86/asm: Make some functions local")

Reported-by: Sami Tolvanen <samito...@google.com>
Signed-off-by: Fangrui Song <mas...@google.com>
Cc: <sta...@vger.kernel.org>
---

Changes in v2
* Correct the message: SYM_FUNC_START_WEAK was not available at commit 393f203f5fd5.
* Mention Fixes: ef1e03152cb0

2.29.1.341.ge80a0c044ae-goog

[Nathan Chancellor]

On Mon, Nov 02, 2020 at 05:23:58PM -0800, 'Fangrui Song' via Clang Built Linux wrote:
> Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
> memset/memmove/memcpy functions") added .weak directives to
> arch/x86/lib/mem*_64.S instead of changing the existing ENTRY macros to
> WEAK. This can lead to the assembly snippet `.weak memcpy ... .globl
> memcpy` which will produce a STB_WEAK memcpy with GNU as but STB_GLOBAL
> memcpy with LLVM's integrated assembler before LLVM 12. LLVM 12 (since
> https://reviews.llvm.org/D90108) will error on such an overridden symbol
> binding.
>
> Commit ef1e03152cb0 ("x86/asm: Make some functions local") changed ENTRY in
> arch/x86/lib/memcpy_64.S to SYM_FUNC_START_LOCAL, which was ineffective due to
> the preceding .weak directive.
>
> Use the appropriate SYM_FUNC_START_WEAK instead.
>
> Fixes: 393f203f5fd5 ("x86_64: kasan: add interceptors for memset/memmove/memcpy functions")
> Fixes: ef1e03152cb0 ("x86/asm: Make some functions local")
> Reported-by: Sami Tolvanen <samito...@google.com>
> Signed-off-by: Fangrui Song <mas...@google.com>
> Cc: <sta...@vger.kernel.org>

This resolves the build error I see with LLVM_IAS=1 and ToT LLVM.

Tested-by: Nathan Chancellor <natecha...@gmail.com>

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201103012358.168682-1-maskray%40google.com.

[Nick Desaulniers]

+ LKML so I can find this easier to fetch via b4

On Mon, Nov 2, 2020 at 5:46 PM Nathan Chancellor
<natecha...@gmail.com> wrote:
>
> On Mon, Nov 02, 2020 at 05:23:58PM -0800, 'Fangrui Song' via Clang Built Linux wrote:
> > Commit 393f203f5fd5 ("x86_64: kasan: add interceptors for
> > memset/memmove/memcpy functions") added .weak directives to
> > arch/x86/lib/mem*_64.S instead of changing the existing ENTRY macros to
> > WEAK. This can lead to the assembly snippet `.weak memcpy ... .globl
> > memcpy` which will produce a STB_WEAK memcpy with GNU as but STB_GLOBAL
> > memcpy with LLVM's integrated assembler before LLVM 12. LLVM 12 (since
> > https://reviews.llvm.org/D90108) will error on such an overridden symbol
> > binding.
> >
> > Commit ef1e03152cb0 ("x86/asm: Make some functions local") changed ENTRY in
> > arch/x86/lib/memcpy_64.S to SYM_FUNC_START_LOCAL, which was ineffective due to
> > the preceding .weak directive.
> >
> > Use the appropriate SYM_FUNC_START_WEAK instead.
> >
> > Fixes: 393f203f5fd5 ("x86_64: kasan: add interceptors for memset/memmove/memcpy functions")
> > Fixes: ef1e03152cb0 ("x86/asm: Make some functions local")
> > Reported-by: Sami Tolvanen <samito...@google.com>
> > Signed-off-by: Fangrui Song <mas...@google.com>
> > Cc: <sta...@vger.kernel.org>
>
> This resolves the build error I see with LLVM_IAS=1 and ToT LLVM.
>
> Tested-by: Nathan Chancellor <natecha...@gmail.com>

Reviewed-by: Nick Desaulniers <ndesau...@google.com>
Tested-by: Nick Desaulniers <ndesau...@google.com>

>

--
Thanks,
~Nick Desaulniers