perf per-symbol histogram causes memory corruption

perf per-symbol histogram causes memory corruption

[David Miller]

When builtin-annotate.c processes sample events via
process_sample_event() it uses 'sample_filter'

sample_filter() sizes the histogram object for a symbol based upon the
size, calculated as "sym->end - sym->start", to determine the number
of IP sample slots to allocate.

The problem is, the sym->end value is not stable at this point.

For example, dso__load_sym() first loads all of the symbols, then it
makes another pass over the symbols by calling symbols__fixup_end()
which will adjust the sym->end values of various symbols.

At this point, the histogram IP sample array allocated by
sample_filter() can become too small, and hits recorded can thus
access past the end of the array corrupting memory.

I get this very reliably on sparc64, and it took me a few days to root
cause this. :-)

I don't see an immediate way to fix this, any ideas?

Re: perf per-symbol histogram causes memory corruption

[Ingo Molnar]

(Added Arnaldo to the Cc: - he's maintaining the symbol lookup bits of perf.)

* David Miller <davem@davemloft.net> wrote:

> When builtin-annotate.c processes sample events via
> process_sample_event() it uses 'sample_filter'
> 
> sample_filter() sizes the histogram object for a symbol based upon the
> size, calculated as "sym->end - sym->start", to determine the number
> of IP sample slots to allocate.
> 
> The problem is, the sym->end value is not stable at this point.
> 
> For example, dso__load_sym() first loads all of the symbols, then it
> makes another pass over the symbols by calling symbols__fixup_end()
> which will adjust the sym->end values of various symbols.
> 
> At this point, the histogram IP sample array allocated by
> sample_filter() can become too small, and hits recorded can thus
> access past the end of the array corrupting memory.
> 
> I get this very reliably on sparc64, and it took me a few days to root
> cause this. :-)
> 
> I don't see an immediate way to fix this, any ideas?

Re: perf per-symbol histogram causes memory corruption

[Arnaldo Carvalho de Melo]

Em Thu, Feb 25, 2010 at 12:11:48PM +0100, Ingo Molnar escreveu:
> 
> (Added Arnaldo to the Cc: - he's maintaining the symbol lookup bits of perf.)
> 
> * David Miller <davem@davemloft.net> wrote:
> 
> > When builtin-annotate.c processes sample events via
> > process_sample_event() it uses 'sample_filter'
> > 
> > sample_filter() sizes the histogram object for a symbol based upon the
> > size, calculated as "sym->end - sym->start", to determine the number
> > of IP sample slots to allocate.
> > 
> > The problem is, the sym->end value is not stable at this point.
> > 
> > For example, dso__load_sym() first loads all of the symbols, then it
> > makes another pass over the symbols by calling symbols__fixup_end()
> > which will adjust the sym->end values of various symbols.
> > 
> > At this point, the histogram IP sample array allocated by
> > sample_filter() can become too small, and hits recorded can thus
> > access past the end of the array corrupting memory.
> > 
> > I get this very reliably on sparc64, and it took me a few days to root
> > cause this. :-)
> > 
> > I don't see an immediate way to fix this, any ideas?

This was reported by Anton as well, I guess the right way to fix this is
to first load all symbols and then iterate thru them allocating the
resources that need to have ->end fixed up.

As we already fix up the symbols when we finish loading them, annotate
would have meaningful ->end members.

I.e. 'perf annotate' is not using this mechanism as a filter. Its a post
processing phase.

I'll provide a patch.

- Arnaldo

Re: perf per-symbol histogram causes memory corruption

[David Miller]

From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 25 Feb 2010 11:13:51 -0300

> I.e. 'perf annotate' is not using this mechanism as a filter. Its a post
> processing phase.

Right, it's abusing the filter callback.

> I'll provide a patch.

Sounds good.

Re: perf per-symbol histogram causes memory corruption

[Arnaldo Carvalho de Melo]

Em Thu, Feb 25, 2010 at 07:02:50AM -0800, David Miller escreveu:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Thu, 25 Feb 2010 11:13:51 -0300
> 
> > I.e. 'perf annotate' is not using this mechanism as a filter. Its a post
> > processing phase.
> 
> Right, it's abusing the filter callback.
> 
> > I'll provide a patch.
> 
> Sounds good.

Can you try this one?

>From f7f95900abe585e1fba887f5eecbc6a9e2686071 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 25 Feb 2010 13:50:26 -0300
Subject: [PATCH 1/1] perf annotate: Defer allocating sym_priv->hist array

Because symbol->end is not fixed up at symbol_filter time, only after all
symbols for a DSO are loaded, and that, for asm symbols, may be bogus, causing
segfaults when hits happen in these symbols.

