* [PATCH] arm64: reduce trampoline data alignment
@ 2020-03-04 9:36 Rémi Denis-Courmont
2020-03-04 14:29 ` Mark Rutland
0 siblings, 1 reply; 5+ messages in thread
From: Rémi Denis-Courmont @ 2020-03-04 9:36 UTC (permalink / raw)
To: linux-arm-kernel
From: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
The trampoline data, currently consisting of two relocated pointers,
must be within a single page. However, there are no needs for it to
start a page.
This reduces the alignment to 16 bytes, which ensures that the 16 bytes
of data are in the same page.
This patch assumes that the page alignment was just a quick and dirty
trick to not worry about fixmap. If however the page alignment was
meant to present a data page with no other data than the trampoline's,
then both the current code and this patch are wrong.
Signed-off-by: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
---
arch/arm64/kernel/entry.S | 6 +++---
arch/arm64/mm/mmu.c | 5 ++---
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ae99bf8bb0ae..97d6f8d15b3b 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -805,9 +805,9 @@ alternative_else_nop_endif
2:
tramp_map_kernel x30
#ifdef CONFIG_RANDOMIZE_BASE
- adr x30, tramp_vectors + PAGE_SIZE
+ adrp x30, tramp_vectors + PAGE_SIZE
alternative_insn isb, nop, ARM64_WORKAROUND_QCOM_FALKOR_E1003
- ldr x30, [x30]
+ ldr x30, [x30, #:lo12:__entry_tramp_data_start]
#else
ldr x30, =vectors
#endif
@@ -858,7 +858,7 @@ END(tramp_exit_compat)
.popsection // .entry.tramp.text
#ifdef CONFIG_RANDOMIZE_BASE
.pushsection ".rodata", "a"
- .align PAGE_SHIFT
+ .align 4
.globl __entry_tramp_data_start
__entry_tramp_data_start:
.quad vectors
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 05ec8e5f1436..03e35572fb1e 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -599,9 +599,8 @@ static int __init map_entry_trampoline(void)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
extern char __entry_tramp_data_start[];
- __set_fixmap(FIX_ENTRY_TRAMP_DATA,
- __pa_symbol(__entry_tramp_data_start),
- PAGE_KERNEL_RO);
+ pa_start = __pa_symbol(__entry_tramp_data_start) & PAGE_MASK;
+ __set_fixmap(FIX_ENTRY_TRAMP_DATA, pa_start, PAGE_KERNEL_RO);
}
return 0;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: reduce trampoline data alignment
2020-03-04 9:36 [PATCH] arm64: reduce trampoline data alignment Rémi Denis-Courmont
@ 2020-03-04 14:29 ` Mark Rutland
2020-03-05 9:23 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2020-03-04 14:29 UTC (permalink / raw)
To: Rémi Denis-Courmont
Cc: catalin.marinas, will, james.morse, linux-arm-kernel
[adding arm64 folk]
On Wed, Mar 04, 2020 at 11:36:21AM +0200, Rémi Denis-Courmont wrote:
> From: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
>
> The trampoline data, currently consisting of two relocated pointers,
> must be within a single page. However, there are no needs for it to
> start a page.
>
> This reduces the alignment to 16 bytes, which ensures that the 16 bytes
> of data are in the same page.
>
> This patch assumes that the page alignment was just a quick and dirty
> trick to not worry about fixmap. If however the page alignment was
> meant to present a data page with no other data than the trampoline's,
> then both the current code and this patch are wrong.
I think that aligning it to a page was a simplification overall, not
just for the fixmap. However, I do agree that as the page isn't mapped
during EL0 execution, it's fine for other bits of .rodata to share the
page.
I also think that it's a bit scary that we rely on nothing being placed
in .rodata between __entry_tramp_data_start and
__sdei_asm_trampoline_next_handler, since macros could easily place
something between the two. Luckily NOKPROBE() adds stuff to .init.data,
but I had to check.
I think it would be better to add a new .entry.tramp.data section to
ensure that, which we can align appropriately in the linker script.
Thanks,
Mark.
>
> Signed-off-by: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
> ---
> arch/arm64/kernel/entry.S | 6 +++---
> arch/arm64/mm/mmu.c | 5 ++---
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index ae99bf8bb0ae..97d6f8d15b3b 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -805,9 +805,9 @@ alternative_else_nop_endif
> 2:
> tramp_map_kernel x30
> #ifdef CONFIG_RANDOMIZE_BASE
> - adr x30, tramp_vectors + PAGE_SIZE
> + adrp x30, tramp_vectors + PAGE_SIZE
> alternative_insn isb, nop, ARM64_WORKAROUND_QCOM_FALKOR_E1003
> - ldr x30, [x30]
> + ldr x30, [x30, #:lo12:__entry_tramp_data_start]
> #else
> ldr x30, =vectors
> #endif
> @@ -858,7 +858,7 @@ END(tramp_exit_compat)
> .popsection // .entry.tramp.text
> #ifdef CONFIG_RANDOMIZE_BASE
> .pushsection ".rodata", "a"
> - .align PAGE_SHIFT
> + .align 4
> .globl __entry_tramp_data_start
> __entry_tramp_data_start:
> .quad vectors
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 05ec8e5f1436..03e35572fb1e 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -599,9 +599,8 @@ static int __init map_entry_trampoline(void)
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> extern char __entry_tramp_data_start[];
>
> - __set_fixmap(FIX_ENTRY_TRAMP_DATA,
> - __pa_symbol(__entry_tramp_data_start),
> - PAGE_KERNEL_RO);
> + pa_start = __pa_symbol(__entry_tramp_data_start) & PAGE_MASK;
> + __set_fixmap(FIX_ENTRY_TRAMP_DATA, pa_start, PAGE_KERNEL_RO);
> }
>
> return 0;
> --
> 2.25.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists•infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: reduce trampoline data alignment
2020-03-04 14:29 ` Mark Rutland
@ 2020-03-05 9:23 ` Will Deacon
2020-03-05 10:10 ` Remi Denis-Courmont
2020-03-05 16:34 ` Mark Rutland
0 siblings, 2 replies; 5+ messages in thread
From: Will Deacon @ 2020-03-05 9:23 UTC (permalink / raw)
To: Mark Rutland
Cc: catalin.marinas, james.morse, linux-arm-kernel,
Rémi Denis-Courmont
On Wed, Mar 04, 2020 at 02:29:51PM +0000, Mark Rutland wrote:
> [adding arm64 folk]
>
> On Wed, Mar 04, 2020 at 11:36:21AM +0200, Rémi Denis-Courmont wrote:
> > From: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
> >
> > The trampoline data, currently consisting of two relocated pointers,
> > must be within a single page. However, there are no needs for it to
> > start a page.
> >
> > This reduces the alignment to 16 bytes, which ensures that the 16 bytes
> > of data are in the same page.
I don't follow this bit: it's one 8 byte pointer.
> > This patch assumes that the page alignment was just a quick and dirty
> > trick to not worry about fixmap. If however the page alignment was
> > meant to present a data page with no other data than the trampoline's,
> > then both the current code and this patch are wrong.
>
> I think that aligning it to a page was a simplification overall, not
> just for the fixmap. However, I do agree that as the page isn't mapped
> during EL0 execution, it's fine for other bits of .rodata to share the
> page.
>
> I also think that it's a bit scary that we rely on nothing being placed
> in .rodata between __entry_tramp_data_start and
> __sdei_asm_trampoline_next_handler, since macros could easily place
> something between the two. Luckily NOKPROBE() adds stuff to .init.data,
> but I had to check.
>
> I think it would be better to add a new .entry.tramp.data section to
> ensure that, which we can align appropriately in the linker script.
Remi's patch looks fine though, no? (modulo the confusing commit message)
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: reduce trampoline data alignment
2020-03-05 9:23 ` Will Deacon
@ 2020-03-05 10:10 ` Remi Denis-Courmont
2020-03-05 16:34 ` Mark Rutland
1 sibling, 0 replies; 5+ messages in thread
From: Remi Denis-Courmont @ 2020-03-05 10:10 UTC (permalink / raw)
To: Will Deacon; +Cc: Mark Rutland, catalin.marinas, james.morse, linux-arm-kernel
Le 2020-03-05 11:23, Will Deacon a écrit :
> On Wed, Mar 04, 2020 at 02:29:51PM +0000, Mark Rutland wrote:
>> [adding arm64 folk]
>>
>> On Wed, Mar 04, 2020 at 11:36:21AM +0200, Rémi Denis-Courmont wrote:
>> > From: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
>> >
>> > The trampoline data, currently consisting of two relocated pointers,
>> > must be within a single page. However, there are no needs for it to
>> > start a page.
>> >
>> > This reduces the alignment to 16 bytes, which ensures that the 16 bytes
>> > of data are in the same page.
>
> I don't follow this bit: it's one 8 byte pointer.
I guess I misread the quad pseudo op as 4x4 instead of 4x2 bytes.
So it might actually be possible to remove the align stanza
completely...
>> > This patch assumes that the page alignment was just a quick and dirty
>> > trick to not worry about fixmap. If however the page alignment was
>> > meant to present a data page with no other data than the trampoline's,
>> > then both the current code and this patch are wrong.
>>
>> I think that aligning it to a page was a simplification overall, not
>> just for the fixmap. However, I do agree that as the page isn't mapped
>> during EL0 execution, it's fine for other bits of .rodata to share the
>> page.
>>
>> I also think that it's a bit scary that we rely on nothing being
>> placed
>> in .rodata between __entry_tramp_data_start and
>> __sdei_asm_trampoline_next_handler, since macros could easily place
>> something between the two. Luckily NOKPROBE() adds stuff to
>> .init.data,
>> but I had to check.
>>
>> I think it would be better to add a new .entry.tramp.data section to
>> ensure that, which we can align appropriately in the linker script.
>
> Remi's patch looks fine though, no? (modulo the confusing commit
> message)
... though now I'm not sure if we want to have a separate .tramp.rodata
section, a separate page within .rodata, or just a fixed version of this
patch?
I suppose it's a tradeoff between peace of mind regarding leaking kernel
read-only data in the trampoline mappings versus memory use?
--
Rémi Denis-Courmont
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: reduce trampoline data alignment
2020-03-05 9:23 ` Will Deacon
2020-03-05 10:10 ` Remi Denis-Courmont
@ 2020-03-05 16:34 ` Mark Rutland
1 sibling, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2020-03-05 16:34 UTC (permalink / raw)
To: Will Deacon
Cc: catalin.marinas, james.morse, linux-arm-kernel,
Rémi Denis-Courmont
On Thu, Mar 05, 2020 at 09:23:54AM +0000, Will Deacon wrote:
> On Wed, Mar 04, 2020 at 02:29:51PM +0000, Mark Rutland wrote:
> > [adding arm64 folk]
> >
> > On Wed, Mar 04, 2020 at 11:36:21AM +0200, Rémi Denis-Courmont wrote:
> > > From: Remi Denis-Courmont <remi.denis.courmont@huawei•com>
> > >
> > > The trampoline data, currently consisting of two relocated pointers,
> > > must be within a single page. However, there are no needs for it to
> > > start a page.
> > >
> > > This reduces the alignment to 16 bytes, which ensures that the 16 bytes
> > > of data are in the same page.
>
> I don't follow this bit: it's one 8 byte pointer.
When the kernel is built with SDEI, there are two 8-byte pointers that
need to live in the same page: the one at __entry_tramp_data_start and
the one at __sdei_asm_trampoline_next_handler.
Reducing the alignment to 8 would mean that the SDEI pointer could get
placed in a separate page, and we'd generate a bogus address in
__sdei_asm_entry_trampoline.
> > > This patch assumes that the page alignment was just a quick and dirty
> > > trick to not worry about fixmap. If however the page alignment was
> > > meant to present a data page with no other data than the trampoline's,
> > > then both the current code and this patch are wrong.
> >
> > I think that aligning it to a page was a simplification overall, not
> > just for the fixmap. However, I do agree that as the page isn't mapped
> > during EL0 execution, it's fine for other bits of .rodata to share the
> > page.
> >
> > I also think that it's a bit scary that we rely on nothing being placed
> > in .rodata between __entry_tramp_data_start and
> > __sdei_asm_trampoline_next_handler, since macros could easily place
> > something between the two. Luckily NOKPROBE() adds stuff to .init.data,
> > but I had to check.
> >
> > I think it would be better to add a new .entry.tramp.data section to
> > ensure that, which we can align appropriately in the linker script.
>
> Remi's patch looks fine though, no? (modulo the confusing commit message)
It looks fine, I just think that if we're going to clean this up it'd be
good to make it more robust. Given how easy it is to miss the SDEI case
(as above), it's likely that'll get broken, and it's rarely tested.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-05 16:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-04 9:36 [PATCH] arm64: reduce trampoline data alignment Rémi Denis-Courmont
2020-03-04 14:29 ` Mark Rutland
2020-03-05 9:23 ` Will Deacon
2020-03-05 10:10 ` Remi Denis-Courmont
2020-03-05 16:34 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox