Re: [PATCH] powerpc/dexcr: Move HASHCHK trap handler

[Michael Ellerman <mpe@ellerman.id.au>]

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