Skip to contentSkip to conversation

perf tools: Fix for compressed kernel modules (v4)

PATCH In review Namhyung Kim 4 participants 21 messages Jun 8, 2017 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

cover perf tools: Fix for compressed kernel modules (v4)

Hello,

This is v4 of my compressed kernel module work. Please take a look.

* changes in v4)
- fix build-id cache symbolic link handling in annotate
- separate fix for a memory leak (Arnaldo)
- consolidate error path in __open_dso (Arnaldo)
- remove decompressed file after use

The code is avaiable at 'perf/kmod-decomp-v4' branch in my tree:

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (9):
perf annotate: Fix symbolic link of build-id cache perf tools: Fix a memory leak in __open_dso() perf tools: Introduce dso__decompress_kmodule_{fd,path} perf annotate: Use dso__decompress_kmodule_path() perf tools: Decompress kernel module when reading DSO data perf tools: Consolidate error path in __open_dso() perf test: Decompress kernel module before objdump perf symbols: Keep DSO->symtab_type after decompress perf symbols: Kill dso__build_id_is_kmod()

 tools/perf/tests/code-reading.c | 20 +++++++++-
 tools/perf/util/annotate.c      | 37 ++++++------------
 tools/perf/util/build-id.c      | 45 ----------------------
 tools/perf/util/build-id.h      |  1 -
 tools/perf/util/dso.c           | 85 ++++++++++++++++++++++++++++++++++++++---
 tools/perf/util/dso.h           |  6 +++
 tools/perf/util/symbol-elf.c    | 38 ++----------------
 tools/perf/util/symbol.c        |  4 --
 8 files changed, 119 insertions(+), 117 deletions(-)
Signature · 1 line
2.13.1
author maintainer
#1 lore raw

PATCH v4 1/9 perf annotate: Fix symbolic link of build-id cache

The commit 6ebd2547dd24 ("perf annotate: Fix a bug following symbolic link of a build-id file") changed to use dirname to follow the symlink. But it only considers new-style build-id cache names so old names fail on readlink() and force to use system path which might not available.

---
 tools/perf/util/annotate.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
tools/perf/util/annotate.c +9 -1
index 1367d7e35242..df4486c3a2fa 100644
@@ -1321,6 +1321,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	char linkname[PATH_MAX];
 	char *build_id_filename;
 	char *build_id_path = NULL;
+	char *pos;
 
 	if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
 	    !dso__is_kcore(dso))
@@ -1340,7 +1341,14 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	if (!build_id_path)
 		return -1;
 