Reported-by: David Miller <davem@davemloft.net>
Reported-by: Anton Blanchard <anton@samba.org>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-annotate.c |   59 +++++++++++++++++++++--------------------
 tools/perf/util/symbol.c      |    2 +-
 tools/perf/util/symbol.h      |    2 +
 3 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 28ea4e0..e47dd15 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -53,32 +53,20 @@ struct sym_priv {
 
 static const char *sym_hist_filter;
 
-static int symbol_filter(struct map *map __used, struct symbol *sym)
+static int sym__alloc_hist(struct symbol *self)
 {
-	if (sym_hist_filter == NULL ||
-	    strcmp(sym->name, sym_hist_filter) == 0) {
-		struct sym_priv *priv = symbol__priv(sym);
-		const int size = (sizeof(*priv->hist) +
-				  (sym->end - sym->start) * sizeof(u64));
+	struct sym_priv *priv = symbol__priv(self);
+	const int size = (sizeof(*priv->hist) +
+			  (self->end - self->start) * sizeof(u64));
 
-		priv->hist = malloc(size);
-		if (priv->hist)
-			memset(priv->hist, 0, size);
-		return 0;
-	}
-	/*
-	 * FIXME: We should really filter it out, as we don't want to go thru symbols
-	 * we're not interested, and if a DSO ends up with no symbols, delete it too,
-	 * but right now the kernel loading routines in symbol.c bail out if no symbols
-	 * are found, fix it later.
-	 */
-	return 0;
+	priv->hist = zalloc(size);
+	return priv->hist == NULL ? -1 : 0;
 }
 
 /*
  * collect histogram counts
  */
-static void hist_hit(struct hist_entry *he, u64 ip)
+static int annotate__hist_hit(struct hist_entry *he, u64 ip)
 {
 	unsigned int sym_size, offset;
 	struct symbol *sym = he->sym;
@@ -88,11 +76,11 @@ static void hist_hit(struct hist_entry *he, u64 ip)
 	he->count++;
 
 	if (!sym || !he->map)
-		return;
+		return 0;
 
 	priv = symbol__priv(sym);
-	if (!priv->hist)
-		return;
+	if (priv->hist == NULL && sym__alloc_hist(sym) < 0)
+		return -ENOMEM;
 
 	sym_size = sym->end - sym->start;
 	offset = ip - sym->start;
@@ -100,7 +88,7 @@ static void hist_hit(struct hist_entry *he, u64 ip)
 	pr_debug3("%s: ip=%#Lx\n", __func__, he->map->unmap_ip(he->map, ip));
 
 	if (offset >= sym_size)
-		return;
+		return 0;
 
 	h = priv->hist;
 	h->sum++;
@@ -108,18 +96,31 @@ static void hist_hit(struct hist_entry *he, u64 ip)
 
 	pr_debug3("%#Lx %s: count++ [ip: %#Lx, %#Lx] => %Ld\n", he->sym->start,
 		  he->sym->name, ip, ip - he->sym->start, h->ip[offset]);
+	return 0;
 }
 
 static int perf_session__add_hist_entry(struct perf_session *self,
 					struct addr_location *al, u64 count)
 {
 	bool hit;
-	struct hist_entry *he = __perf_session__add_hist_entry(self, al, NULL,
-							       count, &hit);
+	struct hist_entry *he;
+
+	if (sym_hist_filter != NULL &&
+	    (al->sym == NULL || strcmp(sym_hist_filter, al->sym->name) != 0)) {
+		/* We're only interested in a symbol named sym_hist_filter */
+		if (al->sym != NULL) {
+			rb_erase(&al->sym->rb_node,
+				 &al->map->dso->symbols[al->map->type]);
+			symbol__delete(al->sym);
+		}
+		return 0;
+	}
+
+	he = __perf_session__add_hist_entry(self, al, NULL, count, &hit);
 	if (he == NULL)
 		return -ENOMEM;
-	hist_hit(he, al->addr);
-	return 0;
+
+	return annotate__hist_hit(he, al->addr);
 }
 
 static int process_sample_event(event_t *event, struct perf_session *session)
@@ -129,7 +130,7 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 	dump_printf("(IP, %d): %d: %#Lx\n", event->header.misc,
 		    event->ip.pid, event->ip.ip);
 
-	if (event__preprocess_sample(event, session, &al, symbol_filter) < 0) {
+	if (event__preprocess_sample(event, session, &al, NULL) < 0) {
 		pr_warning("problem processing %d event, skipping it.\n",
 			   event->header.type);
 		return -1;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 7aab4e5..323c0ae 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -144,7 +144,7 @@ static struct symbol *symbol__new(u64 start, u64 len, const char *name)
 	return self;
 }
 
-static void symbol__delete(struct symbol *self)
+void symbol__delete(struct symbol *self)
 {
 	free(((void *)self) - symbol_conf.priv_size);
 }
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 1b4192e..280dadd 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -51,6 +51,8 @@ struct symbol {
 	char		name[0];
 };
 
+void symbol__delete(struct symbol *self);
+
 struct strlist;
 
 struct symbol_conf {
-- 
1.5.5.1

Re: perf per-symbol histogram causes memory corruption

[David Miller]

From: Arnaldo Carvalho de Melo <acme@infradead.org>
Date: Thu, 25 Feb 2010 12:57:40 -0300

> Em Thu, Feb 25, 2010 at 07:02:50AM -0800, David Miller escreveu:
>> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Date: Thu, 25 Feb 2010 11:13:51 -0300
>> 
>> > I.e. 'perf annotate' is not using this mechanism as a filter. Its a post
>> > processing phase.
>> 
>> Right, it's abusing the filter callback.
>> 
>> > I'll provide a patch.
>> 
>> Sounds good.
> 
> Can you try this one?

Works well so far, thanks!

Acked-by: David S. Miller <davem@davemloft.net>