Skip to contentSkip to conversation

perf/probe: verify instruction/offset in perf before adding a uprobe

PATCH Superseded Prashanth Nageshappa 2 participants 2 messages Apr 24, 2012 1 message not in the archive

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH v2 1/1 perf/probe: verify instruction/offset in perf before adding a uprobe

This patch is to augment Srikar's perf support for uprobes patch
(https://lkml.org/lkml/2012/4/11/191) with the following features:

a. Instruction verification for user space tracing
b. Function boundary validation support to uprobes as its kernel counterpart (Commit-ID: 1c1bc922).

This will help in ensuring uprobe is placed at right location inside the intended function.

To verify instructions in perf before adding a uprobe, we need to use arch/x86/lib/insn.c. Since perf Makefile enables -Wswitch-default flag it causes build warnings/failures. Masami's patch
https://lkml.org/lkml/2012/4/12/598 addresses those warnings and it is a pre-req for this patch.

v2 addresses Masami's review comments: Rebuild insn.c and inat.c while building perf and a few other minor ones.

---

 tools/perf/Makefile                    |    1 
 tools/perf/arch/x86/Makefile           |   20 ++++++++
 tools/perf/arch/x86/util/probe-event.c |   84 ++++++++++++++++++++++++++++++++
 tools/perf/util/include/linux/string.h |    1 
 tools/perf/util/probe-event.c          |   22 ++++++++
 tools/perf/util/probe-event.h          |    2 +
 tools/perf/util/symbol.c               |    2 +
 tools/perf/util/symbol.h               |    1 
 8 files changed, 132 insertions(+), 1 deletions(-)
 create mode 100644 tools/perf/arch/x86/util/probe-event.c
tools/perf/Makefile +1 -0
index 820371f..94879b9 100644
@@ -61,6 +61,7 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/ -e s/sun4u/sparc64/ \
 
 CC = $(CROSS_COMPILE)gcc
 AR = $(CROSS_COMPILE)ar
+AWK = awk
 
 # Additional ARCH settings for x86
 ifeq ($(ARCH),i386)
tools/perf/arch/x86/Makefile +20 -0
index 744e629..1a7215b 100644
@@ -1,5 +1,25 @@
+inat_tables_script = ../../arch/$(ARCH)/tools/gen-insn-attr-x86.awk
+inat_tables_maps = ../../arch/$(ARCH)/lib/x86-opcode-map.txt
+cmd_inat_tables = $(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
+
+BASIC_CFLAGS += -I. -I../../arch/$(ARCH)/include
+
 ifndef NO_DWARF
 PERF_HAVE_DWARF_REGS := 1
 LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o
 endif
 LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/header.o
+LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/probe-event.o
+LIB_OBJS += $(OUTPUT)arch/$(ARCH)/inat.o
+LIB_OBJS += $(OUTPUT)arch/$(ARCH)/insn.o
+
+$(OUTPUT)arch/$(ARCH)/inat.o:../../arch/$(ARCH)/lib/inat-tables.c
+
+../../arch/$(ARCH)/lib/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
+	$(cmd_inat_tables)
+
+$(OUTPUT)arch/$(ARCH)/inat.o:../../arch/$(ARCH)/lib/inat.c $(OUTPUT)PERF-CFLAGS
+	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
+
+$(OUTPUT)arch/$(ARCH)/insn.o:../../arch/$(ARCH)/lib/insn.c $(OUTPUT)PERF-CFLAGS
+	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
tools/perf/arch/x86/util/probe-event.c new file +84 -0

84 lines not on this page · raw message

tools/perf/util/include/linux/string.h +1 -0
index 3b2f590..9d5eb21 100644
@@ -1 +1,2 @@
 #include <string.h>
+#include <perf.h>
tools/perf/util/probe-event.c +21 -1
index b7dec82..8f3de61 100644
@@ -2254,6 +2254,17 @@ int show_available_funcs(const char *target, struct strfilter *_filter,
 }
 
 /*
+ * Check if a given offset from start of a function is valid or not
+ */
+bool __attribute__((weak)) can_probe(char *name __used,
+					unsigned long long vaddr __used,
+					unsigned long offset __used,
+					u8 class __used)
+{
+	return true;
+}
+
+/*
  * uprobe_events only accepts address:
  * Convert function and any offset to address
  */
@@ -2307,7 +2318,16 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
 
 	if (map->start > sym->start)
 		vaddr = map->start;
-	vaddr += sym->start + pp->offset + map->pgoff;
+
+	vaddr += sym->start + map->pgoff;
+	if (pp->offset)
+		if ((vaddr + pp->offset > sym->end) ||
+			!can_probe(name, vaddr, pp->offset,
+					map->dso->class)) {
+			pr_err("Failed to insert probe, ensure offset is within function and on insn boundary.\n");
+			return -EINVAL;
+		}
+	vaddr += pp->offset;
 	pp->offset = 0;
 
 	if (!pev->event) {
tools/perf/util/probe-event.h +2 -0
index f9f3de8..af96594 100644
@@ -137,4 +137,6 @@ extern int show_available_funcs(const char *module, struct strfilter *filter,
 /* Maximum index number of event-name postfix */
 #define MAX_EVENT_INDEX	1024
 
+extern bool can_probe(char *name, unsigned long long vaddr,
+			unsigned long offset, u8 class);
 #endif /*_PROBE_EVENT_H */
tools/perf/util/symbol.c +2 -0
index caaf75a..be58b06 100644
@@ -1184,6 +1184,7 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name,
 		goto out_close;
 	}
 
+	dso->class = gelf_getclass(elf);
 	if (gelf_getehdr(elf, &ehdr) == NULL) {
 		pr_debug("%s: cannot get elf header.\n", __func__);
 		goto out_elf_end;
@@ -1326,6 +1327,7 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name,
 				curr_dso->kernel = dso->kernel;
 				curr_dso->long_name = dso->long_name;
 				curr_dso->long_name_len = dso->long_name_len;
+				curr_dso->class = dso->class;
 				curr_map = map__new2(start, curr_dso,
 						     map->type);
 				if (curr_map == NULL) {
tools/perf/util/symbol.h +1 -0
index 9e7742c..1d0cc28 100644
@@ -174,6 +174,7 @@ struct dso {
 	char	 	 *long_name;
 	u16		 long_name_len;
 	u16		 short_name_len;
+	u8		 class;
 	char		 name[0];
 };
 
#2 lore raw

Em Tue, Apr 24, 2012 at 12:06:46PM +0530, Prashanth Nageshappa escreveu:

Quoted 10 lines

This patch is to augment Srikar's perf support for uprobes patch
(https://lkml.org/lkml/2012/4/11/191) with the following features:

a. Instruction verification for user space tracing
b. Function boundary validation support to uprobes as its kernel counterpart (Commit-ID: 1c1bc922).

This will help in ensuring uprobe is placed at right location inside the intended function.

Quoted 8 lines

To verify instructions in perf before adding a uprobe, we need to use arch/x86/lib/insn.c. Since perf Makefile enables -Wswitch-default flag it causes build warnings/failures. Masami's patch
https://lkml.org/lkml/2012/4/12/598 addresses those warnings and it is a pre-req for this patch.

v2 addresses Masami's review comments: Rebuild insn.c and inat.c while building perf and a few other minor ones.

Some minor nits below

tools/perf/arch/x86/util/probe-event.c line 4

Quoted 7 lines
tools/perf/arch/x86/util/probe-event.c new file +4 -0
@@ -0,0 +1,84 @@
+bool can_probe(char *name, unsigned long long vaddr, unsigned long offset,
+		u8 class)
+{
+	unsigned long long eaddr, saddr;

Why not the much shorter 'u64'?

tools/perf/arch/x86/util/probe-event.c line 52

Quoted 12 lines
tools/perf/arch/x86/util/probe-event.c new file +12 -0
+	unsigned long fileoffset, readbytes;
+	int fd = 0;
+	bool ret = false;
+	char *buf = NULL;
+	struct insn insn;
+
+	fd = open(name, O_RDONLY);
+	if (fd == -1) {
+		pr_warning("Failed to open %s: %s\n", name, strerror(errno));
+		return ret;
+	}
+	buf = (char *)malloc(offset + MAX_INSN_SIZE);

No need to cast malloc's return

tools/perf/arch/x86/util/probe-event.c line 60

Quoted 8 lines
tools/perf/arch/x86/util/probe-event.c new file +8 -0
+	if (buf == NULL) {
+		pr_warning("Failed to allocate memory");
+		goto out;
+	}
+	fileoffset = lseek(fd, vaddr, SEEK_SET);
+	if (fileoffset != vaddr) {
+		pr_warning("Failed to lseek %s: %s\n", name, strerror(errno));
+		goto out;

Goto out_close and...

tools/perf/arch/x86/util/probe-event.c line 67

Quoted 7 lines
tools/perf/arch/x86/util/probe-event.c new file +7 -0
+	}
+	saddr = (unsigned long long)buf;
+	eaddr = (unsigned long long)buf + offset;
+	readbytes = read(fd, buf, offset + MAX_INSN_SIZE);
+	if (readbytes != offset + MAX_INSN_SIZE) {
+		pr_warning("Failed to read %s: %s\n", name, strerror(errno));
+		goto out;

out_close

tools/perf/arch/x86/util/probe-event.c line 78

Quoted 11 lines
tools/perf/arch/x86/util/probe-event.c new file +11 -0
+	}
+	while (saddr < eaddr) {
+		insn_init(&insn, (void *)saddr, class == ELFCLASS64);
+		insn_get_length(&insn);
+		saddr += insn.length;
+	}
+	ret = (saddr == eaddr);
+
+out:
+	if (buf)
+		free(buf);

free can handle NULL pointers just fine

tools/perf/arch/x86/util/probe-event.c line 81

Quoted 3 lines
tools/perf/arch/x86/util/probe-event.c new file +3 -0
+
+	if (fd)
+		close(fd);

You can move this to before 'free' and label it out_close, avoiding the test.

Quoted 36 lines
tools/perf/arch/x86/util/probe-event.c new file +3 -0
+
+	return ret;
+}
tools/perf/util/include/linux/string.h +1 -0
index 3b2f590..9d5eb21 100644
@@ -1 +1,2 @@
 #include <string.h>
+#include <perf.h>
tools/perf/util/probe-event.c +11 -0
index b7dec82..8f3de61 100644
@@ -2254,6 +2254,17 @@ int show_available_funcs(const char *target, struct strfilter *_filter,
 }
 
 /*
+ * Check if a given offset from start of a function is valid or not
+ */
+bool __attribute__((weak)) can_probe(char *name __used,
+					unsigned long long vaddr __used,
+					unsigned long offset __used,
+					u8 class __used)
+{
+	return true;
+}
+
+/*
  * uprobe_events only accepts address:
  * Convert function and any offset to address
  */
@@ -2307,7 +2318,16 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
 
 	if (map->start > sym->start)
 		vaddr = map->start;

tools/perf/util/probe-event.c static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec) · line 2329

Quoted 10 lines
tools/perf/util/probe-event.c +9 -1
-	vaddr += sym->start + pp->offset + map->pgoff;
+
+	vaddr += sym->start + map->pgoff;
+	if (pp->offset)
+		if ((vaddr + pp->offset > sym->end) ||
+			!can_probe(name, vaddr, pp->offset,
+					map->dso->class)) {
+			pr_err("Failed to insert probe, ensure offset is within function and on insn boundary.\n");
+			return -EINVAL;
+		}

Multi line if bodies gets visually nicer when enclosed with {}

Quoted 40 lines
tools/perf/util/probe-event.c +1 -0
+	vaddr += pp->offset;
 	pp->offset = 0;
 
 	if (!pev->event) {
tools/perf/util/probe-event.h +2 -0
index f9f3de8..af96594 100644
@@ -137,4 +137,6 @@ extern int show_available_funcs(const char *module, struct strfilter *filter,
 /* Maximum index number of event-name postfix */
 #define MAX_EVENT_INDEX	1024
 
+extern bool can_probe(char *name, unsigned long long vaddr,
+			unsigned long offset, u8 class);
 #endif /*_PROBE_EVENT_H */
tools/perf/util/symbol.c +2 -0
index caaf75a..be58b06 100644
@@ -1184,6 +1184,7 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name,
 		goto out_close;
 	}
 
+	dso->class = gelf_getclass(elf);
 	if (gelf_getehdr(elf, &ehdr) == NULL) {
 		pr_debug("%s: cannot get elf header.\n", __func__);
 		goto out_elf_end;
@@ -1326,6 +1327,7 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name,
 				curr_dso->kernel = dso->kernel;
 				curr_dso->long_name = dso->long_name;
 				curr_dso->long_name_len = dso->long_name_len;
+				curr_dso->class = dso->class;
 				curr_map = map__new2(start, curr_dso,
 						     map->type);
 				if (curr_map == NULL) {
tools/perf/util/symbol.h +0 -0
index 9e7742c..1d0cc28 100644

tools/perf/util/symbol.h struct dso { · line 180

Quoted 7 lines
tools/perf/util/symbol.h +1 -0
@@ -174,6 +174,7 @@ struct dso {
 	char	 	 *long_name;
 	u16		 long_name_len;
 	u16		 short_name_len;
+	u8		 class;
 	char		 name[0];
 };
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>