| Message ID | 1484146493-18460-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 41c066f2c4d436c535616fe182331766c57838f0 |
| Headers | show |
Hi Ard, On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: > When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded > modules and the core kernel may exceed 4 GB, putting symbols exported > by the core kernel out of the reach of the ordinary adrp/add instruction > pairs used to generate relative symbol references. So make the adr_l > macro emit a movz/movk sequence instead when executing in module context. AFAICT, we only use adr_l in a few assembly files that shouldn't matter to modules: * arch/arm64/kernel/head.S * arch/arm64/kernel/sleep.S * arch/arm64/kvm/hyp-init.S * arch/arm64/kvm/hyp/hyp-entry.S ... so I don't follow why we need this. Have I missed something? Or do you intend to use this in module code in future? It seems somewhat surprising to me to have adr_l expand to something that doesn't use adr/adrp, but that's not necessarily a problem. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote: > Hi Ard, > > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded >> modules and the core kernel may exceed 4 GB, putting symbols exported >> by the core kernel out of the reach of the ordinary adrp/add instruction >> pairs used to generate relative symbol references. So make the adr_l >> macro emit a movz/movk sequence instead when executing in module context. > > AFAICT, we only use adr_l in a few assembly files that shouldn't matter > to modules: > > * arch/arm64/kernel/head.S > * arch/arm64/kernel/sleep.S > * arch/arm64/kvm/hyp-init.S > * arch/arm64/kvm/hyp/hyp-entry.S > > ... so I don't follow why we need this. > > Have I missed something? Or do you intend to use this in module code in > future? > Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses the lookup tables from crypto/aes_generic.c, which may be built into the core kernel, while the code itself may be built as a module. But in general, if the macro is available to modules, I would like to make sure that it does not result in code that builds fine but may fail in some cases only at runtime, especially given the fact that there is also a Cortex-A53 erratum regarding adrp instructions, for which reason we build modules with -mcmodel=large (which amounts to the same thing as the patch above) > It seems somewhat surprising to me to have adr_l expand to something > that doesn't use adr/adrp, but that's not necessarily a problem. > I did realise that, but I don't think it is a problem tbh. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11 January 2017 at 15:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote: >> Hi Ard, >> >> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: >>> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded >>> modules and the core kernel may exceed 4 GB, putting symbols exported >>> by the core kernel out of the reach of the ordinary adrp/add instruction >>> pairs used to generate relative symbol references. So make the adr_l >>> macro emit a movz/movk sequence instead when executing in module context. >> >> AFAICT, we only use adr_l in a few assembly files that shouldn't matter >> to modules: >> >> * arch/arm64/kernel/head.S >> * arch/arm64/kernel/sleep.S >> * arch/arm64/kvm/hyp-init.S >> * arch/arm64/kvm/hyp/hyp-entry.S >> >> ... so I don't follow why we need this. >> >> Have I missed something? Or do you intend to use this in module code in >> future? >> > > Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses > the lookup tables from crypto/aes_generic.c, which may be built into > the core kernel, while the code itself may be built as a module. > > But in general, if the macro is available to modules, I would like to > make sure that it does not result in code that builds fine but may > fail in some cases only at runtime, especially given the fact that > there is also a Cortex-A53 erratum regarding adrp instructions, for > which reason we build modules with -mcmodel=large (which amounts to > the same thing as the patch above) > Actually, we could test for defined(MODULE) && defined(CONFIG_ARM64_MODULE_CMODEL_LARGE) instead, so we revert to adrp/add pairs for modules if the erratum and KASLR are both disabled >> It seems somewhat surprising to me to have adr_l expand to something >> that doesn't use adr/adrp, but that's not necessarily a problem. >> > > I did realise that, but I don't think it is a problem tbh. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote: > On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Ard, > > > > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: > >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded > >> modules and the core kernel may exceed 4 GB, putting symbols exported > >> by the core kernel out of the reach of the ordinary adrp/add instruction > >> pairs used to generate relative symbol references. So make the adr_l > >> macro emit a movz/movk sequence instead when executing in module context. > > > > AFAICT, we only use adr_l in a few assembly files that shouldn't matter > > to modules: > > > > * arch/arm64/kernel/head.S > > * arch/arm64/kernel/sleep.S > > * arch/arm64/kvm/hyp-init.S > > * arch/arm64/kvm/hyp/hyp-entry.S > > > > ... so I don't follow why we need this. > > > > Have I missed something? Or do you intend to use this in module code in > > future? > > Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses > the lookup tables from crypto/aes_generic.c, which may be built into > the core kernel, while the code itself may be built as a module. Ah, ok. > But in general, if the macro is available to modules, I would like to > make sure that it does not result in code that builds fine but may > fail in some cases only at runtime, especially given the fact that > there is also a Cortex-A53 erratum regarding adrp instructions, for > which reason we build modules with -mcmodel=large (which amounts to > the same thing as the patch above) > > > It seems somewhat surprising to me to have adr_l expand to something > > that doesn't use adr/adrp, but that's not necessarily a problem. > > I did realise that, but I don't think it is a problem tbh. In this case it should be fine, certainly. There are cases like the early boot code and hyp code where it's critical that we use adr. It's also possible that we might build (modular) drivers which want some idmapped code, where we want adr, so it seems unfortunate that this depends on howthe code is built. So, maybe it's better to have a mov_sym helper for this case, to be explicit about what we want? That can use either adr* or mov*, or the latter consistently. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11 January 2017 at 15:34, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote: >> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote: >> > Hi Ard, >> > >> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: >> >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded >> >> modules and the core kernel may exceed 4 GB, putting symbols exported >> >> by the core kernel out of the reach of the ordinary adrp/add instruction >> >> pairs used to generate relative symbol references. So make the adr_l >> >> macro emit a movz/movk sequence instead when executing in module context. >> > >> > AFAICT, we only use adr_l in a few assembly files that shouldn't matter >> > to modules: >> > >> > * arch/arm64/kernel/head.S >> > * arch/arm64/kernel/sleep.S >> > * arch/arm64/kvm/hyp-init.S >> > * arch/arm64/kvm/hyp/hyp-entry.S >> > >> > ... so I don't follow why we need this. >> > >> > Have I missed something? Or do you intend to use this in module code in >> > future? >> >> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses >> the lookup tables from crypto/aes_generic.c, which may be built into >> the core kernel, while the code itself may be built as a module. > > Ah, ok. > >> But in general, if the macro is available to modules, I would like to >> make sure that it does not result in code that builds fine but may >> fail in some cases only at runtime, especially given the fact that >> there is also a Cortex-A53 erratum regarding adrp instructions, for >> which reason we build modules with -mcmodel=large (which amounts to >> the same thing as the patch above) >> >> > It seems somewhat surprising to me to have adr_l expand to something >> > that doesn't use adr/adrp, but that's not necessarily a problem. >> >> I did realise that, but I don't think it is a problem tbh. > > In this case it should be fine, certainly. > > There are cases like the early boot code and hyp code where it's > critical that we use adr. It's also possible that we might build > (modular) drivers which want some idmapped code, where we want adr, so > it seems unfortunate that this depends on howthe code is built. > How would /that/ work? Modules are vmalloc'ed, and not covered by the ID map to begin with, so it is impossible to execute those adr instructions in a way that would make them return anything other than the virtual address of the symbol they refer to. > So, maybe it's better to have a mov_sym helper for this case, to be > explicit about what we want? That can use either adr* or mov*, or the > latter consistently. > Well, the point is that adr_l should not be used for modules so adding something that may be used is fine, but that still leaves the risk that someone may end up using it in a module. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 11, 2017 at 04:08:30PM +0000, Ard Biesheuvel wrote: > On 11 January 2017 at 15:34, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote: > >> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote: > >> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: > >> But in general, if the macro is available to modules, I would like to > >> make sure that it does not result in code that builds fine but may > >> fail in some cases only at runtime, especially given the fact that > >> there is also a Cortex-A53 erratum regarding adrp instructions, for > >> which reason we build modules with -mcmodel=large (which amounts to > >> the same thing as the patch above) > >> > >> > It seems somewhat surprising to me to have adr_l expand to something > >> > that doesn't use adr/adrp, but that's not necessarily a problem. > >> > >> I did realise that, but I don't think it is a problem tbh. > > > > In this case it should be fine, certainly. > > > > There are cases like the early boot code and hyp code where it's > > critical that we use adr. It's also possible that we might build > > (modular) drivers which want some idmapped code, where we want adr, so > > it seems unfortunate that this depends on howthe code is built. > > How would /that/ work? Modules are vmalloc'ed, and not covered by the > ID map to begin with, so it is impossible to execute those adr > instructions in a way that would make them return anything other than > the virtual address of the symbol they refer to. That's a fair point. > > So, maybe it's better to have a mov_sym helper for this case, to be > > explicit about what we want? That can use either adr* or mov*, or the > > latter consistently. > > Well, the point is that adr_l should not be used for modules so adding > something that may be used is fine, but that still leaves the risk > that someone may end up using it in a module. Sure. Given you have a concrete use case, and all I have are some vague concerns for code that doesn't exist at present, I have no real objection to the patch as it stands. FWIW: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: > When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded > modules and the core kernel may exceed 4 GB, putting symbols exported > by the core kernel out of the reach of the ordinary adrp/add instruction > pairs used to generate relative symbol references. So make the adr_l > macro emit a movz/movk sequence instead when executing in module context. > > While at it, remove the pointless special case for the stack pointer. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/include/asm/assembler.h | 36 +++++++++++++++----- > 1 file changed, 27 insertions(+), 9 deletions(-) Given that you need this for 4.11, I suggest Catalin takes it as a fix for 4.10 to avoid the crypto dependency. Acked-by: Will Deacon <will.deacon@arm.com> Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Jan 12, 2017 at 03:44:20PM +0000, Will Deacon wrote: > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote: > > When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded > > modules and the core kernel may exceed 4 GB, putting symbols exported > > by the core kernel out of the reach of the ordinary adrp/add instruction > > pairs used to generate relative symbol references. So make the adr_l > > macro emit a movz/movk sequence instead when executing in module context. > > > > While at it, remove the pointless special case for the stack pointer. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/arm64/include/asm/assembler.h | 36 +++++++++++++++----- > > 1 file changed, 27 insertions(+), 9 deletions(-) > > Given that you need this for 4.11, I suggest Catalin takes it as a fix > for 4.10 to avoid the crypto dependency. > > Acked-by: Will Deacon <will.deacon@arm.com> That's fine by me. Thanks for the ack. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 446f6c46d4b1..3a4301163e04 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -164,22 +164,25 @@ lr .req x30 // link register /* * Pseudo-ops for PC-relative adr/ldr/str <reg>, <symbol> where - * <symbol> is within the range +/- 4 GB of the PC. + * <symbol> is within the range +/- 4 GB of the PC when running + * in core kernel context. In module context, a movz/movk sequence + * is used, since modules may be loaded far away from the kernel + * when KASLR is in effect. */ /* * @dst: destination register (64 bit wide) * @sym: name of the symbol - * @tmp: optional scratch register to be used if <dst> == sp, which - * is not allowed in an adrp instruction */ - .macro adr_l, dst, sym, tmp= - .ifb \tmp + .macro adr_l, dst, sym +#ifndef MODULE adrp \dst, \sym add \dst, \dst, :lo12:\sym - .else - adrp \tmp, \sym - add \dst, \tmp, :lo12:\sym - .endif +#else + movz \dst, #:abs_g3:\sym + movk \dst, #:abs_g2_nc:\sym + movk \dst, #:abs_g1_nc:\sym + movk \dst, #:abs_g0_nc:\sym +#endif .endm /* @@ -190,6 +193,7 @@ lr .req x30 // link register * the address */ .macro ldr_l, dst, sym, tmp= +#ifndef MODULE .ifb \tmp adrp \dst, \sym ldr \dst, [\dst, :lo12:\sym] @@ -197,6 +201,15 @@ lr .req x30 // link register adrp \tmp, \sym ldr \dst, [\tmp, :lo12:\sym] .endif +#else + .ifb \tmp + adr_l \dst, \sym + ldr \dst, [\dst] + .else + adr_l \tmp, \sym + ldr \dst, [\tmp] + .endif +#endif .endm /* @@ -206,8 +219,13 @@ lr .req x30 // link register * while <src> needs to be preserved. */ .macro str_l, src, sym, tmp +#ifndef MODULE adrp \tmp, \sym str \src, [\tmp, :lo12:\sym] +#else + adr_l \tmp, \sym + str \src, [\tmp] +#endif .endm /*
When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded modules and the core kernel may exceed 4 GB, putting symbols exported by the core kernel out of the reach of the ordinary adrp/add instruction pairs used to generate relative symbol references. So make the adr_l macro emit a movz/movk sequence instead when executing in module context. While at it, remove the pointless special case for the stack pointer. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/include/asm/assembler.h | 36 +++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel