Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-19 Thread Michael Ellerman 
Ravi Bangoria  writes:

> 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
>>  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 
>>> 0xc00819c41764
>>> Faulting instruction address: 0xc0084a14
>>> NIP [c0084a14] branch_target+0x4/0x70
>>> LR [c00eb828] record_and_restart+0x568/0x5c0
>>> Call Trace:
>>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>>> [c00ec378] perf_event_interrupt+0x298/0x460
>>> [c0027964] performance_monitor_exception+0x54/0x70
>>> [c0009ba4] 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.
>
> 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();
> +    }
>  
>  /* 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?

No.

I've already merged the patch, and am fairly happy with it.

cheers

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-19 Thread Michael Ellerman 
Ravi Bangoria  writes:

> 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
>>  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 
>>> 0xc00819c41764
>>> Faulting instruction address: 0xc0084a14
>>> NIP [c0084a14] branch_target+0x4/0x70
>>> LR [c00eb828] record_and_restart+0x568/0x5c0
>>> Call Trace:
>>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>>> [c00ec378] perf_event_interrupt+0x298/0x460
>>> [c0027964] performance_monitor_exception+0x54/0x70
>>> [c0009ba4] 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.
>
> 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();
> +    }
>  
>  /* 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?

No.

I've already merged the patch, and am fairly happy with it.

cheers

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-19 Thread Balbir Singh 
On Tue, Dec 19, 2017 at 6:23 PM, Ravi Bangoria
 wrote:
> Hi Balbir,
>
> Sorry was away for few days.
>

No problem at all

> On 12/14/2017 05:54 PM, Balbir Singh wrote:
>> 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 
>>> 0xc00819c41764
>>> Faulting instruction address: 0xc0084a14
>>> NIP [c0084a14] branch_target+0x4/0x70
>>> LR [c00eb828] record_and_restart+0x568/0x5c0
>>> Call Trace:
>>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>>> [c00ec378] perf_event_interrupt+0x298/0x460
>>> [c0027964] performance_monitor_exception+0x54/0x70
>>> [c0009ba4] 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.
>
> 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)) {

More like if (__kernel_text_address(addr))
 if (probe_kernel_address(...))



> +if (probe_kernel_address((void *)addr, instr)) {
> +WARN_ON(!__kernel_text_address(addr));
> +return 0;
> +}
> +return branch_target();
> +}
>
>  /* 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?
>

I'd rather we not probe addresses that are not text for this case. if
it is unmapped
that is a challenge, but that might happen for unloaded modules and eBPF code
(hopefully that will be rare)

Balbir Singh

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-19 Thread Balbir Singh 
On Tue, Dec 19, 2017 at 6:23 PM, Ravi Bangoria
 wrote:
> Hi Balbir,
>
> Sorry was away for few days.
>

No problem at all

> On 12/14/2017 05:54 PM, Balbir Singh wrote:
>> 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 
>>> 0xc00819c41764
>>> Faulting instruction address: 0xc0084a14
>>> NIP [c0084a14] branch_target+0x4/0x70
>>> LR [c00eb828] record_and_restart+0x568/0x5c0
>>> Call Trace:
>>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>>> [c00ec378] perf_event_interrupt+0x298/0x460
>>> [c0027964] performance_monitor_exception+0x54/0x70
>>> [c0009ba4] 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.
>
> 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)) {

More like if (__kernel_text_address(addr))
 if (probe_kernel_address(...))



> +if (probe_kernel_address((void *)addr, instr)) {
> +WARN_ON(!__kernel_text_address(addr));
> +return 0;
> +}
> +return branch_target();
> +}
>
>  /* 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?
>

I'd rather we not probe addresses that are not text for this case. if
it is unmapped
that is a challenge, but that might happen for unloaded modules and eBPF code
(hopefully that will be rare)

Balbir Singh

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-18 Thread Ravi Bangoria 
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
>  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 
>> 0xc00819c41764
>> Faulting instruction address: 0xc0084a14
>> NIP [c0084a14] branch_target+0x4/0x70
>> LR [c00eb828] record_and_restart+0x568/0x5c0
>> Call Trace:
>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>> [c00ec378] perf_event_interrupt+0x298/0x460
>> [c0027964] performance_monitor_exception+0x54/0x70
>> [c0009ba4] 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.

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();
+    }
 
 /* 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();
>> +   }
>>
>> /* Userspace: need copy instruction here then translate it */
>> pagefault_disable();
> Otherwise,
>
> Reviewed-by: Balbir Singh 
>

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-18 Thread Ravi Bangoria 
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
>  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 
>> 0xc00819c41764
>> Faulting instruction address: 0xc0084a14
>> NIP [c0084a14] branch_target+0x4/0x70
>> LR [c00eb828] record_and_restart+0x568/0x5c0
>> Call Trace:
>> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>> [c00ec378] perf_event_interrupt+0x298/0x460
>> [c0027964] performance_monitor_exception+0x54/0x70
>> [c0009ba4] 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.

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();
+    }
 
 /* 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();
>> +   }
>>
>> /* Userspace: need copy instruction here then translate it */
>> pagefault_disable();
> Otherwise,
>
> Reviewed-by: Balbir Singh 
>

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-14 Thread Balbir Singh 
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 
> 0xc00819c41764
> Faulting instruction address: 0xc0084a14
> NIP [c0084a14] branch_target+0x4/0x70
> LR [c00eb828] record_and_restart+0x568/0x5c0
> Call Trace:
> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
> [c00ec378] perf_event_interrupt+0x298/0x460
> [c0027964] performance_monitor_exception+0x54/0x70
> [c0009ba4] 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();
> +   }
>
> /* Userspace: need copy instruction here then translate it */
> pagefault_disable();

