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

[Adrian Hunter]

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;
>>  			}
>>  		}
>>  	}
>