Messages in this thread | | | Subject | Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore | From | pi3orama <> | Date | Fri, 6 Nov 2015 22:33:40 +0800 |
| |
发自我的 iPhone
> 在 2015年11月6日,下午10:03,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.
And also thanks to our great testing team. They found this bug and push me to solve it.
Thank you.
> >> 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
| |