Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[Krister Johansen]

Hi Adrian,

On Wed, Jan 25, 2023 at 09:37:59AM +0200, Adrian Hunter wrote:
> On 25/01/23 09:29, Adrian Hunter wrote:
> 
> Also subject line has spelling mistake, and should identify kcore
> as the issue e.g.
> 
> perf symbol: Symbol lookup with kcore can fail if multiple segments match stext
> 
> > On 25/01/23 00:35, Krister Johansen wrote:
> >> This problem was encountered on an arm64 system with a lot of memory.
> >> Without kernel debug symbols installed, and with both kcore and kallsyms
> >> available, perf managed to get confused and returned "unknown" for all
> >> of the kernel symbols that it tried to look up.
> >>
> >> On this system, stext fell within the vmalloc segment.  The kcore symbol
> >> matching code tries to find the first segment that contains stext and
> >> uses that to replace the segment generated from just the kallsyms
> >> information.  In this case, however, there were two: a very large
> >> vmalloc segment, and the text segment.  This caused perf to get confused
> >> because multiple overlapping segments were inserted into the RB tree
> >> that holds the discovered segments.  However, that alone wasn't
> >> sufficient to cause the problem. Even when we could find the segment,
> >> the offsets were adjusted in such a way that the newly generated symbols
> >> didn't line up with the instruction addresses in the trace.  The most
> >> obvious solution would be to consult which segment type is text from
> >> kcore, but this information is not exposed to users.
> >>
> >> Instead, select the smallest matching segment that contains stext
> >> instead of the first matching segment.  This allows us to match the text
> >> segment instead of vmalloc, if one is contained within the other.
> >>
> >> Signed-off-by: Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>  tools/perf/util/symbol.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> >> index a3a165ae933a..14ac4189eaff 100644
> >> --- a/tools/perf/util/symbol.c
> >> +++ b/tools/perf/util/symbol.c
> >> @@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >>  
> >>  	/* Find the kernel map using the '_stext' symbol */
> >>  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> >> +		u64 replacement_size = 0;
> > 
> > We'd usually put a blank line here
> > 
> >>  		list_for_each_entry(new_map, &md.maps, node) {
> >> -			if (stext >= new_map->start && stext < new_map->end) {
> >> +			u64 new_size = new_map->end - new_map->start;
> >> +
> >> +			if (!(stext >= new_map->start && stext < new_map->end))
> >> +				continue;
> >> +
> > 
> > Really needs a comment, and please be specific e.g.
> > 
> >  ARM64 vmalloc segment overlaps the kernel text segment, so
> >  choosing the smaller segment will get the kernel text.
> > 
> > 
> > 
> >> +			if (!replacement_map || new_size < replacement_size) {
> >>  				replacement_map = new_map;
> >> -				break;
> >> +				replacement_size = new_size;
> >>  			}
> >>  		}
> >>  	}

Thanks for all the feedback.  I'll incorporate these changes and send
out a v2 shortly.

-K