diff mbox

[2/2] perf tools: Fix find_perf_probe_point_from_map() which incorrectly returns success

Message ID 563C6539.7030605@huawei.com
State New
Headers show

Commit Message

Wang Nan Nov. 6, 2015, 8:30 a.m. UTC 
On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:
> From: acme@kernel.org [mailto:acme@kernel.org]

>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:

>>>> From: Wang Nan [mailto:wangnan0@huawei.com]

>>>>> It is possible that find_perf_probe_point_from_map() fails to find

>>>>> symbol but still returns 0 because of an small error when coding:

>>>>> find_perf_probe_point_from_map() set 'ret' to error code at first,

>>>>> but also use it to hold return value of

>>>>> kernel_get_symbol_address_by_name().

>>>> OK, I didn't expect that there is a symbol which can be found by

>>>> kernel_get_symbol_address_by_name() but not by __find_kernel_function()...

>>>> Would you have any example of the error?

>>>>

>>>>> This patch resets 'ret' to error even kernel_get_symbol_address_by_name()

>>>>> success, so if !sym, the whole function returns error correctly.

>>>> Hmm, that sounds tricky. I'd rather like to add *psym to kernel_get_symbol_address_by_name()

>>>> to save symbol and don't use __find_kernel_function() instead.

>>> Tricky? I don't think so, suboptimal? possibly, but it fixes an error,

>>> so should be processed quickly, right? I'm applying his patch and then

>>> whatever improvement can be done on top.

>> OK, then I'll send an improvement patch.

> Ah, finally I got what happened. I guess the problem may happen when we put

> a probe on the kernel somewhere outside of any functions and run "perf probe -l".

> I think it should not be allowed to put the probe outside any symbol.

>

> The background is here, at first "perf-probe -a somewhere" defines a probe in

> the kernel but its address is relative from "_text". (thus, vfs_read becomes "_text+2348080"

>   for example). Since it is not readable by human, perf probe -l tries to get an appropriate

> symbol from the "_text+OFFSET".

> For the purpose, the first kernel_get_symbol_address_by_name() is for translating _text to

> an address, and the second  __find_kernel_function() is for finding a symbol from the

> address+OFFSET.

> Then, if the address+OFFSET is out of the symbol map, the second one can fail.

> This means the first symbol and the second symbol is not same.

>

> So, the direction of Wang solution is good :). Just a cleanup is required.

>

> Thank you!


I also tried to finger out the problem for all day and made some 
progress. It is another
problem. It happeneds when probing an address reside in a module on 
aarch64 system.

On my aarch64 system I use kcore. Different from x86, on aarch64, 
modules address is lower
than normal kernel. For example:

On x86_64:

# readelf -a /proc/kcore

   Type           Offset             VirtAddr           PhysAddr
                  FileSiz            MemSiz              Flags  Align
   ...
   LOAD           0x00007fff81003000 0xffffffff81000000 
0x0000000000000000   <-- kernel
                  0x0000000001026000 0x0000000001026000  RWE    1000
   LOAD           0x00007fffa0003000 0xffffffffa0000000 
0x0000000000000000   <-- module
                  0x000000005f000000 0x000000005f000000  RWE    1000

On aarch64:

   Type           Offset             VirtAddr           PhysAddr
                  FileSiz            MemSiz              Flags  Align
   ...
   LOAD           0x0000000000002000 0xffffffc000000000 
0x0000000000000000    <-- kernel
                  0x000000007fc00000 0x000000007fc00000  RWE    1000
   LOAD           0xfffffffffc002000 0xffffffbffc000000 
0x0000000000000000    <-- module
                  0x0000000004000000 0x0000000004000000  RWE    1000

See? On aarch64, Offset field of module address area is negative.

Which causes a problem in dso__split_kallsyms_for_kcore(): when it 
adjusting symbols
using "pos->start -= curr_map->start - curr_map->pgoff", the relative 
order between
module functions and normal kernel function is changed.

For example:

funca at 0xffffffc00021b428 is a normal kernel function.
funcb at 0xffffffbffc000000 is a function in kernel.

During parsing /proc/kallsyms, address of funca > address of funcb.

However, after the adjusting:

funca becomes:

0xffffffc00021b428 - (0xffffffc000000000 - 0x2000) = 0x21d428

funcb becomes:

0xffffffbffc000000 - (0xffffffbffc000000 - 0xfffffffffc002000) = 
0xfffffffffc002000

address of funca < address of funcb.

Unfortunately, the rbtree is not adjusted in this case.

I hacked symbols__find:

         return NULL;

and get correct result:

try to find information at 3ffc000000 in kernel_module
Failed to find module kernel_module.
Failed to find the path for kernel_module: [kernel_module]
Failed to find corresponding probes from debuginfo.
found 0xfffffffffc002000

However, what we really need is adjusting rbtree in this case.

Could you please give me some hint for fixing this problem? I'm not 
familiar with
this part of code.

