Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752161AbdLNMYO (ORCPT ); Thu, 14 Dec 2017 07:24:14 -0500 Received: from mail-vk0-f66.google.com ([209.85.213.66]:42224 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751719AbdLNMYM (ORCPT ); Thu, 14 Dec 2017 07:24:12 -0500 X-Google-Smtp-Source: ACJfBou+OKc6XyBwrl829ZZOWstCy9zBa4d9yFaN9n9yW0+d2eYO60i7vqQbWGelzpzxniH1DElfi93+ddMRhSJSbMk= MIME-Version: 1.0 In-Reply-To: <20171212122915.20338-1-ravi.bangoria@linux.vnet.ibm.com> References: <20171212122915.20338-1-ravi.bangoria@linux.vnet.ibm.com> From: Balbir Singh Date: Thu, 14 Dec 2017 23:24:10 +1100 Message-ID: Subject: Re: [PATCH] powerpc/perf: Dereference bhrb entries safely To: Ravi Bangoria Cc: Michael Ellerman , Madhavan Srinivasan , "linux-kernel@vger.kernel.org" , Kamalesh Babulal , Paul Mackerras , kan.liang@intel.com, Thomas Gleixner , "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" , "Naveen N. Rao" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2154 Lines: 53 On Tue, Dec 12, 2017 at 11:29 PM, Ravi Bangoria wrote: > It may very well happen that branch instructions recorded by > bhrb entries already get unmapped before they get processed by > the kernel. Hence, trying to dereference such memory location > will endup in a crash. Ex, > > Unable to handle kernel paging request for data at address 0xc008000019c41764 > Faulting instruction address: 0xc000000000084a14 > NIP [c000000000084a14] branch_target+0x4/0x70 > LR [c0000000000eb828] record_and_restart+0x568/0x5c0 > Call Trace: > [c0000000000eb3b4] record_and_restart+0xf4/0x5c0 (unreliable) > [c0000000000ec378] perf_event_interrupt+0x298/0x460 > [c000000000027964] performance_monitor_exception+0x54/0x70 > [c000000000009ba4] performance_monitor_common+0x114/0x120 > > Fix this by deferefencing them safely. > > Suggested-by: Naveen N. Rao > Signed-off-by: Ravi Bangoria > --- > arch/powerpc/perf/core-book3s.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 9e3da168d54c..5a68d2effdf9 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -410,8 +410,11 @@ static __u64 power_pmu_bhrb_to(u64 addr) > int ret; > __u64 target; > > - if (is_kernel_addr(addr)) > - return branch_target((unsigned int *)addr); > + if (is_kernel_addr(addr)) { I think __kernel_text_address() is more accurate right? In which case you need to check for is_kernel_addr(addr) and if its not kernel_text_address() then we have an interesting case of a branch from something not text. It would be nice to catch such cases. > + if (probe_kernel_address((void *)addr, instr)) > + return 0; > + return branch_target(&instr); > + } > > /* Userspace: need copy instruction here then translate it */ > pagefault_disable(); Otherwise, Reviewed-by: Balbir Singh