| Message ID | 20171115213428.22559-9-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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.
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
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.
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.
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
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?
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
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.
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
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;
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(-)