Re: [PATCH v6 5/8] powerpc/watchpoint: Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N

From: Rogerio Alves <rcardoso@linux.ibm.com>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
	mpe@ellerman.id.au, christophe.leroy@c-s.fr
Cc: mikey@neuling.org, jniethe5@gmail.com, pedromfc@linux.ibm.com,
	linux-kernel@vger.kernel.org, paulus@samba.org,
	rogealve@linux.ibm.com, naveen.n.rao@linux.vnet.ibm.com,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v6 5/8] powerpc/watchpoint: Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N
Date: Thu, 17 Sep 2020 10:25:51 -0300	[thread overview]
Message-ID: <7632bf52-3a0c-66d7-a236-7f896b1ce13e@linux.ibm.com> (raw)
In-Reply-To: <20200902042945.129369-6-ravi.bangoria@linux.ibm.com>


On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel
> disables event every time it fires and user has to re-enable it.
> Also, in case of ptrace watchpoint, kernel notifies ptrace user
> before executing instruction.
> 
> With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable
> ptrace event and thus it's causing infinite loop of exceptions.
> This is especially harmful when user watches on a data which is
> also read/written by kernel, eg syscall parameters. In such case,
> infinite exceptions happens in kernel mode which causes soft-lockup.
> 
> Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers")
> Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h  |  3 ++
>   arch/powerpc/kernel/process.c             | 48 +++++++++++++++++++++++
>   arch/powerpc/kernel/ptrace/ptrace-noadv.c |  4 +-
>   3 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 81872c420476..abebfbee5b1c 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -18,6 +18,7 @@ struct arch_hw_breakpoint {
>   	u16		type;
>   	u16		len; /* length of the target data symbol */
>   	u16		hw_len; /* length programmed in hw */
> +	u8		flags;
>   };
>   
>   /* Note: Don't change the first 6 bits below as they are in the same order
> @@ -37,6 +38,8 @@ struct arch_hw_breakpoint {
>   #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
>   				 HW_BRK_TYPE_HYP)
>   
> +#define HW_BRK_FLAG_DISABLED	0x1
> +
>   /* Minimum granularity */
>   #ifdef CONFIG_PPC_8xx
>   #define HW_BREAKPOINT_SIZE  0x4
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 016bd831908e..160fbbf41d40 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address,
>   				    (void __user *)address);
>   }
>   #else	/* !CONFIG_PPC_ADV_DEBUG_REGS */
> +
> +static void do_break_handler(struct pt_regs *regs)
> +{
> +	struct arch_hw_breakpoint null_brk = {0};
> +	struct arch_hw_breakpoint *info;
> +	struct ppc_inst instr = ppc_inst(0);
> +	int type = 0;
> +	int size = 0;
> +	unsigned long ea;
> +	int i;
> +
> +	/*
> +	 * If underneath hw supports only one watchpoint, we know it
> +	 * caused exception. 8xx also falls into this category.
> +	 */
> +	if (nr_wp_slots() == 1) {
> +		__set_breakpoint(0, &null_brk);
> +		current->thread.hw_brk[0] = null_brk;
> +		current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED;
> +		return;
> +	}
> +
> +	/* Otherwise findout which DAWR caused exception and disable it. */
> +	wp_get_instr_detail(regs, &instr, &type, &size, &ea);
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		info = &current->thread.hw_brk[i];
> +		if (!info->address)
> +			continue;
> +
> +		if (wp_check_constraints(regs, instr, ea, type, size, info)) {
> +			__set_breakpoint(i, &null_brk);
> +			current->thread.hw_brk[i] = null_brk;
> +			current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED;
> +		}
> +	}
> +}
> +
>   void do_break (struct pt_regs *regs, unsigned long address,
>   		    unsigned long error_code)
>   {
> @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address,
>   	if (debugger_break_match(regs))
>   		return;
>   
> +	/*
> +	 * We reach here only when watchpoint exception is generated by ptrace
> +	 * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set,
> +	 * watchpoint is already handled by hw_breakpoint_handler() so we don't
> +	 * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set,
> +	 * we need to manually handle the watchpoint here.
> +	 */
> +	if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
> +		do_break_handler(regs);
> +
>   	/* Deliver the signal to userspace */
>   	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
>   }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 57a0ab822334..c9122ed91340 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
>   	}
>   	return ret;
>   #else /* CONFIG_HAVE_HW_BREAKPOINT */
> -	if (child->thread.hw_brk[data - 1].address == 0)
> +	if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) &&
> +	    child->thread.hw_brk[data - 1].address == 0)
>   		return -ENOENT;
>   
>   	child->thread.hw_brk[data - 1].address = 0;
>   	child->thread.hw_brk[data - 1].type = 0;
> +	child->thread.hw_brk[data - 1].flags = 0;
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>   
>   	return 0;
>