| Message ID | 20201124155039.13804-5-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | An alternative series for asymmetric AArch32 systems | expand |
On 11/24/20 15:50, Will Deacon wrote: > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea. > > Ensure that 32-bit applications always take the slow-path when returning > to userspace on a system with mismatched support at EL0, so that we can > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead. > > Signed-off-by: Will Deacon <will@kernel.org> > --- nit: We drop this patch at the end. Can't we avoid it altogether instead? [...] > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index a8184cad8890..bcb6ca2d9a7c 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs) > restore_saved_sigmask(); > } > > +static bool cpu_affinity_invalid(struct pt_regs *regs) > +{ > + if (!compat_user_mode(regs)) > + return false; Silly question. Is there an advantage of using compat_user_mode() vs is_compat_task()? I see the latter used in the file although struct pt_regs *regs is passed to the functions calling it. Nothing's wrong with it, just curious. Thanks -- Qais Yousef > + > + /* > + * We're preemptible, but a reschedule will cause us to check the > + * affinity again. > + */ > + return !cpumask_test_cpu(raw_smp_processor_id(), > + system_32bit_el0_cpumask()); > +}
On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote: > On 11/24/20 15:50, Will Deacon wrote: > > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea. > > > > Ensure that 32-bit applications always take the slow-path when returning > > to userspace on a system with mismatched support at EL0, so that we can > > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead. > > > > Signed-off-by: Will Deacon <will@kernel.org> > > --- > > nit: We drop this patch at the end. Can't we avoid it altogether instead? I did it like this so that the last patch can be reverted for testing/debugging, but also because I think it helps the structure of the series. > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > > index a8184cad8890..bcb6ca2d9a7c 100644 > > --- a/arch/arm64/kernel/signal.c > > +++ b/arch/arm64/kernel/signal.c > > @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs) > > restore_saved_sigmask(); > > } > > > > +static bool cpu_affinity_invalid(struct pt_regs *regs) > > +{ > > + if (!compat_user_mode(regs)) > > + return false; > > Silly question. Is there an advantage of using compat_user_mode() vs > is_compat_task()? I see the latter used in the file although struct pt_regs > *regs is passed to the functions calling it. > > Nothing's wrong with it, just curious. Not sure about advantages, but is_compat_task() is available in core code, whereas compat_user_mode() is specific to arm64. The former implicitly operates on 'current' and just checks thread flag, whereas the latter actually goes and looks at mode field of the spsr to see what we're going to be returning into. Will
On 12/01/20 16:56, Will Deacon wrote: > On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote: > > On 11/24/20 15:50, Will Deacon wrote: > > > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea. > > > > > > Ensure that 32-bit applications always take the slow-path when returning > > > to userspace on a system with mismatched support at EL0, so that we can > > > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead. > > > > > > Signed-off-by: Will Deacon <will@kernel.org> > > > --- > > > > nit: We drop this patch at the end. Can't we avoid it altogether instead? > > I did it like this so that the last patch can be reverted for > testing/debugging, but also because I think it helps the structure of the > series. Cool. I had a comment about the barrier(), you were worried about cpu_affinity_invalid() being inlined by the compiler and then things get mangled such that TIF_NOTIFY_RESUME clearing is moved after the call as you described? Can the compiler move things if cpu_affinity_invalid() is a proper function call (not inlined)? > > > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > > > index a8184cad8890..bcb6ca2d9a7c 100644 > > > --- a/arch/arm64/kernel/signal.c > > > +++ b/arch/arm64/kernel/signal.c > > > @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs) > > > restore_saved_sigmask(); > > > } > > > > > > +static bool cpu_affinity_invalid(struct pt_regs *regs) > > > +{ > > > + if (!compat_user_mode(regs)) > > > + return false; > > > > Silly question. Is there an advantage of using compat_user_mode() vs > > is_compat_task()? I see the latter used in the file although struct pt_regs > > *regs is passed to the functions calling it. > > > > Nothing's wrong with it, just curious. > > Not sure about advantages, but is_compat_task() is available in core code, > whereas compat_user_mode() is specific to arm64. The former implicitly > operates on 'current' and just checks thread flag, whereas the latter > actually goes and looks at mode field of the spsr to see what we're > going to be returning into. Okay, so just 2 different ways to do the same thing and you happened to pick the one that first came to mind. Thanks -- Qais Yousef
On Wed, Dec 02, 2020 at 01:52:16PM +0000, Qais Yousef wrote: > On 12/01/20 16:56, Will Deacon wrote: > > On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote: > > > On 11/24/20 15:50, Will Deacon wrote: > > > > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea. > > > > > > > > Ensure that 32-bit applications always take the slow-path when returning > > > > to userspace on a system with mismatched support at EL0, so that we can > > > > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead. > > > > > > > > Signed-off-by: Will Deacon <will@kernel.org> > > > > --- > > > > > > nit: We drop this patch at the end. Can't we avoid it altogether instead? > > > > I did it like this so that the last patch can be reverted for > > testing/debugging, but also because I think it helps the structure of the > > series. > > Cool. I had a comment about the barrier(), you were worried about > cpu_affinity_invalid() being inlined by the compiler and then things get > mangled such that TIF_NOTIFY_RESUME clearing is moved after the call as you > described? Can the compiler move things if cpu_affinity_invalid() is a proper > function call (not inlined)? I think function calls implicitly clobber memory, but you'd have to annotate the thing as noinline to prevent it being inlined. Will
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 4784011cecac..1540ab0fbf23 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -542,6 +542,15 @@ static void erratum_1418040_thread_switch(struct task_struct *prev, write_sysreg(val, cntkctl_el1); } +static void compat_thread_switch(struct task_struct *next) +{ + if (!is_compat_thread(task_thread_info(next))) + return; + + if (static_branch_unlikely(&arm64_mismatched_32bit_el0)) + set_tsk_thread_flag(next, TIF_NOTIFY_RESUME); +} + /* * Thread switching. */ @@ -558,6 +567,7 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, uao_thread_switch(next); ssbs_thread_switch(next); erratum_1418040_thread_switch(prev, next); + compat_thread_switch(next); /* * Complete any pending TLB or cache maintenance on this CPU in case @@ -620,8 +630,15 @@ unsigned long arch_align_stack(unsigned long sp) */ void arch_setup_new_exec(void) { - current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0; + unsigned long mmflags = 0; + + if (is_compat_task()) { + mmflags = MMCF_AARCH32; + if (static_branch_unlikely(&arm64_mismatched_32bit_el0)) + set_tsk_thread_flag(current, TIF_NOTIFY_RESUME); + } + current->mm->context.flags = mmflags; ptrauth_thread_init_user(current); if (task_spec_ssb_noexec(current)) { diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index a8184cad8890..bcb6ca2d9a7c 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs) restore_saved_sigmask(); } +static bool cpu_affinity_invalid(struct pt_regs *regs) +{ + if (!compat_user_mode(regs)) + return false; + + /* + * We're preemptible, but a reschedule will cause us to check the + * affinity again. + */ + return !cpumask_test_cpu(raw_smp_processor_id(), + system_32bit_el0_cpumask()); +} + asmlinkage void do_notify_resume(struct pt_regs *regs, unsigned long thread_flags) { @@ -948,6 +961,19 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, if (thread_flags & _TIF_NOTIFY_RESUME) { tracehook_notify_resume(regs); rseq_handle_notify_resume(NULL, regs); + + /* + * If we reschedule after checking the affinity + * then we must ensure that TIF_NOTIFY_RESUME + * is set so that we check the affinity again. + * Since tracehook_notify_resume() clears the + * flag, ensure that the compiler doesn't move + * it after the affinity check. + */ + barrier(); + + if (cpu_affinity_invalid(regs)) + force_sig(SIGKILL); } if (thread_flags & _TIF_FOREIGN_FPSTATE)
Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea. Ensure that 32-bit applications always take the slow-path when returning to userspace on a system with mismatched support at EL0, so that we can avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead. Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/process.c | 19 ++++++++++++++++++- arch/arm64/kernel/signal.c | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)