Otherwise,

Reviewed-by: Balbir Singh

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-14 Thread Balbir Singh 
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 
> 0xc00819c41764
> Faulting instruction address: 0xc0084a14
> NIP [c0084a14] branch_target+0x4/0x70
> LR [c00eb828] record_and_restart+0x568/0x5c0
> Call Trace:
> [c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
> [c00ec378] perf_event_interrupt+0x298/0x460
> [c0027964] performance_monitor_exception+0x54/0x70
> [c0009ba4] 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();
> +   }
>
> /* Userspace: need copy instruction here then translate it */
> pagefault_disable();

Otherwise,

Reviewed-by: Balbir Singh

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-12 Thread Naveen N. Rao 

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 
0xc00819c41764
Faulting instruction address: 0xc0084a14
NIP [c0084a14] branch_target+0x4/0x70
LR [c00eb828] record_and_restart+0x568/0x5c0
Call Trace:
[c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
[c00ec378] perf_event_interrupt+0x298/0x460
[c0027964] performance_monitor_exception+0x54/0x70
[c0009ba4] performance_monitor_common+0x114/0x120

Fix this by deferefencing them safely.

Suggested-by: Naveen N. Rao 
Signed-off-by: Ravi Bangoria 


Reviewed-by: Naveen N. Rao 


- Naveen

Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-12 Thread Naveen N. Rao 

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 
0xc00819c41764
Faulting instruction address: 0xc0084a14
NIP [c0084a14] branch_target+0x4/0x70
LR [c00eb828] record_and_restart+0x568/0x5c0
Call Trace:
[c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
[c00ec378] perf_event_interrupt+0x298/0x460
[c0027964] performance_monitor_exception+0x54/0x70
[c0009ba4] performance_monitor_common+0x114/0x120

Fix this by deferefencing them safely.

Suggested-by: Naveen N. Rao 
Signed-off-by: Ravi Bangoria 


Reviewed-by: Naveen N. Rao 


- Naveen

[PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-12 Thread Ravi Bangoria 
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 
0xc00819c41764
Faulting instruction address: 0xc0084a14
NIP [c0084a14] branch_target+0x4/0x70
LR [c00eb828] record_and_restart+0x568/0x5c0
Call Trace:
[c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
[c00ec378] perf_event_interrupt+0x298/0x460
[c0027964] performance_monitor_exception+0x54/0x70
[c0009ba4] 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)) {
+   if (probe_kernel_address((void *)addr, instr))
+   return 0;
+   return branch_target();
+   }
 
/* Userspace: need copy instruction here then translate it */
pagefault_disable();
-- 
2.13.6

[PATCH] powerpc/perf: Dereference bhrb entries safely

2017-12-12 Thread Ravi Bangoria 
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 
0xc00819c41764
Faulting instruction address: 0xc0084a14
NIP [c0084a14] branch_target+0x4/0x70
LR [c00eb828] record_and_restart+0x568/0x5c0
Call Trace:
[c00eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
[c00ec378] perf_event_interrupt+0x298/0x460
[c0027964] performance_monitor_exception+0x54/0x70
[c0009ba4] 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)) {
+   if (probe_kernel_address((void *)addr, instr))
+   return 0;
+   return branch_target();
+   }
 
/* Userspace: need copy instruction here then translate it */
pagefault_disable();
-- 
2.13.6