-	dirname(build_id_path);
+	/*
+	 * 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 (dso__is_kcore(dso) ||
 	    readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
Signature · 1 line
2.13.1
#2 lore raw

[tip:perf/urgent] perf annotate: Fix symbolic link of build-id cache

perf annotate: Fix symbolic link of build-id cache
Commit-ID:  3619ef76b37d4803bc9daee9d03d82c8526db378
Gitweb:     http://git.kernel.org/tip/3619ef76b37d4803bc9daee9d03d82c8526db378
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:01 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:38:41 -0300

perf annotate: Fix symbolic link of build-id cache

The commit 6ebd2547dd24 ("perf annotate: Fix a bug following symbolic link of a build-id file") changed to use dirname to follow the symlink. But it only considers new-style build-id cache names so old names fail on readlink() and force to use system path which might not available.

Cc Adrian Hunter <[email protected]>
Cc David Ahern <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Taeung Song <[email protected]>
Cc Wang Nan <[email protected]>
Fixes 6ebd2547dd24 ("perf annotate: Fix a bug following symbolic link of a build-id file")
---
 tools/perf/util/annotate.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
tools/perf/util/annotate.c +9 -1
index 1367d7e..df4486c 100644
@@ -1321,6 +1321,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	char linkname[PATH_MAX];
 	char *build_id_filename;
 	char *build_id_path = NULL;
+	char *pos;
 
 	if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
 	    !dso__is_kcore(dso))
@@ -1340,7 +1341,14 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	if (!build_id_path)
 		return -1;
 
-	dirname(build_id_path);
+	/*
+	 * 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 (dso__is_kcore(dso) ||
 	    readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
author maintainer
#3 lore raw

PATCH v4 2/9 perf tools: Fix a memory leak in __open_dso()

In reply to Namhyung Kim

The name should be freed on error path.

---
 tools/perf/util/dso.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/dso.c +3 -1
index b27d127cdf68..1f29e4fe7af0 100644
@@ -412,8 +412,10 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
-	if (!is_regular_file(name))
+	if (!is_regular_file(name)) {
+		free(name);
 		return -EINVAL;
+	}
 
 	fd = do_open(name);
 	free(name);
Signature · 1 line
2.13.1
#4 lore raw

[tip:perf/urgent] perf tools: Fix a memory leak in __open_dso()

perf tools: Fix a memory leak in __open_dso()
Commit-ID:  44ad6b8852529eb39066edbedc027a6901da6803
Gitweb:     http://git.kernel.org/tip/44ad6b8852529eb39066edbedc027a6901da6803
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:02 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:38:47 -0300

perf tools: Fix a memory leak in __open_dso()

The 'name' variable should be freed on the error path.

---
 tools/perf/util/dso.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/dso.c +3 -1
index b27d127..1f29e4f 100644
@@ -412,8 +412,10 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
-	if (!is_regular_file(name))
+	if (!is_regular_file(name)) {
+		free(name);
 		return -EINVAL;
+	}
 
 	fd = do_open(name);
 	free(name);
author maintainer
#5 lore raw

PATCH v4 3/9 perf tools: Introduce dso__decompress_kmodule_{fd,path}

In reply to Namhyung Kim

Move decompress_kmodule() to util/dso.c and split it to two functions returning fd and (decompressed) file path. Existing user only wants the fd version but the path version will be used soon.

---
 tools/perf/util/dso.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.h        |  6 +++++
 tools/perf/util/symbol-elf.c | 36 +--------------------------
 3 files changed, 65 insertions(+), 35 deletions(-)
tools/perf/util/dso.c +58 -0
index 1f29e4fe7af0..b346b8eba65c 100644
@@ -248,6 +248,64 @@ bool dso__needs_decompress(struct dso *dso)
 		dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
 }
 
+static int decompress_kmodule(struct dso *dso, const char *name, char *tmpbuf)
+{
+	int fd = -1;
+	struct kmod_path m;
+
+	if (!dso__needs_decompress(dso))
+		return -1;
+
+	if (kmod_path__parse_ext(&m, dso->long_name))
+		return -1;
+
+	if (!m.comp)
+		goto out;
+
+	fd = mkstemp(tmpbuf);
+	if (fd < 0) {
+		dso->load_errno = errno;
+		goto out;
+	}
+
+	if (!decompress_to_file(m.ext, name, fd)) {
+		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
+		close(fd);
+		fd = -1;
+	}
+
+out:
+	free(m.ext);
+	return fd;
+}
+
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name)
+{
+	char tmpbuf[] = KMOD_DECOMP_NAME;
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	unlink(tmpbuf);
+	return fd;
+}
+
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len)
+{
+	char tmpbuf[] = KMOD_DECOMP_NAME;
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	if (fd < 0) {
+		unlink(tmpbuf);
+		return -1;
+	}
+
+	strncpy(pathname, tmpbuf, len);
+	close(fd);
+	return 0;
+}
+
 /*
  * Parses kernel module specified in @path and updates
  * @m argument like:
tools/perf/util/dso.h +6 -0
index 5fe2ab5877bd..bd061ba7b47c 100644
@@ -244,6 +244,12 @@ bool is_supported_compression(const char *ext);
 bool is_kernel_module(const char *pathname, int cpumode);
 bool decompress_to_file(const char *ext, const char *filename, int output_fd);
 bool dso__needs_decompress(struct dso *dso);
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name);
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len);
+
+#define KMOD_DECOMP_NAME  "/tmp/perf-kmod-XXXXXX"
+#define KMOD_DECOMP_LEN   sizeof(KMOD_DECOMP_NAME)
 
 struct kmod_path {
 	char *name;
tools/perf/util/symbol-elf.c +1 -35
index 1fb2efae4f02..d342e771dbad 100644
@@ -637,40 +637,6 @@ static int dso__swap_init(struct dso *dso, unsigned char eidata)
 	return 0;
 }
 
-static int decompress_kmodule(struct dso *dso, const char *name,
-			      enum dso_binary_type type)
-{
-	int fd = -1;
-	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
-	struct kmod_path m;
-
-	if (type != DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__GUEST_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		return -1;
-
-	if (kmod_path__parse_ext(&m, dso->long_name) || !m.comp)
-		return -1;
-
-	fd = mkstemp(tmpbuf);
-	if (fd < 0) {
-		dso->load_errno = errno;
-		goto out;
-	}
-
-	if (!decompress_to_file(m.ext, name, fd)) {
-		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
-		close(fd);
-		fd = -1;
-	}
-
-	unlink(tmpbuf);
-
-out:
-	free(m.ext);
-	return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
 	return ss->dynsym || ss->opdsec;
@@ -702,7 +668,7 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	int fd;
 
 	if (dso__needs_decompress(dso)) {
-		fd = decompress_kmodule(dso, name, type);
+		fd = dso__decompress_kmodule_fd(dso, name);
 		if (fd < 0)
 			return -1;
 	} else {
Signature · 1 line
2.13.1
#6 lore raw

[tip:perf/urgent] perf tools: Introduce dso__decompress_kmodule_{fd,path}

perf tools: Introduce dso__decompress_kmodule_{fd,path}
Commit-ID:  42b3fa670825983fc8bd0ac7b80cc84ae3abb75b
Gitweb:     http://git.kernel.org/tip/42b3fa670825983fc8bd0ac7b80cc84ae3abb75b
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:03 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:38:55 -0300

perf tools: Introduce dso__decompress_kmodule_{fd,path}

Move decompress_kmodule() to util/dso.c and split it into two functions returning fd and (decompressed) file path. The existing user only wants the fd version but the path version will be used soon.

---
 tools/perf/util/dso.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.h        |  6 +++++
 tools/perf/util/symbol-elf.c | 36 +--------------------------
 3 files changed, 65 insertions(+), 35 deletions(-)
tools/perf/util/dso.c +58 -0
index 1f29e4f..b346b8e 100644
@@ -248,6 +248,64 @@ bool dso__needs_decompress(struct dso *dso)
 		dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
 }
 
+static int decompress_kmodule(struct dso *dso, const char *name, char *tmpbuf)
+{
+	int fd = -1;
+	struct kmod_path m;
+
+	if (!dso__needs_decompress(dso))
+		return -1;
+
+	if (kmod_path__parse_ext(&m, dso->long_name))
+		return -1;
+
+	if (!m.comp)
+		goto out;
+
+	fd = mkstemp(tmpbuf);
+	if (fd < 0) {
+		dso->load_errno = errno;
+		goto out;
+	}
+
+	if (!decompress_to_file(m.ext, name, fd)) {
+		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
+		close(fd);
+		fd = -1;
+	}
+
+out:
+	free(m.ext);
+	return fd;
+}
+
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name)
+{
+	char tmpbuf[] = KMOD_DECOMP_NAME;
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	unlink(tmpbuf);
+	return fd;
+}
+
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len)
+{
+	char tmpbuf[] = KMOD_DECOMP_NAME;
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	if (fd < 0) {
+		unlink(tmpbuf);
+		return -1;
+	}
+
+	strncpy(pathname, tmpbuf, len);
+	close(fd);
+	return 0;
+}
+
 /*
  * Parses kernel module specified in @path and updates
  * @m argument like:
tools/perf/util/dso.h +6 -0
index 5fe2ab5..bd061ba 100644
@@ -244,6 +244,12 @@ bool is_supported_compression(const char *ext);
 bool is_kernel_module(const char *pathname, int cpumode);
 bool decompress_to_file(const char *ext, const char *filename, int output_fd);
 bool dso__needs_decompress(struct dso *dso);
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name);
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len);
+
+#define KMOD_DECOMP_NAME  "/tmp/perf-kmod-XXXXXX"
+#define KMOD_DECOMP_LEN   sizeof(KMOD_DECOMP_NAME)
 
 struct kmod_path {
 	char *name;
tools/perf/util/symbol-elf.c +1 -35
index 1fb2efa..d342e77 100644
@@ -637,40 +637,6 @@ static int dso__swap_init(struct dso *dso, unsigned char eidata)
 	return 0;
 }
 
-static int decompress_kmodule(struct dso *dso, const char *name,
-			      enum dso_binary_type type)
-{
-	int fd = -1;
-	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
-	struct kmod_path m;
-
-	if (type != DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__GUEST_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		return -1;
-
-	if (kmod_path__parse_ext(&m, dso->long_name) || !m.comp)
-		return -1;
-
-	fd = mkstemp(tmpbuf);
-	if (fd < 0) {
-		dso->load_errno = errno;
-		goto out;
-	}
-
-	if (!decompress_to_file(m.ext, name, fd)) {
-		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
-		close(fd);
-		fd = -1;
-	}
-
-	unlink(tmpbuf);
-
-out:
-	free(m.ext);
-	return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
 	return ss->dynsym || ss->opdsec;
@@ -702,7 +668,7 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	int fd;
 
 	if (dso__needs_decompress(dso)) {
-		fd = decompress_kmodule(dso, name, type);
+		fd = dso__decompress_kmodule_fd(dso, name);
 		if (fd < 0)
 			return -1;
 	} else {
author maintainer
#7 lore raw

PATCH v4 4/9 perf annotate: Use dso__decompress_kmodule_path()

In reply to Namhyung Kim

Convert open-coded decompress routine to use the function.

---
 tools/perf/util/annotate.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)
tools/perf/util/annotate.c +3 -24
index df4486c3a2fa..ddbd56df9187 100644
@@ -1431,31 +1431,10 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 				sizeof(symfs_filename));
 		}
 	} else if (dso__needs_decompress(dso)) {
-		char tmp[PATH_MAX];
-		struct kmod_path m;
-		int fd;
-		bool ret;
+		char tmp[KMOD_DECOMP_LEN];
 
-		if (kmod_path__parse_ext(&m, symfs_filename))
-			goto out;
-
-		snprintf(tmp, PATH_MAX, "/tmp/perf-kmod-XXXXXX");
-
-		fd = mkstemp(tmp);
-		if (fd < 0) {
-			free(m.ext);
-			goto out;
-		}
-
-		ret = decompress_to_file(m.ext, symfs_filename, fd);
-
-		if (ret)
-			pr_err("Cannot decompress %s %s\n", m.ext, symfs_filename);
-
-		free(m.ext);
-		close(fd);
-
-		if (!ret)
+		if (dso__decompress_kmodule_path(dso, symfs_filename,
+						 tmp, sizeof(tmp)) < 0)
 			goto out;
 
 		strcpy(symfs_filename, tmp);
Signature · 1 line
2.13.1
#8 lore raw

[tip:perf/urgent] perf annotate: Use dso__decompress_kmodule_path()

perf annotate: Use dso__decompress_kmodule_path()
Commit-ID:  3c84fd53044f98017271101b59a21ddb20fb312b
Gitweb:     http://git.kernel.org/tip/3c84fd53044f98017271101b59a21ddb20fb312b
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:04 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:02 -0300

perf annotate: Use dso__decompress_kmodule_path()

Convert open-coded decompress routine to use the function.

---
 tools/perf/util/annotate.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)
tools/perf/util/annotate.c +3 -24
index df4486c..ddbd56d 100644
@@ -1431,31 +1431,10 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 				sizeof(symfs_filename));
 		}
 	} else if (dso__needs_decompress(dso)) {
-		char tmp[PATH_MAX];
-		struct kmod_path m;
-		int fd;
-		bool ret;
+		char tmp[KMOD_DECOMP_LEN];
 
-		if (kmod_path__parse_ext(&m, symfs_filename))
-			goto out;
-
-		snprintf(tmp, PATH_MAX, "/tmp/perf-kmod-XXXXXX");
-
-		fd = mkstemp(tmp);
-		if (fd < 0) {
-			free(m.ext);
-			goto out;
-		}
-
-		ret = decompress_to_file(m.ext, symfs_filename, fd);
-
-		if (ret)
-			pr_err("Cannot decompress %s %s\n", m.ext, symfs_filename);
-
-		free(m.ext);
-		close(fd);
-
-		if (!ret)
+		if (dso__decompress_kmodule_path(dso, symfs_filename,
+						 tmp, sizeof(tmp)) < 0)
 			goto out;
 
 		strcpy(symfs_filename, tmp);
author maintainer
#9 lore raw

PATCH v4 5/9 perf tools: Decompress kernel module when reading DSO data

In reply to Namhyung Kim

Currently perf decompresses kernel modules when loading symbol table but it missed to do it when reading raw data.

---
 tools/perf/util/dso.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
tools/perf/util/dso.c +16 -0
index b346b8eba65c..c63525d845c5 100644
@@ -475,7 +475,23 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
+	if (dso__needs_decompress(dso)) {
+		char newpath[KMOD_DECOMP_LEN];
+		size_t len = sizeof(newpath);
+
+		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
+			free(name);
+			return -dso->load_errno;
+		}
+
+		strcpy(name, newpath);
+	}
+
 	fd = do_open(name);
+
+	if (dso__needs_decompress(dso))
+		unlink(name);
+
 	free(name);
 	return fd;
 }
Signature · 1 line
2.13.1
#10 lore raw

[tip:perf/urgent] perf tools: Decompress kernel module when reading DSO data

perf tools: Decompress kernel module when reading DSO data
Commit-ID:  1d6b3c9ba756a5134fd7ad1959acac776d17404b
Gitweb:     http://git.kernel.org/tip/1d6b3c9ba756a5134fd7ad1959acac776d17404b
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:05 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:07 -0300

perf tools: Decompress kernel module when reading DSO data

Currently perf decompresses kernel modules when loading the symbol table but it missed to do it when reading raw data.

---
 tools/perf/util/dso.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
tools/perf/util/dso.c +16 -0
index b346b8e..c63525d 100644
@@ -475,7 +475,23 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
+	if (dso__needs_decompress(dso)) {
+		char newpath[KMOD_DECOMP_LEN];
+		size_t len = sizeof(newpath);
+
+		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
+			free(name);
+			return -dso->load_errno;
+		}
+
+		strcpy(name, newpath);
+	}
+
 	fd = do_open(name);
+
+	if (dso__needs_decompress(dso))
+		unlink(name);
+
 	free(name);
 	return fd;
 }
author maintainer
#11 lore raw

PATCH v4 6/9 perf tools: Consolidate error path in __open_dso()

In reply to Namhyung Kim

On failure, it should free the 'name', so clean up the error path using goto.

---
 tools/perf/util/dso.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
tools/perf/util/dso.c +8 -11
index c63525d845c5..4e7ab611377a 100644
@@ -454,7 +454,7 @@ static int do_open(char *name)
 
 static int __open_dso(struct dso *dso, struct machine *machine)
 {
-	int fd;
+	int fd = -EINVAL;
 	char *root_dir = (char *)"";
 	char *name = malloc(PATH_MAX);
 
@@ -465,23 +465,19 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		root_dir = machine->root_dir;
 
 	if (dso__read_binary_type_filename(dso, dso->binary_type,
-					    root_dir, name, PATH_MAX)) {
-		free(name);
-		return -EINVAL;
-	}
+					    root_dir, name, PATH_MAX))
+		goto out;
 
-	if (!is_regular_file(name)) {
-		free(name);
-		return -EINVAL;
-	}
+	if (!is_regular_file(name))
+		goto out;
 
 	if (dso__needs_decompress(dso)) {
 		char newpath[KMOD_DECOMP_LEN];
 		size_t len = sizeof(newpath);
 
 		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
-			free(name);
-			return -dso->load_errno;
+			fd = -dso->load_errno;
+			goto out;
 		}
 
 		strcpy(name, newpath);
@@ -492,6 +488,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	if (dso__needs_decompress(dso))
 		unlink(name);
 
+out:
 	free(name);
 	return fd;
 }
Signature · 1 line
2.13.1
#12 lore raw

[tip:perf/urgent] perf tools: Consolidate error path in __open_dso()

perf tools: Consolidate error path in __open_dso()
Commit-ID:  8ba29adf9a7cdff3c7283d5fc0dcf5e777d3b40f
Gitweb:     http://git.kernel.org/tip/8ba29adf9a7cdff3c7283d5fc0dcf5e777d3b40f
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:06 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:13 -0300

perf tools: Consolidate error path in __open_dso()

On failure, it should free the 'name', so clean up the error path using goto.

---
 tools/perf/util/dso.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
tools/perf/util/dso.c +8 -11
index c63525d..4e7ab61 100644
@@ -454,7 +454,7 @@ static int do_open(char *name)
 
 static int __open_dso(struct dso *dso, struct machine *machine)
 {
-	int fd;
+	int fd = -EINVAL;
 	char *root_dir = (char *)"";
 	char *name = malloc(PATH_MAX);
 
@@ -465,23 +465,19 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		root_dir = machine->root_dir;
 
 	if (dso__read_binary_type_filename(dso, dso->binary_type,
-					    root_dir, name, PATH_MAX)) {
-		free(name);
-		return -EINVAL;
-	}
+					    root_dir, name, PATH_MAX))
+		goto out;
 
-	if (!is_regular_file(name)) {
-		free(name);
-		return -EINVAL;
-	}
+	if (!is_regular_file(name))
+		goto out;
 
 	if (dso__needs_decompress(dso)) {
 		char newpath[KMOD_DECOMP_LEN];
 		size_t len = sizeof(newpath);
 
 		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
-			free(name);
-			return -dso->load_errno;
+			fd = -dso->load_errno;
+			goto out;
 		}
 
 		strcpy(name, newpath);
@@ -492,6 +488,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	if (dso__needs_decompress(dso))
 		unlink(name);
 
+out:
 	free(name);
 	return fd;
 }
author maintainer
#13 lore raw

PATCH v4 7/9 perf test: Decompress kernel module before objdump

In reply to Namhyung Kim

If a kernel modules is compressed, it should be decompressed before running objdump to parse binary data correctly. This fixes a failure of object code reading test for me.

---
 tools/perf/tests/code-reading.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
tools/perf/tests/code-reading.c +19 -1
index 1f14e7612cbb..94b7c7b02bde 100644
@@ -229,6 +229,8 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 	unsigned char buf2[BUFSZ];
 	size_t ret_len;
 	u64 objdump_addr;
+	const char *objdump_name;
+	char decomp_name[KMOD_DECOMP_LEN];
 	int ret;
 
 	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
@@ -289,9 +291,25 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 		state->done[state->done_cnt++] = al.map->start;
 	}
 
+	objdump_name = al.map->dso->long_name;
+	if (dso__needs_decompress(al.map->dso)) {
+		if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
+						 decomp_name,
+						 sizeof(decomp_name)) < 0) {
+			pr_debug("decompression failed\n");
+			return -1;
+		}
+
+		objdump_name = decomp_name;
+	}
+
 	/* Read the object code using objdump */
 	objdump_addr = map__rip_2objdump(al.map, al.addr);
