Re: [PATCH] arm64: reduce trampoline data alignment - Remi Denis-Courmont

From: Remi Denis-Courmont <remi@remlab•net>
To: Will Deacon <will@kernel•org>
Cc: Mark Rutland <mark.rutland@arm•com>,
	catalin.marinas@arm•com, james.morse@arm•com,
	linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH] arm64: reduce trampoline data alignment
Date: Thu, 05 Mar 2020 12:10:59 +0200	[thread overview]
Message-ID: <43edb84793aaa43a0ce2774b3df19e2b@remlab.net> (raw)
In-Reply-To: <20200305092353.GC19208@willie-the-truck>

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