* [PATCH] powerpc/dexcr: Move HASHCHK trap handler
@ 2023-08-25 1:49 Benjamin Gray
2023-09-14 13:55 ` Michael Ellerman
2023-09-21 9:24 ` Michael Ellerman
0 siblings, 2 replies; 3+ messages in thread
From: Benjamin Gray @ 2023-08-25 1:49 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Benjamin Gray, ruscur
To determine if a trap was caused by a HASHCHK instruction, we inspect
the user instruction that triggered the trap. However this may sleep
if the page needs to be faulted in.
Move the HASHCHK handler logic to after we allow IRQs, which is fine
because we are only interested in HASHCHK if it's a user space trap.
Fixes: 5bcba4e6c13f ("powerpc/dexcr: Handle hashchk exception")
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
arch/powerpc/kernel/traps.c | 56 ++++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f5ce282dc4b8..32b5e841ea97 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1512,23 +1512,11 @@ static void do_program_check(struct pt_regs *regs)
return;
}
- if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {
- ppc_inst_t insn;
-
- if (get_user_instr(insn, (void __user *)regs->nip)) {
- _exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
- return;
- }
-
- if (ppc_inst_primary_opcode(insn) == 31 &&
- get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
- _exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
- return;
- }
+ /* User mode considers other cases after enabling IRQs */
+ if (!user_mode(regs)) {
+ _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
+ return;
}
-
- _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
- return;
}
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
if (reason & REASON_TM) {
@@ -1561,16 +1549,44 @@ static void do_program_check(struct pt_regs *regs)
/*
* If we took the program check in the kernel skip down to sending a
- * SIGILL. The subsequent cases all relate to emulating instructions
- * which we should only do for userspace. We also do not want to enable
- * interrupts for kernel faults because that might lead to further
- * faults, and loose the context of the original exception.
+ * SIGILL. The subsequent cases all relate to user space, such as
+ * emulating instructions which we should only do for user space. We
+ * also do not want to enable interrupts for kernel faults because that
+ * might lead to further faults, and loose the context of the original
+ * exception.
*/
if (!user_mode(regs))
goto sigill;
interrupt_cond_local_irq_enable(regs);
+ /*
+ * (reason & REASON_TRAP) is mostly handled before enabling IRQs,
+ * except get_user_instr() can sleep so we cannot reliably inspect the
+ * current instruction in that context. Now that we know we are
+ * handling a user space trap and can sleep, we can check if the trap
+ * was a hashchk failure.
+ */
+ if (reason & REASON_TRAP) {
+ if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE)) {
+ ppc_inst_t insn;
+
+ if (get_user_instr(insn, (void __user *)regs->nip)) {
+ _exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
+ return;
+ }
+
+ if (ppc_inst_primary_opcode(insn) == 31 &&
+ get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
+ _exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
+ return;
+ }
+ }
+
+ _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
+ return;
+ }
+
/* (reason & REASON_ILLEGAL) would be the obvious thing here,
* but there seems to be a hardware bug on the 405GP (RevD)
* that means ESR is sometimes set incorrectly - either to
--
2.41.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/dexcr: Move HASHCHK trap handler
2023-08-25 1:49 [PATCH] powerpc/dexcr: Move HASHCHK trap handler Benjamin Gray
@ 2023-09-14 13:55 ` Michael Ellerman
2023-09-21 9:24 ` Michael Ellerman
1 sibling, 0 replies; 3+ messages in thread
From: Michael Ellerman @ 2023-09-14 13:55 UTC (permalink / raw)
To: Benjamin Gray, linuxppc-dev; +Cc: Benjamin Gray, ruscur
Benjamin Gray <bgray@linux.ibm.com> writes:
> To determine if a trap was caused by a HASHCHK instruction, we inspect
> the user instruction that triggered the trap. However this may sleep
> if the page needs to be faulted in.
It would be good to include the output from the might_sleep() check that
failed.
And I think this was found by syzkaller? If so we should say so.
cheers
> Move the HASHCHK handler logic to after we allow IRQs, which is fine
> because we are only interested in HASHCHK if it's a user space trap.
>
> Fixes: 5bcba4e6c13f ("powerpc/dexcr: Handle hashchk exception")
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
> arch/powerpc/kernel/traps.c | 56 ++++++++++++++++++++++++-------------
> 1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index f5ce282dc4b8..32b5e841ea97 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1512,23 +1512,11 @@ static void do_program_check(struct pt_regs *regs)
> return;
> }
>
> - if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {
> - ppc_inst_t insn;
> -
> - if (get_user_instr(insn, (void __user *)regs->nip)) {
> - _exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
> - return;
> - }
> -
> - if (ppc_inst_primary_opcode(insn) == 31 &&
> - get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
> - _exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
> - return;
> - }
> + /* User mode considers other cases after enabling IRQs */
> + if (!user_mode(regs)) {
> + _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> + return;
> }
> -
> - _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> - return;
> }
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> if (reason & REASON_TM) {
> @@ -1561,16 +1549,44 @@ static void do_program_check(struct pt_regs *regs)
>
> /*
> * If we took the program check in the kernel skip down to sending a
> - * SIGILL. The subsequent cases all relate to emulating instructions
> - * which we should only do for userspace. We also do not want to enable
> - * interrupts for kernel faults because that might lead to further
> - * faults, and loose the context of the original exception.
> + * SIGILL. The subsequent cases all relate to user space, such as
> + * emulating instructions which we should only do for user space. We
> + * also do not want to enable interrupts for kernel faults because that
> + * might lead to further faults, and loose the context of the original
> + * exception.
> */
> if (!user_mode(regs))
> goto sigill;
>
> interrupt_cond_local_irq_enable(regs);
>
> + /*
> + * (reason & REASON_TRAP) is mostly handled before enabling IRQs,
> + * except get_user_instr() can sleep so we cannot reliably inspect the
> + * current instruction in that context. Now that we know we are
> + * handling a user space trap and can sleep, we can check if the trap
> + * was a hashchk failure.
> + */
> + if (reason & REASON_TRAP) {
> + if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE)) {
> + ppc_inst_t insn;
> +
> + if (get_user_instr(insn, (void __user *)regs->nip)) {
> + _exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
> + return;
> + }
> +
> + if (ppc_inst_primary_opcode(insn) == 31 &&
> + get_xop(ppc_inst_val(insn)) == OP_31_XOP_HASHCHK) {
> + _exception(SIGILL, regs, ILL_ILLOPN, regs->nip);
> + return;
> + }
> + }
> +
> + _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
> + return;
> + }
> +
> /* (reason & REASON_ILLEGAL) would be the obvious thing here,
> * but there seems to be a hardware bug on the 405GP (RevD)
> * that means ESR is sometimes set incorrectly - either to
> --
> 2.41.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/dexcr: Move HASHCHK trap handler
2023-08-25 1:49 [PATCH] powerpc/dexcr: Move HASHCHK trap handler Benjamin Gray
2023-09-14 13:55 ` Michael Ellerman
@ 2023-09-21 9:24 ` Michael Ellerman
1 sibling, 0 replies; 3+ messages in thread
From: Michael Ellerman @ 2023-09-21 9:24 UTC (permalink / raw)
To: linuxppc-dev, Benjamin Gray; +Cc: ruscur
On Fri, 25 Aug 2023 11:49:10 +1000, Benjamin Gray wrote:
> To determine if a trap was caused by a HASHCHK instruction, we inspect
> the user instruction that triggered the trap. However this may sleep
> if the page needs to be faulted in.
>
> Move the HASHCHK handler logic to after we allow IRQs, which is fine
> because we are only interested in HASHCHK if it's a user space trap.
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/dexcr: Move HASHCHK trap handler
https://git.kernel.org/powerpc/c/c3f4309693758b13fbb34b3741c2e2801ad28769
cheers
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-21 9:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-25 1:49 [PATCH] powerpc/dexcr: Move HASHCHK trap handler Benjamin Gray
2023-09-14 13:55 ` Michael Ellerman
2023-09-21 9:24 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).