-	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
+	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
+
+	if (dso__needs_decompress(al.map->dso))
+		unlink(objdump_name);
+
 	if (ret > 0) {
 		/*
 		 * The kernel maps are inaccurate - assume objdump is right in
Signature · 1 line
2.13.1
reviewer Acked-by on v4 7/9
#14 lore raw

Re: [PATCH v4 7/9] perf test: Decompress kernel module before objdump

Quoted 5 lines · Namhyung Kim

If a kernel modules is compressed, it should be decompressed before running objdump to parse binary data correctly. This fixes a failure of object code reading test for me.

Quoted 45 lines
---
 tools/perf/tests/code-reading.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
tools/perf/tests/code-reading.c +19 -1
index 1f14e7612cbb..94b7c7b02bde 100644
@@ -229,6 +229,8 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 	unsigned char buf2[BUFSZ];
 	size_t ret_len;
 	u64 objdump_addr;
+	const char *objdump_name;
+	char decomp_name[KMOD_DECOMP_LEN];
 	int ret;
 
 	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
@@ -289,9 +291,25 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 		state->done[state->done_cnt++] = al.map->start;
 	}
 
+	objdump_name = al.map->dso->long_name;
+	if (dso__needs_decompress(al.map->dso)) {
+		if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
+						 decomp_name,
+						 sizeof(decomp_name)) < 0) {
+			pr_debug("decompression failed\n");
+			return -1;
+		}
+
+		objdump_name = decomp_name;
+	}
+
 	/* Read the object code using objdump */
 	objdump_addr = map__rip_2objdump(al.map, al.addr);
-	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
+	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
+
+	if (dso__needs_decompress(al.map->dso))
+		unlink(objdump_name);
+
 	if (ret > 0) {
 		/*
 		 * The kernel maps are inaccurate - assume objdump is right in
#15 lore raw

[tip:perf/urgent] perf tests: Decompress kernel module before objdump

In reply to Namhyung Kim
perf tests: Decompress kernel module before objdump
Commit-ID:  94df1040b1e6aacd8dec0ba3c61d7e77cd695f26
Gitweb:     http://git.kernel.org/tip/94df1040b1e6aacd8dec0ba3c61d7e77cd695f26
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:07 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:19 -0300

perf tests: Decompress kernel module before objdump

If a kernel modules is compressed, it should be decompressed before running objdump to parse binary data correctly. This fixes a failure of object code reading test for me.

---
 tools/perf/tests/code-reading.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
tools/perf/tests/code-reading.c +19 -1
index 1f14e76..94b7c7b 100644
@@ -229,6 +229,8 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 	unsigned char buf2[BUFSZ];
 	size_t ret_len;
 	u64 objdump_addr;
+	const char *objdump_name;
+	char decomp_name[KMOD_DECOMP_LEN];
 	int ret;
 
 	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
@@ -289,9 +291,25 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 		state->done[state->done_cnt++] = al.map->start;
 	}
 
+	objdump_name = al.map->dso->long_name;
+	if (dso__needs_decompress(al.map->dso)) {
+		if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
+						 decomp_name,
+						 sizeof(decomp_name)) < 0) {
+			pr_debug("decompression failed\n");
+			return -1;
+		}
+
+		objdump_name = decomp_name;
+	}
+
 	/* Read the object code using objdump */
 	objdump_addr = map__rip_2objdump(al.map, al.addr);
-	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
+	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
+
+	if (dso__needs_decompress(al.map->dso))
+		unlink(objdump_name);
+
 	if (ret > 0) {
 		/*
 		 * The kernel maps are inaccurate - assume objdump is right in
author maintainer
#16 lore raw

PATCH v4 8/9 perf symbols: Keep DSO->symtab_type after decompress

In reply to Namhyung Kim

The symsrc__init() overwrites dso->symtab_type as symsrc->type in dso__load_sym(). But for compressed kernel modules in the build-id cache, it should have original symtab type to be decompressed as needed.

This fixes perf annotate to show disassembly of the function properly.

---
 tools/perf/util/symbol-elf.c | 2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/symbol-elf.c +2 -0
index d342e771dbad..502505cf236a 100644
@@ -671,6 +671,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 		fd = dso__decompress_kmodule_fd(dso, name);
 		if (fd < 0)
 			return -1;
+
+		type = dso->symtab_type;
 	} else {
 		fd = open(name, O_RDONLY);
 		if (fd < 0) {
Signature · 1 line
2.13.1
#17 lore raw

[tip:perf/urgent] perf symbols: Keep DSO->symtab_type after decompress

perf symbols: Keep DSO->symtab_type after decompress
Commit-ID:  c25ec42f846f702f8f532fbc890171e3a1f6ec85
Gitweb:     http://git.kernel.org/tip/c25ec42f846f702f8f532fbc890171e3a1f6ec85
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:08 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:26 -0300

perf symbols: Keep DSO->symtab_type after decompress

The symsrc__init() overwrites dso->symtab_type as symsrc->type in dso__load_sym(). But for compressed kernel modules in the build-id cache, it should have original symtab type to be decompressed as needed.

This fixes perf annotate to show disassembly of the function properly.

---
 tools/perf/util/symbol-elf.c | 2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/symbol-elf.c +2 -0
index d342e77..502505c 100644
@@ -671,6 +671,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 		fd = dso__decompress_kmodule_fd(dso, name);
 		if (fd < 0)
 			return -1;
+
+		type = dso->symtab_type;
 	} else {
 		fd = open(name, O_RDONLY);
 		if (fd < 0) {
author maintainer
#18 lore raw

PATCH v4 9/9 perf symbols: Kill dso__build_id_is_kmod()

In reply to Namhyung Kim

The commit e7ee40475760 ("perf symbols: Fix symbols searching for module in buildid-cache") added the function to check kernel modules reside in the build-id cache. This was because there's no way to identify a DSO which is actually a kernel module. So it searched linkname of the file and find ".ko" suffix.

But this does not work for compressed kernel modules and now such DSOs have correct symtab_type now. So no need to check it anymore. This patch essentially reverts the commit.

---
 tools/perf/util/build-id.c | 45 ---------------------------------------------
 tools/perf/util/build-id.h |  1 -
 tools/perf/util/symbol.c   |  4 ----
 3 files changed, 50 deletions(-)
tools/perf/util/build-id.c +0 -45
index 168cc49654e7..e0148b081bdf 100644
@@ -278,51 +278,6 @@ char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 	return bf;
 }
 
-bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)
-{
-	char *id_name = NULL, *ch;
-	struct stat sb;
-	char sbuild_id[SBUILD_ID_SIZE];
-
-	if (!dso->has_build_id)
-		goto err;
-
-	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
-	id_name = build_id_cache__linkname(sbuild_id, NULL, 0);
-	if (!id_name)
-		goto err;
-	if (access(id_name, F_OK))
-		goto err;
-	if (lstat(id_name, &sb) == -1)
-		goto err;
-	if ((size_t)sb.st_size > size - 1)
-		goto err;
-	if (readlink(id_name, bf, size - 1) < 0)
-		goto err;
-
-	bf[sb.st_size] = '\0';
-
-	/*
-	 * link should be:
-	 * ../../lib/modules/4.4.0-rc4/kernel/net/ipv4/netfilter/nf_nat_ipv4.ko/a09fe3eb3147dafa4e3b31dbd6257e4d696bdc92
-	 */
-	ch = strrchr(bf, '/');
-	if (!ch)
-		goto err;
-	if (ch - 3 < bf)
-		goto err;
-
-	free(id_name);
-	return strncmp(".ko", ch - 3, 3) == 0;
-err:
-	pr_err("Invalid build id: %s\n", id_name ? :
-					 dso->long_name ? :
-					 dso->short_name ? :
-					 "[unknown]");
-	free(id_name);
-	return false;
-}
-
 #define dsos__for_each_with_build_id(pos, head)	\
 	list_for_each_entry(pos, head, node)	\
 		if (!pos->has_build_id)		\
tools/perf/util/build-id.h +0 -1
index 8a89b195c1fc..96690a55c62c 100644
@@ -17,7 +17,6 @@ char *build_id_cache__kallsyms_path(const char *sbuild_id, char *bf,
 				    size_t size);
 
 char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size);
-bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size);
 
 int build_id__mark_dso_hit(struct perf_tool *tool, union perf_event *event,
 			   struct perf_sample *sample, struct perf_evsel *evsel,
tools/perf/util/symbol.c +0 -4
index 8f2b068ff756..e7a98dbd2aed 100644
@@ -1562,10 +1562,6 @@ int dso__load(struct dso *dso, struct map *map)
 	if (!runtime_ss && syms_ss)
 		runtime_ss = syms_ss;
 
-	if (syms_ss && syms_ss->type == DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		if (dso__build_id_is_kmod(dso, name, PATH_MAX))
-			kmod = true;
-
 	if (syms_ss)
 		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
 	else
Signature · 1 line
2.13.1
#19 lore raw

[tip:perf/urgent] perf symbols: Kill dso__build_id_is_kmod()

perf symbols: Kill dso__build_id_is_kmod()
Commit-ID:  b89fe63fbafe307fb72546f7a2320380bf41bdd4
Gitweb:     http://git.kernel.org/tip/b89fe63fbafe307fb72546f7a2320380bf41bdd4
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 8 Jun 2017 16:31:09 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Jun 2017 15:39:34 -0300

perf symbols: Kill dso__build_id_is_kmod()

The commit e7ee40475760 ("perf symbols: Fix symbols searching for module in buildid-cache") added the function to check kernel modules reside in the build-id cache. This was because there's no way to identify a DSO which is actually a kernel module. So it searched linkname of the file and find ".ko" suffix.

But this does not work for compressed kernel modules and now such DSOs hCcave correct symtab_type now. So no need to check it anymore. This patch essentially reverts the commit.

---
 tools/perf/util/build-id.c | 45 ---------------------------------------------
 tools/perf/util/build-id.h |  1 -
 tools/perf/util/symbol.c   |  4 ----
 3 files changed, 50 deletions(-)
tools/perf/util/build-id.c +0 -45
index 168cc49..e0148b0 100644
@@ -278,51 +278,6 @@ char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 	return bf;
 }
 
-bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)
-{
-	char *id_name = NULL, *ch;
-	struct stat sb;
-	char sbuild_id[SBUILD_ID_SIZE];
-
-	if (!dso->has_build_id)
-		goto err;
-
-	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
-	id_name = build_id_cache__linkname(sbuild_id, NULL, 0);
-	if (!id_name)
-		goto err;
-	if (access(id_name, F_OK))
-		goto err;
-	if (lstat(id_name, &sb) == -1)
-		goto err;
-	if ((size_t)sb.st_size > size - 1)
-		goto err;
-	if (readlink(id_name, bf, size - 1) < 0)
-		goto err;
-
-	bf[sb.st_size] = '\0';
-
-	/*
-	 * link should be:
-	 * ../../lib/modules/4.4.0-rc4/kernel/net/ipv4/netfilter/nf_nat_ipv4.ko/a09fe3eb3147dafa4e3b31dbd6257e4d696bdc92
-	 */
-	ch = strrchr(bf, '/');
-	if (!ch)
-		goto err;
-	if (ch - 3 < bf)
-		goto err;
-
-	free(id_name);
-	return strncmp(".ko", ch - 3, 3) == 0;
-err:
-	pr_err("Invalid build id: %s\n", id_name ? :
-					 dso->long_name ? :
-					 dso->short_name ? :
-					 "[unknown]");
-	free(id_name);
-	return false;
-}
-
 #define dsos__for_each_with_build_id(pos, head)	\
 	list_for_each_entry(pos, head, node)	\
 		if (!pos->has_build_id)		\
