diff mbox

[v2,08/18] arm64: don't disable ADR_PREL_PG_HI21* with ARM64_ERRATUM_843419

Message ID 20171115213428.22559-9-samitolvanen@google.com
State New, archived
Headers show

Commit Message

Sami Tolvanen Nov. 15, 2017, 9:34 p.m. UTC
CONFIG_LTO_CLANG depends on GNU gold, which can generate ADR_PREL_PG_HI21*
relocations even with --fix-cortex-a53-843419.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/kernel/module.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Ard Biesheuvel Nov. 15, 2017, 10:29 p.m. UTC | #1
On 15 November 2017 at 21:34, Sami Tolvanen <samitolvanen@google.com> wrote:
> CONFIG_LTO_CLANG depends on GNU gold, which can generate ADR_PREL_PG_HI21*
> relocations even with --fix-cortex-a53-843419.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/kernel/module.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f469e0435903..fec9a578f122 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -336,14 +336,12 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>                         ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 0, 21,
>                                              AARCH64_INSN_IMM_ADR);
>                         break;
> -#ifndef CONFIG_ARM64_ERRATUM_843419
>                 case R_AARCH64_ADR_PREL_PG_HI21_NC:
>                         overflow_check = false;
>                 case R_AARCH64_ADR_PREL_PG_HI21:
>                         ovf = reloc_insn_imm(RELOC_OP_PAGE, loc, val, 12, 21,
>                                              AARCH64_INSN_IMM_ADR);
>                         break;
> -#endif
>                 case R_AARCH64_ADD_ABS_LO12_NC:
>                 case R_AARCH64_LDST8_ABS_LO12_NC:
>                         overflow_check = false;
> --
> 2.15.0.448.gf294e3d99a-goog
>

I think this change is reasonable in itself, but i do wonder how we
can ensure that the adrp instructions that GNU gold does generate are
not affected by the erratum, given that modules are partially linked
object files, not shared libraries.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Will Deacon Nov. 16, 2017, 11:44 a.m. UTC | #2
On Wed, Nov 15, 2017 at 10:29:12PM +0000, Ard Biesheuvel wrote:
> On 15 November 2017 at 21:34, Sami Tolvanen <samitolvanen@google.com> wrote:
> > CONFIG_LTO_CLANG depends on GNU gold, which can generate ADR_PREL_PG_HI21*
> > relocations even with --fix-cortex-a53-843419.
> >
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> >  arch/arm64/kernel/module.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> > index f469e0435903..fec9a578f122 100644
> > --- a/arch/arm64/kernel/module.c
> > +++ b/arch/arm64/kernel/module.c
> > @@ -336,14 +336,12 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> >                         ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 0, 21,
> >                                              AARCH64_INSN_IMM_ADR);
> >                         break;
> > -#ifndef CONFIG_ARM64_ERRATUM_843419
> >                 case R_AARCH64_ADR_PREL_PG_HI21_NC:
> >                         overflow_check = false;
> >                 case R_AARCH64_ADR_PREL_PG_HI21:
> >                         ovf = reloc_insn_imm(RELOC_OP_PAGE, loc, val, 12, 21,
> >                                              AARCH64_INSN_IMM_ADR);
> >                         break;
> > -#endif
> >                 case R_AARCH64_ADD_ABS_LO12_NC:
> >                 case R_AARCH64_LDST8_ABS_LO12_NC:
> >                         overflow_check = false;
> > --
> > 2.15.0.448.gf294e3d99a-goog
> >
> 
> I think this change is reasonable in itself, but i do wonder how we
> can ensure that the adrp instructions that GNU gold does generate are
> not affected by the erratum, given that modules are partially linked
> object files, not shared libraries.

Right, and this would also mean that we silently load vulnerable modules
that are linked with either LD that doesn't support --fix-cortex-a53-843419
or simply wasn't passed.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Tolvanen Nov. 16, 2017, 4:32 p.m. UTC | #3
On Thu, Nov 16, 2017 at 11:44:06AM +0000, Will Deacon wrote:
> Right, and this would also mean that we silently load vulnerable
> modules that are linked with either LD that doesn't support
> --fix-cortex-a53-843419 or simply wasn't passed.