Thank you.


--
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/

Comments

Wang Nan Nov. 6, 2015, 9:27 a.m. UTC | #1 
On 2015/11/6 16:30, Wangnan (F) wrote:
>

>

> On 2015/11/6 15:12, 平松雅巳 / HIRAMATU，MASAMI wrote:

>> From: acme@kernel.org [mailto:acme@kernel.org]

>>>> Em Thu, Nov 05, 2015 at 02:08:48PM +0000, 平松雅巳 / 

>>>> HIRAMATU，MASAMI escreveu:

>>>>> From: Wang Nan [mailto:wangnan0@huawei.com]


[SNIP]
>> Ah, finally I got what happened. I guess the problem may happen when 

>> we put

>> a probe on the kernel somewhere outside of any functions and run 

>> "perf probe -l".

>> I think it should not be allowed to put the probe outside any symbol.

>>

>> The background is here, at first "perf-probe -a somewhere" defines a 

>> probe in

>> the kernel but its address is relative from "_text". (thus, vfs_read 

>> becomes "_text+2348080"

>>   for example). Since it is not readable by human, perf probe -l 

>> tries to get an appropriate

>> symbol from the "_text+OFFSET".

>> For the purpose, the first kernel_get_symbol_address_by_name() is for 

>> translating _text to

>> an address, and the second  __find_kernel_function() is for finding a 

>> symbol from the

>> address+OFFSET.

>> Then, if the address+OFFSET is out of the symbol map, the second one 

>> can fail.

>> This means the first symbol and the second symbol is not same.

>>

>> So, the direction of Wang solution is good :). Just a cleanup is 

>> required.

>>

>> Thank you!

>

> I also tried to finger out the problem for all day and made some 

> progress. It is another

> problem. It happeneds when probing an address reside in a module on 

> aarch64 system.

>

> On my aarch64 system I use kcore. Different from x86, on aarch64, 

> modules address is lower

> than normal kernel. For example:

>

> On x86_64:

>

> # readelf -a /proc/kcore

>

>   Type           Offset             VirtAddr           PhysAddr

>                  FileSiz            MemSiz              Flags Align

>   ...

>   LOAD           0x00007fff81003000 0xffffffff81000000 

> 0x0000000000000000   <-- kernel

>                  0x0000000001026000 0x0000000001026000  RWE 1000

>   LOAD           0x00007fffa0003000 0xffffffffa0000000 

> 0x0000000000000000   <-- module

>                  0x000000005f000000 0x000000005f000000  RWE 1000

>

> On aarch64:

>

>   Type           Offset             VirtAddr           PhysAddr

>                  FileSiz            MemSiz              Flags Align

>   ...

>   LOAD           0x0000000000002000 0xffffffc000000000 

> 0x0000000000000000    <-- kernel

>                  0x000000007fc00000 0x000000007fc00000  RWE 1000

>   LOAD           0xfffffffffc002000 0xffffffbffc000000 

> 0x0000000000000000    <-- module

>                  0x0000000004000000 0x0000000004000000  RWE 1000

>

> See? On aarch64, Offset field of module address area is negative.

>


One thing should be noticed that, even if normal kernel code and modules 
use different
'struct map', they share a same dso. Please see dso__load_kcore, notice 
how it initialize
parameters (md) before calling file__read_maps().

> Which causes a problem in dso__split_kallsyms_for_kcore(): when it 

> adjusting symbols

> using "pos->start -= curr_map->start - curr_map->pgoff", the relative 

> order between

> module functions and normal kernel function is changed.

>

> For example:

>

> funca at 0xffffffc00021b428 is a normal kernel function.

> funcb at 0xffffffbffc000000 is a function in kernel.

>

> During parsing /proc/kallsyms, address of funca > address of funcb.

>

> However, after the adjusting:

>

> funca becomes:

>

> 0xffffffc00021b428 - (0xffffffc000000000 - 0x2000) = 0x21d428

>

> funcb becomes:

>

> 0xffffffbffc000000 - (0xffffffbffc000000 - 0xfffffffffc002000) = 

> 0xfffffffffc002000

>

> address of funca < address of funcb.

>

> Unfortunately, the rbtree is not adjusted in this case.

>


Even if they are in different maps, they share a same dso here, so a 
same rbtree.

Thank you.

--
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/
diff mbox

Patch

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index b4cc766..8463b0c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -332,12 +332,14 @@  static struct symbol *symbols__find(struct rb_root 
*symbols, u64 ip)
         while (n) {
                 struct symbol *s = rb_entry(n, struct symbol, rb_node);

-               if (ip < s->start)
+               if ((s64)ip < (s64)s->start)
                         n = n->rb_left;
-               else if (ip >= s->end)
+               else if ((s64)ip >= (s64)s->end)
                         n = n->rb_right;
-               else
+               else {
+                       pr_debug("found %p\n", (void *)ip);
                         return s;
+               }
         }