lkml.org 
[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] powerpc/perf: Dereference bhrb entries safely
On Tue, Dec 19, 2017 at 6:23 PM, Ravi Bangoria
<ravi.bangoria@linux.vnet.ibm.com> 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
>> <ravi.bangoria@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.rao@linux.vnet.ibm.com>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@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)) {

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(&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?
>

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

\
 
 \ /
  Last update: 2017-12-19 10:55    [W:0.124 / U:1.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site