You'll see a warning at least if the linker doesn't support the flag,
but yes, you're correct. In v1 of this patch set, LTO depended on this
erratum not being selected, but I changed it in v2 based on Ard's
suggestion.

I'm fine with not being able to use LTO on devices that are affected
by this erratum, so either option works for me. I can even change
this so the user must explicitly disable the erratum in order to use
LTO. Thoughts?

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Nov. 16, 2017, 4:34 p.m. UTC | #4
On 16 November 2017 at 16:32, Sami Tolvanen <samitolvanen@google.com> wrote:
> On Thu, Nov 16, 2017 at 11:44:06AM +0000, Will Deacon wrote:
>> Right, and this would also mean that we silently load vulnerable
>> modules that are linked with either LD that doesn't support
>> --fix-cortex-a53-843419 or simply wasn't passed.
>
> You'll see a warning at least if the linker doesn't support the flag,
> but yes, you're correct. In v1 of this patch set, LTO depended on this
> erratum not being selected, but I changed it in v2 based on Ard's
> suggestion.
>
> I'm fine with not being able to use LTO on devices that are affected
> by this erratum, so either option works for me. I can even change
> this so the user must explicitly disable the erratum in order to use
> LTO. Thoughts?
>

You still have not explained to us how GOLD avoids the erratum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Tolvanen Nov. 16, 2017, 9:37 p.m. UTC | #5
On Thu, Nov 16, 2017 at 04:34:03PM +0000, Ard Biesheuvel wrote:
> You still have not explained to us how GOLD avoids the erratum.

Sorry, I didn't realize you were asking that. If gold spots erratum
sequences, looks like it creates stubs to break them up:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l8396

It also attempts to optimize the code by replacing adrps in these
sequences with adr where possible, but otherwise doesn't appear to
touch them:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l2053

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Nov. 16, 2017, 10:14 p.m. UTC | #6
On 16 November 2017 at 21:37, Sami Tolvanen <samitolvanen@google.com> wrote:
> On Thu, Nov 16, 2017 at 04:34:03PM +0000, Ard Biesheuvel wrote:
>> You still have not explained to us how GOLD avoids the erratum.
>
> Sorry, I didn't realize you were asking that. If gold spots erratum
> sequences, looks like it creates stubs to break them up:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l8396
>
> It also attempts to optimize the code by replacing adrps in these
> sequences with adr where possible, but otherwise doesn't appear to
> touch them:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l2053
>

OK, so my concern here is that this code probably only operates on
fully linked binaries, and not partially linked object files like
kernel modules. The same applies to ld.bfd, which is why we need to
use the large module instead of a code model that may emit adrp
instructions..

What is preventing us from using the large model with clang? I know it
uses movk/movz pairs rather than literals, but this shouldn't matter
for modules, given that we support static ELF relocations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Nov. 16, 2017, 10:25 p.m. UTC | #7
On 16 November 2017 at 22:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 16 November 2017 at 21:37, Sami Tolvanen <samitolvanen@google.com> wrote:
>> On Thu, Nov 16, 2017 at 04:34:03PM +0000, Ard Biesheuvel wrote:
>>> You still have not explained to us how GOLD avoids the erratum.
>>
>> Sorry, I didn't realize you were asking that. If gold spots erratum
>> sequences, looks like it creates stubs to break them up:
>>
>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l8396
>>
>> It also attempts to optimize the code by replacing adrps in these
>> sequences with adr where possible, but otherwise doesn't appear to
>> touch them:
>>
>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gold/aarch64.cc#l2053
>>
>
> OK, so my concern here is that this code probably only operates on
> fully linked binaries, and not partially linked object files like
> kernel modules. The same applies to ld.bfd, which is why we need to
> use the large module

*model* not module

