Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Nov 06, 2015 at 09:34:55PM +0800, Wangnan (F) escreveu:
> On 2015/11/6 21:19, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Nov 06, 2015 at 09:46:12AM +0000, Wang Nan escreveu:
> >>In dso__split_kallsyms_for_kcore(), current code adjusts symbol's
> >>address but only reinsert it into rbtree if the symbol belongs to
> >>another map. However, the expression for adjusting symbol (pos->start -=
> >>curr_map->start - curr_map->pgoff) can change the relative order between
> >>two symbols (even if the affected symbols are in different maps, in
> >>kcore case they are possible to share one same dso), which damages the
> >>rbtree.
> >Right, some code does change the symbol values it gets from whatever
> >symtab (kallsyms, ELF, JIT maps, etc) when it should instead use the per
> >map data structure (struct map) and its ->{map,unmap}_ip, ->pgoff,
> >->reloc, members for that :-\
> >
> >I.e. 'struct dso' should be just what comes from the symtab, while
> >'struct map' should be about where that DSO is in memory.
> >
> >With that in mind, do you still think your fix is the correct one?
> 
> Not very sure. I'm not familar with this part of code. Actually
> speaking I don't understand the relationship between what you said
> and what I found...

What I said is that no code should, how did you state it? Here it is:

"In dso__split_kallsyms_for_kcore(), current code adjusts symbol's
address"

It should not, not before adding it to the rbtree, and specially not
_after, any adjustments should be done to 'struct map'.
 
> I spent a whole day to answer Masami's question that why
> kernel_get_symbol_address_by_name success but __find_kernel_function()
> fail in my platform, and described it in commit message.

Well, and that was a good exercise, I think, even one I wouldn't have
done, being as busy as you.

Your fix was perfectly fine, there was no strict need to figure out when
that would result in problem, at that point, if sym was NULL it should
return -ENOENT and since 'ret' was being overwritten...

> This patch is the best one I can find.

And I thank you for that, the investigation + the patch uncovered a bug.
We now need to find a fix, but not necessarily you need to do that tho.

> It solves my problem but may be incorrect. Just want you and other
> know my result. Please let me know if you and other want further
> information. Now its pirority is low because patch 98d3b25 and
> Masami's update are already enough for me.

Sure, lets move forward.

> I'll go back to BPF stuff. There are still much work to do :-)

Indeed, thank you for doing all this work!

- Arnaldo