Hi Balbir,
Sorry was away for few days.
On 12/14/2017 05:54 PM, Balbir Singh wrote:
> On Tue, Dec 12, 2017 at 11:29 PM, Ravi Bangoria
> <ravi.bango...@linux.vnet.ibm.com> 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 <naveen.n....@linux.vnet.ibm.com>
>> Signed-off-by: Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com>
>> ---
>> 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.
Something like this?
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 1538129..627af56 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -410,8 +410,13 @@ 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)) {
+ if (probe_kernel_address((void *)addr, instr)) {
+ WARN_ON(!__kernel_text_address(addr));
+ return 0;
+ }
+ return branch_target(&instr);
+ }
/* Userspace: need copy instruction here then translate it */
pagefault_disable();
I think this will throw warnings when you try to read recently unmapped
vmalloced address. Is that fine?
Thanks for the review.
Ravi
>> + 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 <bsinghar...@gmail.com>
>
