Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754028AbbHDQN7 (ORCPT ); Tue, 4 Aug 2015 12:13:59 -0400 Received: from mail-wi0-f170.google.com ([209.85.212.170]:35401 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751920AbbHDQN6 (ORCPT ); Tue, 4 Aug 2015 12:13:58 -0400 Date: Tue, 4 Aug 2015 18:13:55 +0200 From: Frederic Weisbecker To: Andy Lutomirski Cc: Peter Zijlstra , linux-kernel@vger.kernel.org, Brian Gerst , Steven Rostedt , Borislav Petkov , Thomas Gleixner , Linus Torvalds , X86 ML , Masami Hiramatsu , Oleg Nesterov Subject: Re: [PATCH 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints Message-ID: <20150804161354.GC32738@lerouge> References: <136be387950e78f18cea60e9d1bef74465d0ee8f.1438312874.git.luto@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <136be387950e78f18cea60e9d1bef74465d0ee8f.1438312874.git.luto@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2385 Lines: 60 On Thu, Jul 30, 2015 at 08:32:42PM -0700, Andy Lutomirski wrote: > The check looked wrong, although I think it was actually safe. TASK_SIZE > is unnecessarily small for compat tasks, and it wasn't possible to make > a range breakpoint so large it started in user space and ended in kernel > space. > > Nonetheless, let's fix up the check for the benefit of future > readers. A breakpoint is in the kernel if either end is in the > kernel. > > Signed-off-by: Andy Lutomirski Indeed, in fact Oleg posted the same patch a long while ago but I eventually forgot to track them. See https://lkml.org/lkml/2013/11/24/44 Arm, arm64 and sh show the same issue and powerpc uses some obscure is_kernel_addr(). Eventually we should move this function to kernel/event/hw_breakpoint.c with a weak tag to let archs override it like powerpc does. Although it seems not to care about the breakpoint length so perhaps it's not correct. In fact we should have some sort of generic in_kernel_va_range(start, length). This reminds me something I'm sure it already exists :-) > --- > arch/x86/kernel/hw_breakpoint.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c > index 6f345d302cf6..50a3fad5b89f 100644 > --- a/arch/x86/kernel/hw_breakpoint.c > +++ b/arch/x86/kernel/hw_breakpoint.c > @@ -180,7 +180,11 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp) > va = info->address; > len = bp->attr.bp_len; > > - return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE); > + /* > + * We don't need to worry about va + len - 1 overflowing: > + * we already require that va is aligned to a multiple of len. > + */ > + return (va >= TASK_SIZE_MAX) || ((va + len - 1) >= TASK_SIZE_MAX); > } > > int arch_bp_generic_fields(int x86_len, int x86_type, > -- > 2.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/