On Wed, Sep 04, 2024 at 12:32:21PM +0100, Joey Gouly wrote:
On Wed, Sep 04, 2024 at 11:22:54AM +0100, Will Deacon wrote:
On Tue, Sep 03, 2024 at 03:54:13PM +0100, Joey Gouly wrote:
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote: > On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote: > > On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote: > > > On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote: > > > > +static void permission_overlay_switch(struct task_struct *next) > > > > +{ > > > > + if (!system_supports_poe()) > > > > + return; > > > > + > > > > + current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0); > > > > + if (current->thread.por_el0 != next->thread.por_el0) { > > > > + write_sysreg_s(next->thread.por_el0, SYS_POR_EL0); > > > > + /* ISB required for kernel uaccess routines when chaning POR_EL0 */ > > > > > > nit: typo "chaning". > > > > > > But more substantially, is this just to prevent spurious faults in the > > > context of a new thread using a stale value for POR_EL0? > > > > Not just prevent faults but enforce the permissions from the new > > thread's POR_EL0. The kernel may continue with a uaccess routine from > > here, we can't tell.
[...]
> So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in that case?
Yes, it will retry and this should be fine (I assume you are only talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's slightly more predictable for the user but, OTOH, some syscalls like readv() could be routed through GUP with no checks. As with MTE, we don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a single ISB at the end. In the meantime, I'm fine with dropping the ISB here.
commit 3141fb86bee8d48ae47cab1594dad54f974a8899 Author: Joey Gouly joey.gouly@arm.com Date: Tue Sep 3 15:47:26 2024 +0100
fixup! arm64: context switch POR_EL0 registerdiff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index a3a61ecdb165..c224b0955f1a 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -515,11 +515,8 @@ static void permission_overlay_switch(struct task_struct *next) return;
current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
if (current->thread.por_el0 != next->thread.por_el0) {
if (current->thread.por_el0 != next->thread.por_el0) write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
/* ISB required for kernel uaccess routines when chaning POR_EL0 */isb();}}
What about the one in flush_poe()? I'm inclined to drop that as well.
Yes I guess that one can be removed too. Catalin any comments?
Will, do you want me to re-send the series with this and the permissions diff from the other thread [1], or you ok with applying them when you pull it in?
I'll have a crack now, but if it fails miserably then I'll let you know.
Thanks! Just to make sure, you should pick the patch up from
https://lore.kernel.org/linux-arm-kernel/20240903152937.GA3768522@e124191.ca...
Not the one I linked to in [1] in my previous e-mail.
Right, there's quite a lot I need to do:
- Uncorrupt your patches - Fix the conflict in the kvm selftests - Drop the unnecessary ISBs - Fix the ESR checking - Fix the el2_setup labels - Reorder the patches - Drop the patch that is already in kvmarm
Working on it...
Will