> instead of a code model that may emit adrp
> instructions..
>
> What is preventing us from using the large model with clang? I know it
> uses movk/movz pairs rather than literals, but this shouldn't matter
> for modules, given that we support static ELF relocations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Tolvanen Nov. 16, 2017, 11:09 p.m. UTC | #8
On Thu, Nov 16, 2017 at 10:14:17PM +0000, Ard Biesheuvel wrote:
> OK, so my concern here is that this code probably only operates on
> fully linked binaries, and not partially linked object files like
> kernel modules.

Right. That makes sense.

> What is preventing us from using the large model with clang?

We pass -mcmodel=large to clang, but I just confirmed that the
attribute isn't stored in LLVM IR, which means it's not used during
link time compilation. I'll see if we can solve this problem by
passing the correct code model directly to LLVMgold instead. Thanks
for pointing this out.

Sami

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Nov. 16, 2017, 11:20 p.m. UTC | #9
On 16 November 2017 at 23:09, Sami Tolvanen <samitolvanen@google.com> wrote:
> On Thu, Nov 16, 2017 at 10:14:17PM +0000, Ard Biesheuvel wrote:
>> OK, so my concern here is that this code probably only operates on
>> fully linked binaries, and not partially linked object files like
>> kernel modules.
>
> Right. That makes sense.
>
>> What is preventing us from using the large model with clang?
>
> We pass -mcmodel=large to clang, but I just confirmed that the
> attribute isn't stored in LLVM IR, which means it's not used during
> link time compilation. I'll see if we can solve this problem by
> passing the correct code model directly to LLVMgold instead. Thanks
> for pointing this out.
>

So at which point is the IR in a partially linked object file
converted into executable code?
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Tolvanen Nov. 16, 2017, 11:50 p.m. UTC | #10
On Thu, Nov 16, 2017 at 11:20:40PM +0000, Ard Biesheuvel wrote:
> So at which point is the IR in a partially linked object file
> converted into executable code?

At the final module link step (cmd_ld_ko_o) in scripts/Makefile.modpost,
added in patch 12.

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Nov. 17, 2017, 9:54 a.m. UTC | #11
On 16 November 2017 at 23:50, Sami Tolvanen <samitolvanen@google.com> wrote:
> On Thu, Nov 16, 2017 at 11:20:40PM +0000, Ard Biesheuvel wrote:
>> So at which point is the IR in a partially linked object file
>> converted into executable code?
>
> At the final module link step (cmd_ld_ko_o) in scripts/Makefile.modpost,
> added in patch 12.
>

OK, so all IR objects are converted into a single .o file
encapsulating the module image. Does this give the same benefits as
LTO linking IR objects to a fully linked executable? Even if it does,
partial linking is not something the toolchain people are usually
crazy about, so it would be nice to have some confirmation that this
is a usage model that is fully supported.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Tolvanen Nov. 17, 2017, 6:49 p.m. UTC | #12
On Fri, Nov 17, 2017 at 09:54:48AM +0000, Ard Biesheuvel wrote:
> OK, so all IR objects are converted into a single .o file
> encapsulating the module image. Does this give the same benefits as
> LTO linking IR objects to a fully linked executable?

Yes, it does.

> Even if it does, partial linking is not something the toolchain
> people are usually crazy about, so it would be nice to have some
> confirmation that this is a usage model that is fully supported.

I confirmed with our LLVM developers that while this is less common,
it's fully supported.

I will also drop this patch in v3 as passing code model to LLVMgold
fixes the issue with LTO.

Sami
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f469e0435903..fec9a578f122 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -336,14 +336,12 @@  int apply_relocate_add(Elf64_Shdr *sechdrs,
 			ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 0, 21,
 					     AARCH64_INSN_IMM_ADR);
 			break;
-#ifndef CONFIG_ARM64_ERRATUM_843419
 		case R_AARCH64_ADR_PREL_PG_HI21_NC:
 			overflow_check = false;
 		case R_AARCH64_ADR_PREL_PG_HI21:
 			ovf = reloc_insn_imm(RELOC_OP_PAGE, loc, val, 12, 21,
 					     AARCH64_INSN_IMM_ADR);
 			break;
-#endif
 		case R_AARCH64_ADD_ABS_LO12_NC:
 		case R_AARCH64_LDST8_ABS_LO12_NC:
 			overflow_check = false;