| Message ID | CAKwvOdkK1LgLC4ChptzUTC45WvE9-Sn0OqtgF7-odNSw8xLTYA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | 5.4 and 4.19 fix for LLVM_IAS/clang-12 | expand |
On 09. 12. 20, 1:12, Nick Desaulniers wrote: > Dear stable kernel maintainers, > Please consider accepting the following backport to 5.4 and 4.19 of > commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for > arch/x86/lib/mem*_64.S"), attached. > > The patch to 5.4 had a conflict due to 5.4 missing upstream commit > e9b9d020c487 ("x86/asm: Annotate aliases") which first landed in > v5.5-rc1. > > The patch to 4.19 had a conflict due to 4.19 missing the above commit > and ffedeeb780dc ("linkage: Introduce new macros for assembler > symbols") which also first landed in v5.5-rc1 but was backported to > linux-5.4.y as commit 840d8c9b3e5f ("linkage: Introduce new macros for > assembler symbols") which shipped in v5.4.76. > > This patch fixes a build error from clang's assembler when building > with Clang-12, which now errors when symbols are redeclared with > different bindings. We're using clang's assembler in Android and > ChromeOS for 4.19+. > > Jiri, would you mind reviewing the 4.19 patch (or both)? It simply > open codes what the upstream macros would expand to; this can be and > was observed from running: You don't have to touch (expand) __memcpy, __memmove, and __memset, right? thanks, -- js suse labs
On 09. 12. 20, 8:20, Jiri Slaby wrote: > On 09. 12. 20, 1:12, Nick Desaulniers wrote: >> Dear stable kernel maintainers, >> Please consider accepting the following backport to 5.4 and 4.19 of >> commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for >> arch/x86/lib/mem*_64.S"), attached. >> >> The patch to 5.4 had a conflict due to 5.4 missing upstream commit >> e9b9d020c487 ("x86/asm: Annotate aliases") which first landed in >> v5.5-rc1. >> >> The patch to 4.19 had a conflict due to 4.19 missing the above commit >> and ffedeeb780dc ("linkage: Introduce new macros for assembler >> symbols") which also first landed in v5.5-rc1 but was backported to >> linux-5.4.y as commit 840d8c9b3e5f ("linkage: Introduce new macros for >> assembler symbols") which shipped in v5.4.76. >> >> This patch fixes a build error from clang's assembler when building >> with Clang-12, which now errors when symbols are redeclared with >> different bindings. We're using clang's assembler in Android and >> ChromeOS for 4.19+. >> >> Jiri, would you mind reviewing the 4.19 patch (or both)? It simply >> open codes what the upstream macros would expand to; this can be and >> was observed from running: > > You don't have to touch (expand) __memcpy, __memmove, and __memset, right? Also, no need for doubled p2align. > thanks, -- js
On Tue, Dec 8, 2020 at 11:21 PM Jiri Slaby <jirislaby@kernel.org> wrote: > > On 09. 12. 20, 8:20, Jiri Slaby wrote: > > On 09. 12. 20, 1:12, Nick Desaulniers wrote: > >> Dear stable kernel maintainers, > >> Please consider accepting the following backport to 5.4 and 4.19 of > >> commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for > >> arch/x86/lib/mem*_64.S"), attached. Also, first landed in v5.10-rc3. Exists in v5.9.7 as 305da744c138487864a46b2fb0bd7cf54e1e03b4. > >> > >> The patch to 5.4 had a conflict due to 5.4 missing upstream commit > >> e9b9d020c487 ("x86/asm: Annotate aliases") which first landed in > >> v5.5-rc1. > >> > >> The patch to 4.19 had a conflict due to 4.19 missing the above commit > >> and ffedeeb780dc ("linkage: Introduce new macros for assembler > >> symbols") which also first landed in v5.5-rc1 but was backported to > >> linux-5.4.y as commit 840d8c9b3e5f ("linkage: Introduce new macros for > >> assembler symbols") which shipped in v5.4.76. > >> > >> This patch fixes a build error from clang's assembler when building > >> with Clang-12, which now errors when symbols are redeclared with > >> different bindings. We're using clang's assembler in Android and > >> ChromeOS for 4.19+. > >> > >> Jiri, would you mind reviewing the 4.19 patch (or both)? It simply > >> open codes what the upstream macros would expand to; this can be and > >> was observed from running: > > > > You don't have to touch (expand) __memcpy, __memmove, and __memset, right? Sure, attached a v2. It's actually a little cleaner (smaller diffstat) that way. > > Also, no need for doubled p2align. eh, maybe, but 1. it's what SYM_FUNC_START expands to precisely; removing it would be (too) different from what upstream is doing IMO. 2. it's obviously doubled when in v1 I was expanding the __ prefixed symbols as well; not so obvious in v2. 3. the order of __ prefixed symbols differs between these three; I'd rather not rely on the order or have to reorder them to get the initial symbol realigned; specifying the alignment for both seems safer to me. So I haven't included that suggestion in v2, but please feel free to convince me otherwise if you feel strongly. Thanks for taking the time to review. I appreciate it. -- Thanks, ~Nick Desaulniers From 5707bea0fcef6263d3b23c5f0b61e6a289d54c22 Mon Sep 17 00:00:00 2001 From: Fangrui Song <maskray@google.com> Date: Mon, 2 Nov 2020 17:23:58 -0800 Subject: [PATCH] x86/lib: Change .weak to SYM_FUNC_START_WEAK for arch/x86/lib/mem*_64.S commit 4d6ffa27b8e5116c0abb318790fd01d4e12d75e6 upstream. 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 <samitolvanen@google.com> Signed-off-by: Fangrui Song <maskray@google.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Cc: <stable@vger.kernel.org> Link: https://lkml.kernel.org/r/20201103012358.168682-1-maskray@google.com [nd: backport due to missing commit e9b9d020c487 ("x86/asm: Annotate aliases") commit ffedeeb780dc ("linkage: Introduce new macros for assembler symbols")] Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- arch/x86/lib/memcpy_64.S | 6 +++--- arch/x86/lib/memmove_64.S | 4 ++-- arch/x86/lib/memset_64.S | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S index 9d05572370ed..84b0078272d1 100644 --- a/arch/x86/lib/memcpy_64.S +++ b/arch/x86/lib/memcpy_64.S @@ -14,8 +14,6 @@ * to a jmp to memcpy_erms which does the REP; MOVSB mem copy. */ -.weak memcpy - /* * memcpy - Copy a memory block. * @@ -28,7 +26,9 @@ * rax original destination */ ENTRY(__memcpy) -ENTRY(memcpy) +.weak memcpy +.p2align 4, 0x90 +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 bbec69d8223b..e1cfc880f42d 100644 --- a/arch/x86/lib/memmove_64.S +++ b/arch/x86/lib/memmove_64.S @@ -25,8 +25,8 @@ * rax: dest */ .weak memmove - -ENTRY(memmove) +.p2align 4, 0x90 +memmove: ENTRY(__memmove) /* Handle more 32 bytes in loop */ diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S index 9bc861c71e75..084189acdcd0 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,9 @@ * * rax original destination */ -ENTRY(memset) +.weak memset +.p2align 4, 0x90 +memset: ENTRY(__memset) /* * Some CPUs support enhanced REP MOVSB/STOSB feature. It is recommended
On Thu, Dec 10, 2020 at 01:15:29PM -0800, Nick Desaulniers wrote: > On Tue, Dec 8, 2020 at 11:21 PM Jiri Slaby <jirislaby@kernel.org> wrote: > > > > On 09. 12. 20, 8:20, Jiri Slaby wrote: > > > On 09. 12. 20, 1:12, Nick Desaulniers wrote: > > >> Dear stable kernel maintainers, > > >> Please consider accepting the following backport to 5.4 and 4.19 of > > >> commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for > > >> arch/x86/lib/mem*_64.S"), attached. > > Also, first landed in v5.10-rc3. Exists in v5.9.7 as > 305da744c138487864a46b2fb0bd7cf54e1e03b4. > > > >> > > >> The patch to 5.4 had a conflict due to 5.4 missing upstream commit > > >> e9b9d020c487 ("x86/asm: Annotate aliases") which first landed in > > >> v5.5-rc1. > > >> > > >> The patch to 4.19 had a conflict due to 4.19 missing the above commit > > >> and ffedeeb780dc ("linkage: Introduce new macros for assembler > > >> symbols") which also first landed in v5.5-rc1 but was backported to > > >> linux-5.4.y as commit 840d8c9b3e5f ("linkage: Introduce new macros for > > >> assembler symbols") which shipped in v5.4.76. > > >> > > >> This patch fixes a build error from clang's assembler when building > > >> with Clang-12, which now errors when symbols are redeclared with > > >> different bindings. We're using clang's assembler in Android and > > >> ChromeOS for 4.19+. > > >> > > >> Jiri, would you mind reviewing the 4.19 patch (or both)? It simply > > >> open codes what the upstream macros would expand to; this can be and > > >> was observed from running: > > > > > > You don't have to touch (expand) __memcpy, __memmove, and __memset, right? > > Sure, attached a v2. It's actually a little cleaner (smaller > diffstat) that way. Applied, thanks. greg k-h
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S index 92748660ba51..29f1eecffdfd 100644 --- a/arch/x86/lib/memcpy_64.S +++ b/arch/x86/lib/memcpy_64.S @@ -15,8 +15,6 @@ * to a jmp to memcpy_erms which does the REP; MOVSB mem copy. */ -.weak memcpy - /* * memcpy - Copy a memory block. * @@ -28,8 +26,8 @@ * Output: * rax original destination */ -ENTRY(__memcpy) -ENTRY(memcpy) +SYM_FUNC_START_ALIAS(__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 bbec69d8223b..f8deefb5a994 100644 --- a/arch/x86/lib/memmove_64.S +++ b/arch/x86/lib/memmove_64.S @@ -24,10 +24,8 @@ * Output: * rax: dest */ -.weak memmove - -ENTRY(memmove) -ENTRY(__memmove) +SYM_FUNC_START_WEAK(memmove) +SYM_FUNC_START(__memmove) /* Handle more 32 bytes in loop */ mov %rdi, %rax diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S index 9bc861c71e75..65837f7eeb32 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,8 +17,8 @@ * * rax original destination */ -ENTRY(memset) -ENTRY(__memset) +SYM_FUNC_START_WEAK(memset) +SYM_FUNC_START(__memset) /* * Some CPUs support enhanced REP MOVSB/STOSB feature. It is recommended * to use it when possible. If not available, use fast string instructions.
Dear stable kernel maintainers, Please consider accepting the following backport to 5.4 and 4.19 of commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for arch/x86/lib/mem*_64.S"), attached. The patch to 5.4 had a conflict due to 5.4 missing upstream commit e9b9d020c487 ("x86/asm: Annotate aliases") which first landed in v5.5-rc1. The patch to 4.19 had a conflict due to 4.19 missing the above commit and ffedeeb780dc ("linkage: Introduce new macros for assembler symbols") which also first landed in v5.5-rc1 but was backported to linux-5.4.y as commit 840d8c9b3e5f ("linkage: Introduce new macros for assembler symbols") which shipped in v5.4.76. This patch fixes a build error from clang's assembler when building with Clang-12, which now errors when symbols are redeclared with different bindings. We're using clang's assembler in Android and ChromeOS for 4.19+. Jiri, would you mind reviewing the 4.19 patch (or both)? It simply open codes what the upstream macros would expand to; this can be and was observed from running: $ make ... arch/x86/lib/memmove_64.s (ie. lowercase s, to invoke the C preprocessor on the uppercase .S file) See also: https://github.com/ClangBuiltLinux/linux/issues/1190. -- Thanks, ~Nick Desaulniers From fecddb0ab4ab58b69e29cb936b05336574cb029f Mon Sep 17 00:00:00 2001 From: Fangrui Song <maskray@google.com> Date: Mon, 2 Nov 2020 17:23:58 -0800 Subject: [PATCH] x86/lib: Change .weak to SYM_FUNC_START_WEAK for arch/x86/lib/mem*_64.S commit 4d6ffa27b8e5116c0abb318790fd01d4e12d75e6 upstream. 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 <samitolvanen@google.com> Signed-off-by: Fangrui Song <maskray@google.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Cc: <stable@vger.kernel.org> Link: https://lkml.kernel.org/r/20201103012358.168682-1-maskray@google.com [nd: backport due to missing commit e9b9d020c487 ("x86/asm: Annotate aliases")] Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- arch/x86/lib/memcpy_64.S | 6 ++---- arch/x86/lib/memmove_64.S | 6 ++---- arch/x86/lib/memset_64.S | 6 ++---- 3 files changed, 6 insertions(+), 12 deletions(-)