perf per-symbol histogram causes memory corruption
in [Kernel]
|
Prev: [tip:core/rcu] rcu: Fix rcutorture mod_timer argument to delay one jiffy
Next: KVM: SVM: Implement emulation of vm_cr msr
From: David Miller on 25 Feb 2010 05:30 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 25 Feb 2010 06:20 (Added Arnaldo to the Cc: - he's maintaining the symbol lookup bits of perf.) * David Miller <davem(a)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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 25 Feb 2010 09:20 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(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 25 Feb 2010 10:10 From: Arnaldo Carvalho de Melo <acme(a)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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 25 Feb 2010 11:00
Em Thu, Feb 25, 2010 at 07:02:50AM -0800, David Miller escreveu: > From: Arnaldo Carvalho de Melo <acme(a)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(a)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(a)davemloft.net> Reported-by: Anton Blanchard <anton(a)samba.org> Cc: Fr�d�ric Weisbecker <fweisbec(a)gmail.com> Cc: Mike Galbraith <efault(a)gmx.de> Cc: Peter Zijlstra <a.p.zijlstra(a)chello.nl> Cc: Paul Mackerras <paulus(a)samba.org> Signed-off-by: Arnaldo Carvalho de Melo <acme(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |