linux-kernel - Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if multiple segments 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]

Message-ID: <7b5f872a-f52e-7129-e956-90e97ec7d911@intel.com>
Date:   Mon, 30 Jan 2023 15:29:54 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Krister Johansen <kjlx@...pleofstupid.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        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>
Subject: Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if
 multiple segments match stext

On 25/01/23 20:34, 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>

Reviewed-by: Adrian Hunter <adrian.hunter@...el.com>

> ---
> v2:
>   - Correct whitespace, add comments, and fix-up subject. (Feedback from Adrian
>     Hunter)
> ---
>  tools/perf/util/symbol.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a3a165ae933a..98014f937568 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1368,10 +1368,23 @@ 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;
> +
>  		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;
> +
> +			/*
> +			 * On some architectures, ARM64 for example, the kernel
> +			 * text can get allocated inside of the vmalloc segment.
> +			 * Select the smallest matching segment, in case stext
> +			 * falls within more than one in the list.
> +			 */
> +			if (!replacement_map || new_size < replacement_size) {
>  				replacement_map = new_map;
> -				break;
> +				replacement_size = new_size;
>  			}
>  		}
>  	}