tools/perf/util/build-id.h +0 -1
index 8a89b19..96690a5 100644
@@ -17,7 +17,6 @@ char *build_id_cache__kallsyms_path(const char *sbuild_id, char *bf,
 				    size_t size);
 
 char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size);
-bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size);
 
 int build_id__mark_dso_hit(struct perf_tool *tool, union perf_event *event,
 			   struct perf_sample *sample, struct perf_evsel *evsel,
tools/perf/util/symbol.c +0 -4
index 8f2b068..e7a98db 100644
@@ -1562,10 +1562,6 @@ int dso__load(struct dso *dso, struct map *map)
 	if (!runtime_ss && syms_ss)
 		runtime_ss = syms_ss;
 
-	if (syms_ss && syms_ss->type == DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		if (dso__build_id_is_kmod(dso, name, PATH_MAX))
-			kmod = true;
-
 	if (syms_ss)
 		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
 	else
Acked-by on the v1 series
#20 lore raw
In reply to Namhyung Kim
Quoted 18 lines · Namhyung Kim

Hello,

This is v4 of my compressed kernel module work. Please take a look.

* changes in v4)
- fix build-id cache symbolic link handling in annotate
- separate fix for a memory leak (Arnaldo)
- consolidate error path in __open_dso (Arnaldo)
- remove decompressed file after use

The code is avaiable at 'perf/kmod-decomp-v4' branch in my tree:

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Quoted 10 lines

Namhyung Kim (9):
perf annotate: Fix symbolic link of build-id cache perf tools: Fix a memory leak in __open_dso() perf tools: Introduce dso__decompress_kmodule_{fd,path} perf annotate: Use dso__decompress_kmodule_path() perf tools: Decompress kernel module when reading DSO data perf tools: Consolidate error path in __open_dso() perf test: Decompress kernel module before objdump perf symbols: Keep DSO->symtab_type after decompress perf symbols: Kill dso__build_id_is_kmod()

thanks,
jirka