| 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
| ||
|
Date: Mon, 24 Jun 2024 17:08:18 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Changbin Du <changbin.du@...wei.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Nathan Chancellor <nathan@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Bill Wendling <morbo@...gle.com>,
Justin Stitt <justinstitt@...gle.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
llvm@...ts.linux.dev
Subject: Re: [PATCH v3 2/4] perf: disasm: use build_id_path if fallback failed
Hello,
On Tue, Jun 18, 2024 at 09:55:28AM +0800, Changbin Du wrote:
> If we can not fallback for special dso (vmlinx and vdso), use the
> build_id_path found previously.
>
> To make change easy, this change first refactors the code by extracting
> two functions read_buildid_linkname() and fallback_filename().
Can you please split the refactoring from the actual change? It'd be
easier to review and to maintain the code.
Thanks,
Namhyung
>
> Signed-off-by: Changbin Du <changbin.du@...wei.com>
> ---
> tools/perf/util/disasm.c | 121 ++++++++++++++++++++++++---------------
> 1 file changed, 75 insertions(+), 46 deletions(-)
>
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 442e9802a772..3075daa61916 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1092,14 +1092,72 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
> return 0;
> }
>
> -static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
> +static int read_buildid_linkname(char *filename, char *linkname, size_t linkname_size)
> {
> - char linkname[PATH_MAX];
> - char *build_id_filename;
> char *build_id_path = NULL;
> char *pos;
> int len;
>
> + build_id_path = strdup(filename);
> + if (!build_id_path)
> + return ENOMEM;
> +
> + /*
> + * old style build-id cache has name of XX/XXXXXXX.. while
> + * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
> + * extract the build-id part of dirname in the new style only.
> + */
> + pos = strrchr(build_id_path, '/');
> + if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> + dirname(build_id_path);
> +
> + len = readlink(build_id_path, linkname, linkname_size);
> + if (len < 0) {
> + free(build_id_path);
> + return -1;
> + }
> +
> + linkname[len] = '\0';
> + free(build_id_path);
> + return 0;
> +}
> +
> +static int fallback_filename(struct dso *dso, char *filename, size_t filename_size)
> +{
> + char filepath[PATH_MAX];
> +
> + /*
> + * If we don't have build-ids or the build-id file isn't in the
> + * cache, or is just a kallsyms file, well, lets hope that this
> + * DSO is the same as when 'perf record' ran.
> + */
> + if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
> + snprintf(filepath, sizeof(filepath), "%s", dso__long_name(dso));
> + else
> + __symbol__join_symfs(filepath, sizeof(filepath), dso__long_name(dso));
> +
> + mutex_lock(dso__lock(dso));
> + if (access(filepath, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
> + char *new_name = dso__filename_with_chroot(dso, filepath);
> + if (new_name) {
> + strlcpy(filepath, new_name, sizeof(filepath));
> + free(new_name);
> + }
> + }
> + mutex_unlock(dso__lock(dso));
> +
> + if (access(filepath, R_OK) && errno == ENOENT)
> + return ENOENT;
> +
> + snprintf(filename, filename_size, "%s", filepath);
> + return 0;
> +}
> +
> +static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
> +{
> + char linkname[PATH_MAX];
> + char *build_id_filename;
> +
> if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
> !dso__is_kcore(dso))
> return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
> @@ -1111,57 +1169,28 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> } else {
> if (dso__has_build_id(dso))
> return ENOMEM;
> - goto fallback;
> + return fallback_filename(dso, filename, filename_size);
> }
>
> - build_id_path = strdup(filename);
> - if (!build_id_path)
> - return ENOMEM;
> -
> - /*
> - * old style build-id cache has name of XX/XXXXXXX.. while
> - * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
> - * extract the build-id part of dirname in the new style only.
> - */
> - pos = strrchr(build_id_path, '/');
> - if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> - dirname(build_id_path);
> + if (access(filename, R_OK))
> + return fallback_filename(dso, filename, filename_size);
>
> - if (dso__is_kcore(dso))
> + if (dso__is_kcore(dso) || dso__is_vdso(dso))
> goto fallback;
>
> - len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> - if (len < 0)
> - goto fallback;
> + if (!read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) &&
> + (!strstr(linkname, DSO__NAME_KALLSYMS) && !strstr(linkname, DSO__NAME_VDSO))) {
> + /* not kallsysms or vdso, use build_id path found above */
> + goto out;
> + }
>
> - linkname[len] = '\0';
> - if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
> - access(filename, R_OK)) {
> fallback:
> - /*
> - * If we don't have build-ids or the build-id file isn't in the
> - * cache, or is just a kallsyms file, well, lets hope that this
> - * DSO is the same as when 'perf record' ran.
> - */
> - if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
> - snprintf(filename, filename_size, "%s", dso__long_name(dso));
> - else
> - __symbol__join_symfs(filename, filename_size, dso__long_name(dso));
> -
> - mutex_lock(dso__lock(dso));
> - if (access(filename, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
> - char *new_name = dso__filename_with_chroot(dso, filename);
> - if (new_name) {
> - strlcpy(filename, new_name, filename_size);
> - free(new_name);
> - }
> - }
> - mutex_unlock(dso__lock(dso));
> - } else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
> - dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
> + if (fallback_filename(dso, filename, filename_size)) {
> + /* if fallback failed, use build_id path found above */
> +out:
> + if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
> + dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
> }
> -
> - free(build_id_path);
> return 0;
> }
>
> --
> 2.34.1
>
Powered by blists - more mailing lists