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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Jan 2023 09:37:59 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Krister Johansen <kjlx@...pleofstupid.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Michael Petlan <mpetlan@...hat.com>,
        David Reaver <me@...idreaver.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets
 match stext

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@...pleofstupid.com>
>> ---
>>  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;
>>  			}
>>  		}
>>  	}
>