* Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.
2016-10-05 11:45 ` callchain map refcounting fixes was " Arnaldo Carvalho de Melo
@ 2016-10-06 0:29 ` Masami Hiramatsu
2016-10-06 6:12 ` Krister Johansen
2016-10-07 2:22 ` Namhyung Kim
2 siblings, 0 replies; 25+ messages in thread
From: Masami Hiramatsu @ 2016-10-06 0:29 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Krister Johansen, Masami Hiramatsu, Namhyung Kim,
Frédéric Weisbecker, linux-kernel
On Wed, 5 Oct 2016 08:45:24 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > If dso__load_kcore frees all of the existing maps, but one has already
> > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > any function that happens to try to use this cursor with the invalid
> > map. Use the existing map refcount mechanism to forestall cleanup of a
> > map until the cursor iterates past the node.
>
> Seems ok, thanks for working on this! Can you provide a test case that
> causes the SEGV so that I can, in addition to reviewing your changes and
> auditing the code to check if all cases ara plugged, to reproduce the
> problem?
>
> Frédéric, Namhyung, Ack?
>
> Masami, is this a case that your refcount validator can catch?
Yes, I think so, it is able to be founded by refcnt debugger.
In case of getting SIGSEGV, we can enhance it to handle the
signal and dump traced data.
Thanks,
>
> - Arnaldo
>
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> > tools/perf/util/callchain.c | 12 ++++++++++--
> > tools/perf/util/callchain.h | 20 ++++++++++++++++++++
> > tools/perf/util/hist.c | 4 ++++
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 07fd30b..15c89b2 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
> > }
> > call->ip = cursor_node->ip;
> > call->ms.sym = cursor_node->sym;
> > - call->ms.map = cursor_node->map;
> > + call->ms.map = map__get(cursor_node->map);
> > list_add_tail(&call->list, &node->val);
> >
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >
> > list_for_each_entry_safe(call, tmp, &new->val, list) {
> > list_del(&call->list);
> > + map__zput(call->ms.map);
> > free(call);
> > }
> > free(new);
> > @@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> > callchain_cursor_append(cursor, list->ip,
> > list->ms.map, list->ms.sym);
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
> > }
> >
> > node->ip = ip;
> > - node->map = map;
> > + map__zput(node->map);
> > + node->map = map__get(map);
> > node->sym = sym;
> >
> > cursor->nr++;
> > @@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> > goto out;
> > }
> >
> > + map__get(al->map);
> > +
> > if (al->map->groups == &al->machine->kmaps) {
> > if (machine__is_host(al->machine)) {
> > al->cpumode = PERF_RECORD_MISC_KERNEL;
> > @@ -947,11 +952,13 @@ static void free_callchain_node(struct callchain_node *node)
> >
> > list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > list_for_each_entry_safe(list, tmp, &node->val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -1035,6 +1042,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> > out:
> > list_for_each_entry_safe(chain, new, &head, list) {
> > list_del(&chain->list);
> > + map__zput(chain->ms.map);
> > free(chain);
> > }
> > return -ENOMEM;
> > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > index 13e7554..0d944ef 100644
> > --- a/tools/perf/util/callchain.h
> > +++ b/tools/perf/util/callchain.h
> > @@ -5,6 +5,7 @@
> > #include <linux/list.h>
> > #include <linux/rbtree.h>
> > #include "event.h"
> > +#include "map.h"
> > #include "symbol.h"
> >
> > #define HELP_PAD "\t\t\t\t"
> > @@ -178,8 +179,13 @@ int callchain_merge(struct callchain_cursor *cursor,
> > */
> > static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > cursor->nr = 0;
> > cursor->last = &cursor->first;
> > +
> > + for (node = cursor->first; node != NULL; node = node->next)
> > + map__zput(node->map);
> > }
> >
> > int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
> > @@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
> > static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
> > struct callchain_cursor *src)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > *dest = *src;
> >
> > dest->first = src->curr;
> > dest->nr -= src->pos;
> > +
> > + for (node = dest->first; node != NULL; node = node->next)
> > + map__get(node->map);
> > }
> >
> > +static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
> > +{
> > + struct callchain_cursor_node *node;
> > +
> > + for (node = curs->first; node != NULL; node = node->next)
> > + map__put(node->map);
> > +}
> > +
> > +
> > #ifdef HAVE_SKIP_CALLCHAIN_IDX
> > int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
> > #else
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index b02992e..f8335e8 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1,6 +1,7 @@
> > #include "util.h"
> > #include "build-id.h"
> > #include "hist.h"
> > +#include "map.h"
> > #include "session.h"
> > #include "sort.h"
> > #include "evlist.h"
> > @@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> >
> > if (symbol_conf.use_callchain)
> > callchain_append(he->callchain, &cursor, sample->period);
> > + /* Cleanup temporary cursor. */
> > + callchain_cursor_snapshot_rele(&cursor);
> > return 0;
> > }
> >
> > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > {
> > zfree(&iter->priv);
> > iter->he = NULL;
> > + map__zput(al->map);
> >
> > return 0;
> > }
> > --
> > 2.7.4
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.
2016-10-05 11:45 ` callchain map refcounting fixes was " Arnaldo Carvalho de Melo
2016-10-06 0:29 ` Masami Hiramatsu
@ 2016-10-06 6:12 ` Krister Johansen
2016-10-07 2:22 ` Namhyung Kim
2 siblings, 0 replies; 25+ messages in thread
From: Krister Johansen @ 2016-10-06 6:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Krister Johansen, Masami Hiramatsu, Namhyung Kim,
Frédéric Weisbecker, linux-kernel
On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > If dso__load_kcore frees all of the existing maps, but one has already
> > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > any function that happens to try to use this cursor with the invalid
> > map. Use the existing map refcount mechanism to forestall cleanup of a
> > map until the cursor iterates past the node.
>
> Seems ok, thanks for working on this! Can you provide a test case that
> causes the SEGV so that I can, in addition to reviewing your changes and
> auditing the code to check if all cases ara plugged, to reproduce the
> problem?
Absolutely. Thanks for taking the time to look it over.
AFIACT, this occurs when you're probing a .ko with its own debug
information, but when the kernel has none. The kernel and all of the
in-tree modules were built through an RPM build that strips out
debuginfo into a separate package. On this particular system, the
kernel debuginfo packages were not installed. However, I had recently
changed a dkms build of an external module to use -g and to not strip.
We had one lonely .ko with all of its debug information inside, and a
kernel that perf was going to need to use kallsyms. I was able to
insert the kprobe into the module and record events. It was just script
and report that caused the core.
It should be possible to reproduce this by running script against a
trace that was recorded from kprobes in a module that has debug
inforation, but while running a kernel that does not. I didn't specify
any special options for lookup of vmlinux. I just let the tool figure
it out.
If you think it'd be useful, I can send along the notes that I took when
I debugged this. They're about 15k, which is why I would hesitate to
send it to the entire list unsolicited.
Thanks again,
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.
2016-10-05 11:45 ` callchain map refcounting fixes was " Arnaldo Carvalho de Melo
2016-10-06 0:29 ` Masami Hiramatsu
2016-10-06 6:12 ` Krister Johansen
@ 2016-10-07 2:22 ` Namhyung Kim
2016-10-09 6:13 ` Krister Johansen
2016-10-11 9:28 ` [PATCH v2 " Krister Johansen
2 siblings, 2 replies; 25+ messages in thread
From: Namhyung Kim @ 2016-10-07 2:22 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Krister Johansen, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
Hi Arnaldo and Krister,
On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > If dso__load_kcore frees all of the existing maps, but one has already
> > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > any function that happens to try to use this cursor with the invalid
> > map. Use the existing map refcount mechanism to forestall cleanup of a
> > map until the cursor iterates past the node.
>
> Seems ok, thanks for working on this! Can you provide a test case that
> causes the SEGV so that I can, in addition to reviewing your changes and
> auditing the code to check if all cases ara plugged, to reproduce the
> problem?
>
> Frédéric, Namhyung, Ack?
>
> Masami, is this a case that your refcount validator can catch?
>
> - Arnaldo
>
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> > tools/perf/util/callchain.c | 12 ++++++++++--
> > tools/perf/util/callchain.h | 20 ++++++++++++++++++++
> > tools/perf/util/hist.c | 4 ++++
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 07fd30b..15c89b2 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
> > }
> > call->ip = cursor_node->ip;
> > call->ms.sym = cursor_node->sym;
> > - call->ms.map = cursor_node->map;
> > + call->ms.map = map__get(cursor_node->map);
> > list_add_tail(&call->list, &node->val);
> >
> > callchain_cursor_advance(cursor);
> > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> >
> > list_for_each_entry_safe(call, tmp, &new->val, list) {
> > list_del(&call->list);
> > + map__zput(call->ms.map);
> > free(call);
> > }
> > free(new);
> > @@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> > callchain_cursor_append(cursor, list->ip,
> > list->ms.map, list->ms.sym);
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
> > }
> >
> > node->ip = ip;
> > - node->map = map;
> > + map__zput(node->map);
> > + node->map = map__get(map);
> > node->sym = sym;
> >
> > cursor->nr++;
> > @@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> > goto out;
> > }
> >
> > + map__get(al->map);
> > +
> > if (al->map->groups == &al->machine->kmaps) {
> > if (machine__is_host(al->machine)) {
> > al->cpumode = PERF_RECORD_MISC_KERNEL;
> > @@ -947,11 +952,13 @@ static void free_callchain_node(struct callchain_node *node)
> >
> > list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > list_for_each_entry_safe(list, tmp, &node->val, list) {
> > list_del(&list->list);
> > + map__zput(list->ms.map);
> > free(list);
> > }
> >
> > @@ -1035,6 +1042,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> > out:
> > list_for_each_entry_safe(chain, new, &head, list) {
> > list_del(&chain->list);
> > + map__zput(chain->ms.map);
I think you need to grab the refcnt in the "while (parent)" loop above.
> > free(chain);
> > }
> > return -ENOMEM;
> > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > index 13e7554..0d944ef 100644
> > --- a/tools/perf/util/callchain.h
> > +++ b/tools/perf/util/callchain.h
> > @@ -5,6 +5,7 @@
> > #include <linux/list.h>
> > #include <linux/rbtree.h>
> > #include "event.h"
> > +#include "map.h"
> > #include "symbol.h"
> >
> > #define HELP_PAD "\t\t\t\t"
> > @@ -178,8 +179,13 @@ int callchain_merge(struct callchain_cursor *cursor,
> > */
> > static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > cursor->nr = 0;
> > cursor->last = &cursor->first;
> > +
> > + for (node = cursor->first; node != NULL; node = node->next)
> > + map__zput(node->map);
> > }
> >
> > int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
> > @@ -238,12 +244,26 @@ int perf_callchain_config(const char *var, const char *value);
> > static inline void callchain_cursor_snapshot(struct callchain_cursor *dest,
> > struct callchain_cursor *src)
> > {
> > + struct callchain_cursor_node *node;
> > +
> > *dest = *src;
> >
> > dest->first = src->curr;
> > dest->nr -= src->pos;
> > +
> > + for (node = dest->first; node != NULL; node = node->next)
> > + map__get(node->map);
> > }
> >
> > +static inline void callchain_cursor_snapshot_rele(struct callchain_cursor *curs)
> > +{
> > + struct callchain_cursor_node *node;
> > +
> > + for (node = curs->first; node != NULL; node = node->next)
> > + map__put(node->map);
> > +}
> > +
> > +
> > #ifdef HAVE_SKIP_CALLCHAIN_IDX
> > int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain);
> > #else
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index b02992e..f8335e8 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1,6 +1,7 @@
> > #include "util.h"
> > #include "build-id.h"
> > #include "hist.h"
> > +#include "map.h"
> > #include "session.h"
> > #include "sort.h"
> > #include "evlist.h"
> > @@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> >
> > if (symbol_conf.use_callchain)
> > callchain_append(he->callchain, &cursor, sample->period);
> > + /* Cleanup temporary cursor. */
> > + callchain_cursor_snapshot_rele(&cursor);
This callchain shotshot is used in a short period of time, and it's
guaranteed that the maps in callchains will not freed due to refcnt in
the orignal callchain cursor. So I think we can skip to get/put
refcnt on the snapshot cursor. Also "rele" seems not a good name..
> > return 0;
> > }
> >
> > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > {
> > zfree(&iter->priv);
> > iter->he = NULL;
> > + map__zput(al->map);
What is this needed? Why other places like iter_finish_normal_entry
isn't?
> >
> > return 0;
> > }
> > --
> > 2.7.4
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.
2016-10-07 2:22 ` Namhyung Kim
@ 2016-10-09 6:13 ` Krister Johansen
2016-10-11 9:28 ` Krister Johansen
2016-10-11 9:28 ` [PATCH v2 " Krister Johansen
1 sibling, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2016-10-09 6:13 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
Hi Namhyung,
Thanks for looking this over.
On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > index 07fd30b..15c89b2 100644
> > > --- a/tools/perf/util/callchain.c
> > > +++ b/tools/perf/util/callchain.c
> > > @@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
> > > }
> > > call->ip = cursor_node->ip;
> > > call->ms.sym = cursor_node->sym;
> > > - call->ms.map = cursor_node->map;
> > > + call->ms.map = map__get(cursor_node->map);
> > > list_add_tail(&call->list, &node->val);
> > >
> > > callchain_cursor_advance(cursor);
> > > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> > >
> > > list_for_each_entry_safe(call, tmp, &new->val, list) {
> > > list_del(&call->list);
> > > + map__zput(call->ms.map);
> > > free(call);
> > > }
> > > free(new);
> > > @@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> > > callchain_cursor_append(cursor, list->ip,
> > > list->ms.map, list->ms.sym);
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > @@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
> > > }
> > >
> > > node->ip = ip;
> > > - node->map = map;
> > > + map__zput(node->map);
> > > + node->map = map__get(map);
> > > node->sym = sym;
> > >
> > > cursor->nr++;
> > > @@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> > > goto out;
> > > }
> > >
> > > + map__get(al->map);
> > > +
> > > if (al->map->groups == &al->machine->kmaps) {
> > > if (machine__is_host(al->machine)) {
> > > al->cpumode = PERF_RECORD_MISC_KERNEL;
> > > @@ -947,11 +952,13 @@ static void free_callchain_node(struct callchain_node *node)
> > >
> > > list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > list_for_each_entry_safe(list, tmp, &node->val, list) {
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > @@ -1035,6 +1042,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> > > out:
> > > list_for_each_entry_safe(chain, new, &head, list) {
> > > list_del(&chain->list);
> > > + map__zput(chain->ms.map);
>
> I think you need to grab the refcnt in the "while (parent)" loop above.
Thanks; good catch. I'll fix this.
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index b02992e..f8335e8 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -1,6 +1,7 @@
> > > #include "util.h"
> > > #include "build-id.h"
> > > #include "hist.h"
> > > +#include "map.h"
> > > #include "session.h"
> > > #include "sort.h"
> > > #include "evlist.h"
> > > @@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> > >
> > > if (symbol_conf.use_callchain)
> > > callchain_append(he->callchain, &cursor, sample->period);
> > > + /* Cleanup temporary cursor. */
> > > + callchain_cursor_snapshot_rele(&cursor);
>
> This callchain shotshot is used in a short period of time, and it's
> guaranteed that the maps in callchains will not freed due to refcnt in
> the orignal callchain cursor. So I think we can skip to get/put
> refcnt on the snapshot cursor. Also "rele" seems not a good name..
Ok. I'll remove the get/put from the snapshot stuff, and will excise the
rele function.
> > > return 0;
> > > }
> > >
> > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > > {
> > > zfree(&iter->priv);
> > > iter->he = NULL;
> > > + map__zput(al->map);
>
> What is this needed? Why other places like iter_finish_normal_entry
> isn't?
I put a map__zput() here because iter_next_cumulative_entry() calls
fill_callchain_info(), which takes a reference on the al->map in the
struct addr_location that it returns. I had forgotten all of this by
the time you asked. When I went back to work out my rationale, I
stumbled across addr_location__put(). Think it would make sense to
relocate the put of al->map there instead?
Thanks for the feedback. I'll send out a v2 once I get your changes
incorporated and tested.
-K
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash.
2016-10-09 6:13 ` Krister Johansen
@ 2016-10-11 9:28 ` Krister Johansen
0 siblings, 0 replies; 25+ messages in thread
From: Krister Johansen @ 2016-10-11 9:28 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
On Sat, Oct 08, 2016 at 11:13:21PM -0700, Krister Johansen wrote:
> On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> > On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > index b02992e..f8335e8 100644
> > > > --- a/tools/perf/util/hist.c
> > > > +++ b/tools/perf/util/hist.c
> > > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > > > {
> > > > zfree(&iter->priv);
> > > > iter->he = NULL;
> > > > + map__zput(al->map);
> >
> > What is this needed? Why other places like iter_finish_normal_entry
> > isn't?
>
> I put a map__zput() here because iter_next_cumulative_entry() calls
> fill_callchain_info(), which takes a reference on the al->map in the
> struct addr_location that it returns. I had forgotten all of this by
> the time you asked. When I went back to work out my rationale, I
> stumbled across addr_location__put(). Think it would make sense to
> relocate the put of al->map there instead?
I gave the addr_location__put() approach a try, but it caused me to
remember why I had kept this small. There are certain circumstances
where callers that lookup maps don't take references, seemingly because
the map is already contained within a map group. If I move this to
addr_location__put(), then I need to expand the scope of this change.
Right now, it's just focusing on making sure that any map that gets
embedded into a callchain cursor, or retrieved from one and held onto,
gets referenced.
In other words, moving this to addr_location__put() frees a bunch of
maps that are acquired from elsewhere without their reference counts
incremented.
I made the rest of the modifications we discussed. I'll send a v2 patch
in a separate e-mail.
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-10-07 2:22 ` Namhyung Kim
2016-10-09 6:13 ` Krister Johansen
@ 2016-10-11 9:28 ` Krister Johansen
2016-10-26 0:20 ` Krister Johansen
1 sibling, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2016-10-11 9:28 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
If dso__load_kcore frees all of the existing maps, but one has already
been attached to a callchain cursor node, then we can get a SIGSEGV in
any function that happens to try to use this invalid cursor. Use the
existing map refcount mechanism to forestall cleanup of a map until the
cursor iterates past the node.
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
tools/perf/util/callchain.c | 13 +++++++++++--
tools/perf/util/callchain.h | 6 ++++++
tools/perf/util/hist.c | 2 ++
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 07fd30b..002bf5d 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
}
call->ip = cursor_node->ip;
call->ms.sym = cursor_node->sym;
- call->ms.map = cursor_node->map;
+ call->ms.map = map__get(cursor_node->map);
list_add_tail(&call->list, &node->val);
callchain_cursor_advance(cursor);
@@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
list_for_each_entry_safe(call, tmp, &new->val, list) {
list_del(&call->list);
+ map__zput(call->ms.map);
free(call);
}
free(new);
@@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
callchain_cursor_append(cursor, list->ip,
list->ms.map, list->ms.sym);
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
}
node->ip = ip;
- node->map = map;
+ map__zput(node->map);
+ node->map = map__get(map);
node->sym = sym;
cursor->nr++;
@@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
goto out;
}
+ map__get(al->map);
+
if (al->map->groups == &al->machine->kmaps) {
if (machine__is_host(al->machine)) {
al->cpumode = PERF_RECORD_MISC_KERNEL;
@@ -947,11 +952,13 @@ static void free_callchain_node(struct callchain_node *node)
list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
list_for_each_entry_safe(list, tmp, &node->val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -1015,6 +1022,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
goto out;
*new = *chain;
new->has_children = false;
+ map__get(new->ms.map);
list_add_tail(&new->list, &head);
}
parent = parent->parent;
@@ -1035,6 +1043,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
out:
list_for_each_entry_safe(chain, new, &head, list) {
list_del(&chain->list);
+ map__zput(chain->ms.map);
free(chain);
}
return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 13e7554..798ba81 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/rbtree.h>
#include "event.h"
+#include "map.h"
#include "symbol.h"
#define HELP_PAD "\t\t\t\t"
@@ -178,8 +179,13 @@ int callchain_merge(struct callchain_cursor *cursor,
*/
static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
{
+ struct callchain_cursor_node *node;
+
cursor->nr = 0;
cursor->last = &cursor->first;
+
+ for (node = cursor->first; node != NULL; node = node->next)
+ map__zput(node->map);
}
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b02992e..609b365 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
{
zfree(&iter->priv);
iter->he = NULL;
+ map__zput(al->map);
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-10-11 9:28 ` [PATCH v2 " Krister Johansen
@ 2016-10-26 0:20 ` Krister Johansen
2016-10-26 13:44 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2016-10-26 0:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
On Tue, Oct 11, 2016 at 02:28:39AM -0700, Krister Johansen wrote:
> If dso__load_kcore frees all of the existing maps, but one has already
> been attached to a callchain cursor node, then we can get a SIGSEGV in
> any function that happens to try to use this invalid cursor. Use the
> existing map refcount mechanism to forestall cleanup of a map until the
> cursor iterates past the node.
It has been a couple of weeks since I sent out v2 of this patch. I
understand that folks here have plenty of irons in the fire, but I
wanted to double-check that nobody was waiting on me for additional
information or changes.
Thanks,
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-10-26 0:20 ` Krister Johansen
@ 2016-10-26 13:44 ` Arnaldo Carvalho de Melo
2016-11-11 0:40 ` Krister Johansen
0 siblings, 1 reply; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-26 13:44 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:
> On Tue, Oct 11, 2016 at 02:28:39AM -0700, Krister Johansen wrote:
> > If dso__load_kcore frees all of the existing maps, but one has already
> > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > any function that happens to try to use this invalid cursor. Use the
> > existing map refcount mechanism to forestall cleanup of a map until the
> > cursor iterates past the node.
>
> It has been a couple of weeks since I sent out v2 of this patch. I
> understand that folks here have plenty of irons in the fire, but I
> wanted to double-check that nobody was waiting on me for additional
> information or changes.
It was a mix of waiting for more people to review it, or for Masami to
run its refcount debugger on it, ended up falling thru the cracks.
I'll try to process it now.
- Arnaldo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-10-26 13:44 ` Arnaldo Carvalho de Melo
@ 2016-11-11 0:40 ` Krister Johansen
2016-11-22 19:01 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2016-11-11 0:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
On Wed, Oct 26, 2016 at 11:44:53AM -0200, Arnaldo Carvalho de Melo wrote:
> Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:
> > On Tue, Oct 11, 2016 at 02:28:39AM -0700, Krister Johansen wrote:
> > > If dso__load_kcore frees all of the existing maps, but one has already
> > > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > > any function that happens to try to use this invalid cursor. Use the
> > > existing map refcount mechanism to forestall cleanup of a map until the
> > > cursor iterates past the node.
> >
> > It has been a couple of weeks since I sent out v2 of this patch. I
> > understand that folks here have plenty of irons in the fire, but I
> > wanted to double-check that nobody was waiting on me for additional
> > information or changes.
>
> It was a mix of waiting for more people to review it, or for Masami to
> run its refcount debugger on it, ended up falling thru the cracks.
>
> I'll try to process it now.
Thanks. As part of processing this did you run into any problems?
Would you like me to rebase against the latest perf/core and re-send the
patch?
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-11-11 0:40 ` Krister Johansen
@ 2016-11-22 19:01 ` Arnaldo Carvalho de Melo
2016-12-02 7:12 ` Krister Johansen
2016-12-29 1:39 ` Krister Johansen
0 siblings, 2 replies; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-11-22 19:01 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Thu, Nov 10, 2016 at 04:40:46PM -0800, Krister Johansen escreveu:
> On Wed, Oct 26, 2016 at 11:44:53AM -0200, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Oct 25, 2016 at 05:20:10PM -0700, Krister Johansen escreveu:
> > > On Tue, Oct 11, 2016 at 02:28:39AM -0700, Krister Johansen wrote:
> > > > If dso__load_kcore frees all of the existing maps, but one has already
> > > > been attached to a callchain cursor node, then we can get a SIGSEGV in
> > > > any function that happens to try to use this invalid cursor. Use the
> > > > existing map refcount mechanism to forestall cleanup of a map until the
> > > > cursor iterates past the node.
> > >
> > > It has been a couple of weeks since I sent out v2 of this patch. I
> > > understand that folks here have plenty of irons in the fire, but I
> > > wanted to double-check that nobody was waiting on me for additional
> > > information or changes.
> >
> > It was a mix of waiting for more people to review it, or for Masami to
> > run its refcount debugger on it, ended up falling thru the cracks.
> >
> > I'll try to process it now.
>
> Thanks. As part of processing this did you run into any problems?
> Would you like me to rebase against the latest perf/core and re-send the
> patch?
Sorry for the overly long delay, trying it now after fixing up a
conflict with a recent patchkit (branch stuff) I tested it by running
'perf top -g' and I'm getting some assertion bugs:
# perf top -g
1.34% filemap_map_pages
- 0.59% alloc_pages_vma
1.20% __alloc_pages_nodemask
- 5.87% 0.45% [kernel] [k] handle_mm_fault
- 1.94% handle_mm_fault
1.34% filemap_map_pages
- 0.59% alloc_pages_vma
1.22% __alloc_pages_nodemask
+ 5.75% 0.03% perf [.] hist_entry_iter__add
+ 4.46% 0.00% [unknown] [.] 0000000000000000
- 4.06% 2.74% libc-2.23.so [.] _int_malloc
- 1.95% 0
1.94% _int_malloc
- 3.20% 0.23% perf [.] iter_add_next_cumulative_entry
- 1.49% iter_add_next_cumulative_entry
- 1.43% __hists__add_entry
2.58% 0.01% [kernel] [k] return_from_SYSCALL_64
2.57% 2.55% libperl.so.5.22.2 [.] Perl_fbm_instr
- 2.54% 2.51% liblzma.so.5.2.2 [.] lzma_decode
- 2.51% lzma_decode
2.33% 0.00% ld-2.23.so [.] _dl_sysdep_start
+ 2.24% 0.04% ld-2.23.so [.] dl_main
2.13% 0.03% [kernel] [k] ext4_readdir
2.09% 0.01% [kernel] [k] sys_newstat
2.08% 0.04% [kernel] [k] vfs_fstatat
2.07% 0.02% [kernel] [k] SYSC_newstat
2.02% 0.01% [kernel] [k] iterate_dir
- 1.96% 0.17% [kernel] [k] __alloc_pages_nodemask
- 1.37% __alloc_pages_nodemask
perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
Aborted (core dumped)
[root@jouet ~]#
I'll try to investigate this further later/tomorrow, find the updated patch below.
- Arnaldo
commit af04d2c4a5d1f6bd7f4971118e4e1153cc7c2506
Author: Krister Johansen <kjlx@templeofstupid.com>
Date: Tue Oct 11 02:28:39 2016 -0700
perf callchain: Fix a use after free crash due to refcounting bug
If dso__load_kcore frees all of the existing maps, but one has already
been attached to a callchain cursor node, then we can get a SIGSEGV in
any function that happens to try to use this invalid cursor. Use the
existing map refcount mechanism to forestall cleanup of a map until the
cursor iterates past the node.
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20161011092839.GC7837@templeofstupid.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 823befd8209a..18bb7caee535 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -437,7 +437,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
}
call->ip = cursor_node->ip;
call->ms.sym = cursor_node->sym;
- call->ms.map = cursor_node->map;
+ call->ms.map = map__get(cursor_node->map);
if (cursor_node->branch) {
call->branch_count = 1;
@@ -477,6 +477,7 @@ add_child(struct callchain_node *parent,
list_for_each_entry_safe(call, tmp, &new->val, list) {
list_del(&call->list);
+ map__zput(call->ms.map);
free(call);
}
free(new);
@@ -761,6 +762,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
list->ms.map, list->ms.sym,
false, NULL, 0, 0);
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -811,7 +813,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
}
node->ip = ip;
- node->map = map;
+ map__zput(node->map);
+ node->map = map__get(map);
node->sym = sym;
node->branch = branch;
node->nr_loop_iter = nr_loop_iter;
@@ -868,6 +871,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
goto out;
}
+ map__get(al->map);
+
if (al->map->groups == &al->machine->kmaps) {
if (machine__is_host(al->machine)) {
al->cpumode = PERF_RECORD_MISC_KERNEL;
@@ -1142,11 +1147,13 @@ static void free_callchain_node(struct callchain_node *node)
list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
list_for_each_entry_safe(list, tmp, &node->val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -1210,6 +1217,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
goto out;
*new = *chain;
new->has_children = false;
+ map__get(new->ms.map);
list_add_tail(&new->list, &head);
}
parent = parent->parent;
@@ -1230,6 +1238,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
out:
list_for_each_entry_safe(chain, new, &head, list) {
list_del(&chain->list);
+ map__zput(chain->ms.map);
free(chain);
}
return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index d9c70dccf06a..f551fd2cfe5a 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/rbtree.h>
#include "event.h"
+#include "map.h"
#include "symbol.h"
#define HELP_PAD "\t\t\t\t"
@@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
*/
static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
{
+ struct callchain_cursor_node *node;
+
cursor->nr = 0;
cursor->last = &cursor->first;
+
+ for (node = cursor->first; node != NULL; node = node->next)
+ map__zput(node->map);
}
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index e1be4132054d..be4b07145705 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
{
zfree(&iter->priv);
iter->he = NULL;
+ map__zput(al->map);
return 0;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-11-22 19:01 ` Arnaldo Carvalho de Melo
@ 2016-12-02 7:12 ` Krister Johansen
2016-12-29 1:39 ` Krister Johansen
1 sibling, 0 replies; 25+ messages in thread
From: Krister Johansen @ 2016-12-02 7:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Krister Johansen, Namhyung Kim, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
Hey Arnaldo,
On Tue, Nov 22, 2016 at 04:01:06PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 10, 2016 at 04:40:46PM -0800, Krister Johansen escreveu:
> > Thanks. As part of processing this did you run into any problems?
> > Would you like me to rebase against the latest perf/core and re-send the
> > patch?
>
> Sorry for the overly long delay, trying it now after fixing up a
> conflict with a recent patchkit (branch stuff) I tested it by running
> 'perf top -g' and I'm getting some assertion bugs:
I appreciate you taking another stab at pulling this in. My turn to
apologize for the delay.
> # perf top -g
> 1.34% filemap_map_pages
> - 0.59% alloc_pages_vma
> 1.20% __alloc_pages_nodemask
> - 5.87% 0.45% [kernel] [k] handle_mm_fault
> - 1.94% handle_mm_fault
> 1.34% filemap_map_pages
> - 0.59% alloc_pages_vma
> 1.22% __alloc_pages_nodemask
> + 5.75% 0.03% perf [.] hist_entry_iter__add
> + 4.46% 0.00% [unknown] [.] 0000000000000000
> - 4.06% 2.74% libc-2.23.so [.] _int_malloc
> - 1.95% 0
> 1.94% _int_malloc
> - 3.20% 0.23% perf [.] iter_add_next_cumulative_entry
> - 1.49% iter_add_next_cumulative_entry
> - 1.43% __hists__add_entry
> 2.58% 0.01% [kernel] [k] return_from_SYSCALL_64
> 2.57% 2.55% libperl.so.5.22.2 [.] Perl_fbm_instr
> - 2.54% 2.51% liblzma.so.5.2.2 [.] lzma_decode
> - 2.51% lzma_decode
> 2.33% 0.00% ld-2.23.so [.] _dl_sysdep_start
> + 2.24% 0.04% ld-2.23.so [.] dl_main
> 2.13% 0.03% [kernel] [k] ext4_readdir
> 2.09% 0.01% [kernel] [k] sys_newstat
> 2.08% 0.04% [kernel] [k] vfs_fstatat
> 2.07% 0.02% [kernel] [k] SYSC_newstat
> 2.02% 0.01% [kernel] [k] iterate_dir
> - 1.96% 0.17% [kernel] [k] __alloc_pages_nodemask
> - 1.37% __alloc_pages_nodemask
> perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
Assuming that I'd failed to test 'perf top -g' I went ahead and re-ran
this with the last version of the patch I sent out parented against the
4.8 STABLE branch. That didn't trigger any assertion failures for me.
Is this branch that gave you merge conflicts now in perf/core or
otherwise publicly avilable? If so, I'd be happy to try to resolve any
conflicts and re-test against it. The copy of the patch you sent out
didn't look obviously incorrect.
Thanks,
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-11-22 19:01 ` Arnaldo Carvalho de Melo
2016-12-02 7:12 ` Krister Johansen
@ 2016-12-29 1:39 ` Krister Johansen
2017-01-02 15:15 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2016-12-29 1:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
On Tue, Nov 22, 2016 at 04:01:06PM -0300, Arnaldo Carvalho de Melo wrote:
> Sorry for the overly long delay, trying it now after fixing up a
> conflict with a recent patchkit (branch stuff) I tested it by running
> 'perf top -g' and I'm getting some assertion bugs:
>
>
> # perf top -g
> 1.34% filemap_map_pages
> - 0.59% alloc_pages_vma
> 1.20% __alloc_pages_nodemask
> - 5.87% 0.45% [kernel] [k] handle_mm_fault
> - 1.94% handle_mm_fault
> 1.34% filemap_map_pages
> - 0.59% alloc_pages_vma
> 1.22% __alloc_pages_nodemask
> + 5.75% 0.03% perf [.] hist_entry_iter__add
> + 4.46% 0.00% [unknown] [.] 0000000000000000
> - 4.06% 2.74% libc-2.23.so [.] _int_malloc
> - 1.95% 0
> 1.94% _int_malloc
> - 3.20% 0.23% perf [.] iter_add_next_cumulative_entry
> - 1.49% iter_add_next_cumulative_entry
> - 1.43% __hists__add_entry
> 2.58% 0.01% [kernel] [k] return_from_SYSCALL_64
> 2.57% 2.55% libperl.so.5.22.2 [.] Perl_fbm_instr
> - 2.54% 2.51% liblzma.so.5.2.2 [.] lzma_decode
> - 2.51% lzma_decode
> 2.33% 0.00% ld-2.23.so [.] _dl_sysdep_start
> + 2.24% 0.04% ld-2.23.so [.] dl_main
> 2.13% 0.03% [kernel] [k] ext4_readdir
> 2.09% 0.01% [kernel] [k] sys_newstat
> 2.08% 0.04% [kernel] [k] vfs_fstatat
> 2.07% 0.02% [kernel] [k] SYSC_newstat
> 2.02% 0.01% [kernel] [k] iterate_dir
> - 1.96% 0.17% [kernel] [k] __alloc_pages_nodemask
> - 1.37% __alloc_pages_nodemask
> perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
> Aborted (core dumped)
> [root@jouet ~]#
>
>
> I'll try to investigate this further later/tomorrow, find the updated patch below.
>
> - Arnaldo
>
> commit af04d2c4a5d1f6bd7f4971118e4e1153cc7c2506
> Author: Krister Johansen <kjlx@templeofstupid.com>
> Date: Tue Oct 11 02:28:39 2016 -0700
>
> perf callchain: Fix a use after free crash due to refcounting bug
>
> If dso__load_kcore frees all of the existing maps, but one has already
> been attached to a callchain cursor node, then we can get a SIGSEGV in
> any function that happens to try to use this invalid cursor. Use the
> existing map refcount mechanism to forestall cleanup of a map until the
> cursor iterates past the node.
>
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Link: http://lkml.kernel.org/r/20161011092839.GC7837@templeofstupid.com
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 823befd8209a..18bb7caee535 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -437,7 +437,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
> }
> call->ip = cursor_node->ip;
> call->ms.sym = cursor_node->sym;
> - call->ms.map = cursor_node->map;
> + call->ms.map = map__get(cursor_node->map);
>
> if (cursor_node->branch) {
> call->branch_count = 1;
> @@ -477,6 +477,7 @@ add_child(struct callchain_node *parent,
>
> list_for_each_entry_safe(call, tmp, &new->val, list) {
> list_del(&call->list);
> + map__zput(call->ms.map);
> free(call);
> }
> free(new);
> @@ -761,6 +762,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> list->ms.map, list->ms.sym,
> false, NULL, 0, 0);
> list_del(&list->list);
> + map__zput(list->ms.map);
> free(list);
> }
>
> @@ -811,7 +813,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
> }
>
> node->ip = ip;
> - node->map = map;
> + map__zput(node->map);
> + node->map = map__get(map);
> node->sym = sym;
> node->branch = branch;
> node->nr_loop_iter = nr_loop_iter;
> @@ -868,6 +871,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> goto out;
> }
>
> + map__get(al->map);
> +
> if (al->map->groups == &al->machine->kmaps) {
> if (machine__is_host(al->machine)) {
> al->cpumode = PERF_RECORD_MISC_KERNEL;
> @@ -1142,11 +1147,13 @@ static void free_callchain_node(struct callchain_node *node)
>
> list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> list_del(&list->list);
> + map__zput(list->ms.map);
> free(list);
> }
>
> list_for_each_entry_safe(list, tmp, &node->val, list) {
> list_del(&list->list);
> + map__zput(list->ms.map);
> free(list);
> }
>
> @@ -1210,6 +1217,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> goto out;
> *new = *chain;
> new->has_children = false;
> + map__get(new->ms.map);
> list_add_tail(&new->list, &head);
> }
> parent = parent->parent;
> @@ -1230,6 +1238,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
> out:
> list_for_each_entry_safe(chain, new, &head, list) {
> list_del(&chain->list);
> + map__zput(chain->ms.map);
> free(chain);
> }
> return -ENOMEM;
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index d9c70dccf06a..f551fd2cfe5a 100644
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -5,6 +5,7 @@
> #include <linux/list.h>
> #include <linux/rbtree.h>
> #include "event.h"
> +#include "map.h"
> #include "symbol.h"
>
> #define HELP_PAD "\t\t\t\t"
> @@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
> */
> static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
> {
> + struct callchain_cursor_node *node;
> +
> cursor->nr = 0;
> cursor->last = &cursor->first;
> +
> + for (node = cursor->first; node != NULL; node = node->next)
> + map__zput(node->map);
> }
>
> int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index e1be4132054d..be4b07145705 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1,6 +1,7 @@
> #include "util.h"
> #include "build-id.h"
> #include "hist.h"
> +#include "map.h"
> #include "session.h"
> #include "sort.h"
> #include "evlist.h"
> @@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> {
> zfree(&iter->priv);
> iter->he = NULL;
> + map__zput(al->map);
>
> return 0;
> }
As part of trying to tie up the year-end loose-ends, I went back and
re-tested a rebase'd version of this patch against perf/core. I ended
up with a merge that's identical to yours, except that I'm not seeing
any assertion failures with 'perf top -g', 'perf script', or 'perf
report'. Was perf/core the branch that was giving you trouble?
-K
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2016-12-29 1:39 ` Krister Johansen
@ 2017-01-02 15:15 ` Arnaldo Carvalho de Melo
2017-01-02 17:35 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-01-02 15:15 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Wed, Dec 28, 2016 at 05:39:47PM -0800, Krister Johansen escreveu:
> On Tue, Nov 22, 2016 at 04:01:06PM -0300, Arnaldo Carvalho de Melo wrote:
> > #include "evlist.h"
> > @@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> > {
> > zfree(&iter->priv);
> > iter->he = NULL;
> > + map__zput(al->map);
> As part of trying to tie up the year-end loose-ends, I went back and
> re-tested a rebase'd version of this patch against perf/core. I ended
> up with a merge that's identical to yours, except that I'm not seeing
> any assertion failures with 'perf top -g', 'perf script', or 'perf
> report'. Was perf/core the branch that was giving you trouble?
Yeah, I just tested it with my tip/perf/core and got this:
0.00% 0.00% [kernel] [k] file_free_rcu
0.00% 0.00% [kernel] [k] timerqueue_del
0.00% 0.00% [kernel] [k] irq_work_run
0.00% 0.00% [kernel] [k] native_irq_return_iret
0.00% 0.00% [kernel] [k] native_sched_clock
perf: util/map.c:246: map__exit: Assertion
`!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
Aborted
(core dumped)
[root@jouet 3.4]#
Tried it again with what is in Linus' tree + your patch and got the same
problem:
[acme@jouet linux]$ git remote -v | grep torvalds.*fetch
torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
[acme@jouet linux]$ git checkout -b test-branch torvalds/master
Branch test-branch set up to track remote branch master from torvalds.
Switched to a new branch 'test-branch'
[acme@jouet linux]$ git cherry-pick f7347a33099dbad7e9fb3c22cea211f238bfd320
[test-branch 7d786f548b62] perf callchain: Fix a use after free crash due to refcounting bug
Author: Krister Johansen <kjlx@templeofstupid.com>
Date: Mon Jan 2 12:06:55 2017 -0300
3 files changed, 19 insertions(+), 2 deletions(-)
[acme@jouet linux]$ rm -rf /tmp/build/perf/ ; mkdir -p /tmp/build/perf ; make O=/tmp/build/perf -C tools/perf install-bin
make: Entering directory '/home/acme/git/linux/tools/perf'
BUILD: Doing 'make -j4' parallel build
HOSTCC /tmp/build/perf/fixdep.o
<SNIP>
Then I run it with a higher frequency and no delay in refreshing the screen, to
stress the refcounting code:
# perf top -F 10000 -g -d 0
Do it while running something like 'make -j32 allmodconfig' to create lots of
short lived processes (or use stress-ng, etc).
+ 0.79% 0.00% [kernel] [k] search_binary_handler
+ 0.79% 0.00% [kernel] [k] do_execveat_common.isra.37
+ 0.79% 0.00% [kernel] [k] sys_execve
+ 0.79% 0.00% [kernel] [k] do_syscall_64
perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
Aborted (core dumped)
[root@jouet 3.4]#
- Arnaldo
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-02 15:15 ` Arnaldo Carvalho de Melo
@ 2017-01-02 17:35 ` Arnaldo Carvalho de Melo
2017-01-02 17:36 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-01-02 17:35 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> Tried it again with what is in Linus' tree + your patch and got the same
> problem:
>
> [acme@jouet linux]$ git remote -v | grep torvalds.*fetch
> torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
> [acme@jouet linux]$ git checkout -b test-branch torvalds/master
> Branch test-branch set up to track remote branch master from torvalds.
> Switched to a new branch 'test-branch'
> [acme@jouet linux]$ git cherry-pick f7347a33099dbad7e9fb3c22cea211f238bfd320
> [test-branch 7d786f548b62] perf callchain: Fix a use after free crash due to refcounting bug
> Author: Krister Johansen <kjlx@templeofstupid.com>
> Date: Mon Jan 2 12:06:55 2017 -0300
> 3 files changed, 19 insertions(+), 2 deletions(-)
> [acme@jouet linux]$ rm -rf /tmp/build/perf/ ; mkdir -p /tmp/build/perf ; make O=/tmp/build/perf -C tools/perf install-bin
> make: Entering directory '/home/acme/git/linux/tools/perf'
> BUILD: Doing 'make -j4' parallel build
> HOSTCC /tmp/build/perf/fixdep.o
> <SNIP>
>
> Then I run it with a higher frequency and no delay in refreshing the screen, to
> stress the refcounting code:
>
> # perf top -F 10000 -g -d 0
>
> Do it while running something like 'make -j32 allmodconfig' to create lots of
> short lived processes (or use stress-ng, etc).
Back to acme/perf/core and with debugging, we're getting a refcount hitting zero
while the map is still in a rbtree:
perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
Thread 1 "perf" received signal SIGABRT, Aborted.
0x00007ffff522691f in raise () from /lib64/libc.so.6
(gdb) bt
#0 0x00007ffff522691f in raise () from /lib64/libc.so.6
#1 0x00007ffff522851a in abort () from /lib64/libc.so.6
#2 0x00007ffff521eda7 in __assert_fail_base () from /lib64/libc.so.6
#3 0x00007ffff521ee52 in __assert_fail () from /lib64/libc.so.6
#4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
#5 0x0000000000504ea5 in map__delete (map=0x2393790) at util/map.c:252
#6 0x0000000000504f0a in map__put (map=0x2393790) at util/map.c:259
#7 0x000000000052fa01 in __map__zput (map=0x7fffffff8230) at util/map.h:161
#8 0x000000000053295b in iter_finish_cumulative_entry (iter=0x7fffffff8260, al=0x7fffffff8220) at util/hist.c:983
#9 0x0000000000532b53 in hist_entry_iter__add (iter=0x7fffffff8260, al=0x7fffffff8220, max_stack_depth=127, arg=0x7fffffffa7b0) at util/hist.c:1059
#10 0x000000000044f5cf in perf_event__process_sample (tool=0x7fffffffa7b0, event=0x7ffff7e24578, evsel=0x21515d0, sample=0x7fffffff8410, machine=0x21b2bf8)
at builtin-top.c:774
#11 0x000000000044f8ee in perf_top__mmap_read_idx (top=0x7fffffffa7b0, idx=2) at builtin-top.c:840
#12 0x000000000044fa0d in perf_top__mmap_read (top=0x7fffffffa7b0) at builtin-top.c:857
#13 0x0000000000450080 in __cmd_top (top=0x7fffffffa7b0) at builtin-top.c:1002
#14 0x00000000004514e0 in cmd_top (argc=0, argv=0x7fffffffe130, prefix=0x0) at builtin-top.c:1330
#15 0x00000000004b5af5 in run_builtin (p=0xa0baf8 <commands+312>, argc=6, argv=0x7fffffffe130) at perf.c:358
#16 0x00000000004b5d62 in handle_internal_command (argc=6, argv=0x7fffffffe130) at perf.c:420
#17 0x00000000004b5ea7 in run_argv (argcp=0x7fffffffdf8c, argv=0x7fffffffdf80) at perf.c:466
#18 0x00000000004b6290 in main (argc=6, argv=0x7fffffffe130) at perf.c:610
(gdb) fr 4
#4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
246 BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
(gdb) p map
$1 = (struct map *) 0x2393790
(gdb) p *map
$2 = {{rb_node = {__rb_parent_color = 37304353, rb_right = 0x0, rb_left = 0x0}, node = {next = 0x2393821, prev = 0x0}}, start = 140434683187200,
end = 140434690723840, type = 0 '\000', erange_warned = false, priv = 0, prot = 5, flags = 2, pgoff = 0, reloc = 0, maj = 253, min = 0, ino = 132875,
ino_generation = 3472328296227680304, map_ip = 0x504125 <map__map_ip>, unmap_ip = 0x504174 <map__unmap_ip>, dso = 0x22b3890, groups = 0x2385290, refcnt = {
counter = 0}}
(gdb)
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-02 17:35 ` Arnaldo Carvalho de Melo
@ 2017-01-02 17:36 ` Arnaldo Carvalho de Melo
2017-01-02 19:39 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-01-02 17:36 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Tried it again with what is in Linus' tree + your patch and got the same
> > problem:
> >
> > [acme@jouet linux]$ git remote -v | grep torvalds.*fetch
> > torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
> > [acme@jouet linux]$ git checkout -b test-branch torvalds/master
> > Branch test-branch set up to track remote branch master from torvalds.
> > Switched to a new branch 'test-branch'
> > [acme@jouet linux]$ git cherry-pick f7347a33099dbad7e9fb3c22cea211f238bfd320
> > [test-branch 7d786f548b62] perf callchain: Fix a use after free crash due to refcounting bug
> > Author: Krister Johansen <kjlx@templeofstupid.com>
> > Date: Mon Jan 2 12:06:55 2017 -0300
> > 3 files changed, 19 insertions(+), 2 deletions(-)
> > [acme@jouet linux]$ rm -rf /tmp/build/perf/ ; mkdir -p /tmp/build/perf ; make O=/tmp/build/perf -C tools/perf install-bin
> > make: Entering directory '/home/acme/git/linux/tools/perf'
> > BUILD: Doing 'make -j4' parallel build
> > HOSTCC /tmp/build/perf/fixdep.o
> > <SNIP>
> >
> > Then I run it with a higher frequency and no delay in refreshing the screen, to
> > stress the refcounting code:
> >
> > # perf top -F 10000 -g -d 0
> >
> > Do it while running something like 'make -j32 allmodconfig' to create lots of
> > short lived processes (or use stress-ng, etc).
>
> Back to acme/perf/core and with debugging, we're getting a refcount hitting zero
> while the map is still in a rbtree:
And this was introduced by your patch:
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6770a9645609..72f5c82798e9 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -979,6 +980,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
{
zfree(&iter->priv);
iter->he = NULL;
+ map__zput(al->map);
return 0;
}
> perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
>
> Thread 1 "perf" received signal SIGABRT, Aborted.
> 0x00007ffff522691f in raise () from /lib64/libc.so.6
> (gdb) bt
> #0 0x00007ffff522691f in raise () from /lib64/libc.so.6
> #1 0x00007ffff522851a in abort () from /lib64/libc.so.6
> #2 0x00007ffff521eda7 in __assert_fail_base () from /lib64/libc.so.6
> #3 0x00007ffff521ee52 in __assert_fail () from /lib64/libc.so.6
> #4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
> #5 0x0000000000504ea5 in map__delete (map=0x2393790) at util/map.c:252
> #6 0x0000000000504f0a in map__put (map=0x2393790) at util/map.c:259
> #7 0x000000000052fa01 in __map__zput (map=0x7fffffff8230) at util/map.h:161
> #8 0x000000000053295b in iter_finish_cumulative_entry (iter=0x7fffffff8260, al=0x7fffffff8220) at util/hist.c:983
> #9 0x0000000000532b53 in hist_entry_iter__add (iter=0x7fffffff8260, al=0x7fffffff8220, max_stack_depth=127, arg=0x7fffffffa7b0) at util/hist.c:1059
> #10 0x000000000044f5cf in perf_event__process_sample (tool=0x7fffffffa7b0, event=0x7ffff7e24578, evsel=0x21515d0, sample=0x7fffffff8410, machine=0x21b2bf8)
> at builtin-top.c:774
> #11 0x000000000044f8ee in perf_top__mmap_read_idx (top=0x7fffffffa7b0, idx=2) at builtin-top.c:840
> #12 0x000000000044fa0d in perf_top__mmap_read (top=0x7fffffffa7b0) at builtin-top.c:857
> #13 0x0000000000450080 in __cmd_top (top=0x7fffffffa7b0) at builtin-top.c:1002
> #14 0x00000000004514e0 in cmd_top (argc=0, argv=0x7fffffffe130, prefix=0x0) at builtin-top.c:1330
> #15 0x00000000004b5af5 in run_builtin (p=0xa0baf8 <commands+312>, argc=6, argv=0x7fffffffe130) at perf.c:358
> #16 0x00000000004b5d62 in handle_internal_command (argc=6, argv=0x7fffffffe130) at perf.c:420
> #17 0x00000000004b5ea7 in run_argv (argcp=0x7fffffffdf8c, argv=0x7fffffffdf80) at perf.c:466
> #18 0x00000000004b6290 in main (argc=6, argv=0x7fffffffe130) at perf.c:610
> (gdb) fr 4
> #4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
> 246 BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
> (gdb) p map
> $1 = (struct map *) 0x2393790
> (gdb) p *map
> $2 = {{rb_node = {__rb_parent_color = 37304353, rb_right = 0x0, rb_left = 0x0}, node = {next = 0x2393821, prev = 0x0}}, start = 140434683187200,
> end = 140434690723840, type = 0 '\000', erange_warned = false, priv = 0, prot = 5, flags = 2, pgoff = 0, reloc = 0, maj = 253, min = 0, ino = 132875,
> ino_generation = 3472328296227680304, map_ip = 0x504125 <map__map_ip>, unmap_ip = 0x504174 <map__unmap_ip>, dso = 0x22b3890, groups = 0x2385290, refcnt = {
> counter = 0}}
> (gdb)
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-02 17:36 ` Arnaldo Carvalho de Melo
@ 2017-01-02 19:39 ` Arnaldo Carvalho de Melo
2017-01-03 0:30 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-01-02 19:39 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> {
> zfree(&iter->priv);
> iter->he = NULL;
> + map__zput(al->map);
What this pairs to? I was expecting that since this is called via:
hist_entry_iter__add()
{
<SNIP>
err2 = iter->ops->finish_entry(iter, al);
}
Then it would have to match something done earlier in
hist_entry_iter__add(), most likely by some iter->ops->() method, but I
couldn'd find anything to that extent, can you clarify?
- Arnaldo
> return 0;
> }
>
> > perf: util/map.c:246: map__exit: Assertion `!(!((&map->rb_node)->__rb_parent_color == (unsigned long)(&map->rb_node)))' failed.
> >
> > Thread 1 "perf" received signal SIGABRT, Aborted.
> > 0x00007ffff522691f in raise () from /lib64/libc.so.6
> > (gdb) bt
> > #0 0x00007ffff522691f in raise () from /lib64/libc.so.6
> > #1 0x00007ffff522851a in abort () from /lib64/libc.so.6
> > #2 0x00007ffff521eda7 in __assert_fail_base () from /lib64/libc.so.6
> > #3 0x00007ffff521ee52 in __assert_fail () from /lib64/libc.so.6
> > #4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
> > #5 0x0000000000504ea5 in map__delete (map=0x2393790) at util/map.c:252
> > #6 0x0000000000504f0a in map__put (map=0x2393790) at util/map.c:259
> > #7 0x000000000052fa01 in __map__zput (map=0x7fffffff8230) at util/map.h:161
> > #8 0x000000000053295b in iter_finish_cumulative_entry (iter=0x7fffffff8260, al=0x7fffffff8220) at util/hist.c:983
> > #9 0x0000000000532b53 in hist_entry_iter__add (iter=0x7fffffff8260, al=0x7fffffff8220, max_stack_depth=127, arg=0x7fffffffa7b0) at util/hist.c:1059
> > #10 0x000000000044f5cf in perf_event__process_sample (tool=0x7fffffffa7b0, event=0x7ffff7e24578, evsel=0x21515d0, sample=0x7fffffff8410, machine=0x21b2bf8)
> > at builtin-top.c:774
> > #11 0x000000000044f8ee in perf_top__mmap_read_idx (top=0x7fffffffa7b0, idx=2) at builtin-top.c:840
> > #12 0x000000000044fa0d in perf_top__mmap_read (top=0x7fffffffa7b0) at builtin-top.c:857
> > #13 0x0000000000450080 in __cmd_top (top=0x7fffffffa7b0) at builtin-top.c:1002
> > #14 0x00000000004514e0 in cmd_top (argc=0, argv=0x7fffffffe130, prefix=0x0) at builtin-top.c:1330
> > #15 0x00000000004b5af5 in run_builtin (p=0xa0baf8 <commands+312>, argc=6, argv=0x7fffffffe130) at perf.c:358
> > #16 0x00000000004b5d62 in handle_internal_command (argc=6, argv=0x7fffffffe130) at perf.c:420
> > #17 0x00000000004b5ea7 in run_argv (argcp=0x7fffffffdf8c, argv=0x7fffffffdf80) at perf.c:466
> > #18 0x00000000004b6290 in main (argc=6, argv=0x7fffffffe130) at perf.c:610
> > (gdb) fr 4
> > #4 0x0000000000504e57 in map__exit (map=0x2393790) at util/map.c:246
> > 246 BUG_ON(!RB_EMPTY_NODE(&map->rb_node));
> > (gdb) p map
> > $1 = (struct map *) 0x2393790
> > (gdb) p *map
> > $2 = {{rb_node = {__rb_parent_color = 37304353, rb_right = 0x0, rb_left = 0x0}, node = {next = 0x2393821, prev = 0x0}}, start = 140434683187200,
> > end = 140434690723840, type = 0 '\000', erange_warned = false, priv = 0, prot = 5, flags = 2, pgoff = 0, reloc = 0, maj = 253, min = 0, ino = 132875,
> > ino_generation = 3472328296227680304, map_ip = 0x504125 <map__map_ip>, unmap_ip = 0x504174 <map__unmap_ip>, dso = 0x22b3890, groups = 0x2385290, refcnt = {
> > counter = 0}}
> > (gdb)
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-02 19:39 ` Arnaldo Carvalho de Melo
@ 2017-01-03 0:30 ` Arnaldo Carvalho de Melo
2017-01-04 8:37 ` Krister Johansen
2017-01-06 6:23 ` [PATCH v3 " Krister Johansen
0 siblings, 2 replies; 25+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-01-03 0:30 UTC (permalink / raw)
To: Krister Johansen
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > {
> > zfree(&iter->priv);
> > iter->he = NULL;
> > + map__zput(al->map);
>
> What this pairs to? I was expecting that since this is called via:
>
> hist_entry_iter__add()
> {
> <SNIP>
> err2 = iter->ops->finish_entry(iter, al);
> }
>
> Then it would have to match something done earlier in
> hist_entry_iter__add(), most likely by some iter->ops->() method, but I
> couldn'd find anything to that extent, can you clarify?
With the following patch it has been running all day, care to explain
why it is needed? I need to run this on valgrind or with Masami's
refcount debugger to get more clues :-\
- Arnaldo
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 72f5c82798e9..c27bda16e9cd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -980,7 +980,6 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
{
zfree(&iter->priv);
iter->he = NULL;
- map__zput(al->map);
return 0;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-03 0:30 ` Arnaldo Carvalho de Melo
@ 2017-01-04 8:37 ` Krister Johansen
2017-01-06 6:22 ` Krister Johansen
2017-01-06 6:23 ` [PATCH v3 " Krister Johansen
1 sibling, 1 reply; 25+ messages in thread
From: Krister Johansen @ 2017-01-04 8:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Krister Johansen, Namhyung Kim, Masami Hiramatsu,
Frédéric Weisbecker, linux-kernel
On Mon, Jan 02, 2017 at 09:30:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > {
> > > zfree(&iter->priv);
> > > iter->he = NULL;
> > > + map__zput(al->map);
> >
> > What this pairs to? I was expecting that since this is called via:
> >
> > hist_entry_iter__add()
> > {
> > <SNIP>
> > err2 = iter->ops->finish_entry(iter, al);
> > }
> >
> > Then it would have to match something done earlier in
> > hist_entry_iter__add(), most likely by some iter->ops->() method, but I
> > couldn'd find anything to that extent, can you clarify?
>
> With the following patch it has been running all day, care to explain
> why it is needed? I need to run this on valgrind or with Masami's
> refcount debugger to get more clues :-\
>
> - Arnaldo
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 72f5c82798e9..c27bda16e9cd 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -980,7 +980,6 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> {
> zfree(&iter->priv);
> iter->he = NULL;
> - map__zput(al->map);
>
> return 0;
> }
Thanks for taking the time to debug it this far. I certainly appreciate
it.
The map_zput() in iter_finish_cumulative_entry() was intended to offset
the map_get() in iter_next_cumulative_entry() -> fill_callchain_info().
The call to fill_callchain_info should perform a map__get() on the
addr_location that gets passed in. However, it looks like I mistakenly
assumed that fill_callcahin_info would always get called from
iter_next_cumulative_entry when clearly that doesn't happen if
callchain_cursor_current returns a NULL node.
Looking back, it's not entirely clear to me that the map__get() in
fill_callchain_info is necessary either, since its only caller is the
hist_iter_cumulative used by hist_entry_iter__add.
Let me try a version of this patch that dispenses with the code in both
fill_callchain_info() and iter_finish_cumulative_entry. However, before
I do that I'll make sure I can figure out how to reproduce the core that
you were seeing. I don't want to send you yet another patch that
doesn't run. The busy system may be the clue I needed. I had been
running these on a mostly idle system.
Thanks again, and apologies for all of the inconvenience.
-K
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 perf/core] perf script: fix a use after free crash.
2017-01-04 8:37 ` Krister Johansen
@ 2017-01-06 6:22 ` Krister Johansen
0 siblings, 0 replies; 25+ messages in thread
From: Krister Johansen @ 2017-01-06 6:22 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
On Wed, Jan 04, 2017 at 12:37:39AM -0800, Krister Johansen wrote:
> On Mon, Jan 02, 2017 at 09:30:33PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > {
> > > > zfree(&iter->priv);
> > > > iter->he = NULL;
> > > > + map__zput(al->map);
> > >
> > > What this pairs to? I was expecting that since this is called via:
> > >
> > > hist_entry_iter__add()
> > > {
> > > <SNIP>
> > > err2 = iter->ops->finish_entry(iter, al);
> > > }
> > >
> > > Then it would have to match something done earlier in
> > > hist_entry_iter__add(), most likely by some iter->ops->() method, but I
> > > couldn'd find anything to that extent, can you clarify?
> >
> > With the following patch it has been running all day, care to explain
> > why it is needed? I need to run this on valgrind or with Masami's
> > refcount debugger to get more clues :-\
>
> Let me try a version of this patch that dispenses with the code in both
> fill_callchain_info() and iter_finish_cumulative_entry. However, before
> I do that I'll make sure I can figure out how to reproduce the core that
> you were seeing. I don't want to send you yet another patch that
> doesn't run. The busy system may be the clue I needed. I had been
> running these on a mostly idle system.
I was able to reproduce your assertion failure using more load on a
system. After fixing up the issues caused by incorrectly updating the
refcounts in the hist_entry_iter's function pointers, I re-ran my own
tests and found that I was still getting a failure for 'perf report'.
The problem there seemed to be that a map was getting into a hist entry,
but was freed before the entry was actually created. The call to
hist_entry__init() that took a reference on the map was actually getting
freed memory by the time it tried to increment the reference count.
To address that, I modified hist_entry_iter__add() to take a reference
on the incoming al->map, if one exists. It drops that reference on the
way out of the function. With that change, I'm able to pass both your
'perf top' test under load and my own tests against a kernel without
debug info, but a kmod that has it.
I'll send out a v3 in just a moment.
Thanks again,
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 perf/core] perf script: fix a use after free crash.
2017-01-03 0:30 ` Arnaldo Carvalho de Melo
2017-01-04 8:37 ` Krister Johansen
@ 2017-01-06 6:23 ` Krister Johansen
2017-01-21 1:48 ` Krister Johansen
` (2 more replies)
1 sibling, 3 replies; 25+ messages in thread
From: Krister Johansen @ 2017-01-06 6:23 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
If dso__load_kcore frees all of the existing maps, but one has already
been attached to a callchain cursor node, then we can get a SIGSEGV in
any function that happens to try to use this invalid cursor. Use the
existing map refcount mechanism to forestall cleanup of a map until the
cursor iterates past the node.
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
tools/perf/util/callchain.c | 11 +++++++++--
tools/perf/util/callchain.h | 6 ++++++
tools/perf/util/hist.c | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 4292251..8b610dd 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -437,7 +437,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
}
call->ip = cursor_node->ip;
call->ms.sym = cursor_node->sym;
- call->ms.map = cursor_node->map;
+ call->ms.map = map__get(cursor_node->map);
if (cursor_node->branch) {
call->branch_count = 1;
@@ -477,6 +477,7 @@ add_child(struct callchain_node *parent,
list_for_each_entry_safe(call, tmp, &new->val, list) {
list_del(&call->list);
+ map__zput(call->ms.map);
free(call);
}
free(new);
@@ -761,6 +762,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
list->ms.map, list->ms.sym,
false, NULL, 0, 0);
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -811,7 +813,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
}
node->ip = ip;
- node->map = map;
+ map__zput(node->map);
+ node->map = map__get(map);
node->sym = sym;
node->branch = branch;
node->nr_loop_iter = nr_loop_iter;
@@ -1142,11 +1145,13 @@ static void free_callchain_node(struct callchain_node *node)
list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
list_for_each_entry_safe(list, tmp, &node->val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -1210,6 +1215,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
goto out;
*new = *chain;
new->has_children = false;
+ map__get(new->ms.map);
list_add_tail(&new->list, &head);
}
parent = parent->parent;
@@ -1230,6 +1236,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
out:
list_for_each_entry_safe(chain, new, &head, list) {
list_del(&chain->list);
+ map__zput(chain->ms.map);
free(chain);
}
return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 35c8e37..4f4b60f 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/rbtree.h>
#include "event.h"
+#include "map.h"
#include "symbol.h"
#define HELP_PAD "\t\t\t\t"
@@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
*/
static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
{
+ struct callchain_cursor_node *node;
+
cursor->nr = 0;
cursor->last = &cursor->first;
+
+ for (node = cursor->first; node != NULL; node = node->next)
+ map__zput(node->map);
}
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6770a96..9dabbae 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -1019,6 +1020,12 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
int max_stack_depth, void *arg)
{
int err, err2;
+ struct map *alm;
+
+ alm = NULL;
+
+ if (al && al->map)
+ alm = map__get(al->map);
err = sample__resolve_callchain(iter->sample, &callchain_cursor, &iter->parent,
iter->evsel, al, max_stack_depth);
@@ -1058,6 +1065,9 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
if (!err)
err = err2;
+ if (alm)
+ map__put(alm);
+
return err;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 perf/core] perf script: fix a use after free crash.
2017-01-06 6:23 ` [PATCH v3 " Krister Johansen
@ 2017-01-21 1:48 ` Krister Johansen
2017-02-01 14:39 ` [tip:perf/core] perf callchain: Reference count maps tip-bot for Krister Johansen
2017-02-03 19:47 ` [tip:perf/urgent] " tip-bot for Krister Johansen
2 siblings, 0 replies; 25+ messages in thread
From: Krister Johansen @ 2017-01-21 1:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Masami Hiramatsu, Frédéric Weisbecker,
linux-kernel
Hey Arnaldo,
On Thu, Jan 05, 2017 at 10:23:31PM -0800, Krister Johansen wrote:
> If dso__load_kcore frees all of the existing maps, but one has already
> been attached to a callchain cursor node, then we can get a SIGSEGV in
> any function that happens to try to use this invalid cursor. Use the
> existing map refcount mechanism to forestall cleanup of a map until the
> cursor iterates past the node.
It's been a couple of weeks since I sent you the v3 of this patch. Last
time I fiddled with it, I was able to reproduce your 'perf top' core,
and was able to verify that the latest patch I sent out could survive
running 'perf top' through the course of a full kernel build.
Is there anything else I can do to help with this one?
Thanks,
-K
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tip:perf/core] perf callchain: Reference count maps
2017-01-06 6:23 ` [PATCH v3 " Krister Johansen
2017-01-21 1:48 ` Krister Johansen
@ 2017-02-01 14:39 ` tip-bot for Krister Johansen
2017-02-03 19:47 ` [tip:perf/urgent] " tip-bot for Krister Johansen
2 siblings, 0 replies; 25+ messages in thread
From: tip-bot for Krister Johansen @ 2017-02-01 14:39 UTC (permalink / raw)
To: linux-tip-commits
Cc: kjlx, fweisbec, acme, tglx, mhiramat, linux-kernel, hpa, mingo, namhyung
Commit-ID: 9c68ae98c6f714ef573826cfc9055af1bd5e97b1
Gitweb: http://git.kernel.org/tip/9c68ae98c6f714ef573826cfc9055af1bd5e97b1
Author: Krister Johansen <kjlx@templeofstupid.com>
AuthorDate: Thu, 5 Jan 2017 22:23:31 -0800
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 31 Jan 2017 16:19:06 -0300
perf callchain: Reference count maps
If dso__load_kcore frees all of the existing maps, but one has already
been attached to a callchain cursor node, then we can get a SIGSEGV in
any function that happens to try to use this invalid cursor. Use the
existing map refcount mechanism to forestall cleanup of a map until the
cursor iterates past the node.
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: stable@kernel.org
Fixes: 84c2cafa2889 ("perf tools: Reference count struct map")
Link: http://lkml.kernel.org/r/20170106062331.GB2707@templeofstupid.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/callchain.c | 11 +++++++++--
tools/perf/util/callchain.h | 6 ++++++
tools/perf/util/hist.c | 7 +++++++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index e16db30..aba9534 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -449,7 +449,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
}
call->ip = cursor_node->ip;
call->ms.sym = cursor_node->sym;
- call->ms.map = cursor_node->map;
+ call->ms.map = map__get(cursor_node->map);
if (cursor_node->branch) {
call->branch_count = 1;
@@ -489,6 +489,7 @@ add_child(struct callchain_node *parent,
list_for_each_entry_safe(call, tmp, &new->val, list) {
list_del(&call->list);
+ map__zput(call->ms.map);
free(call);
}
free(new);
@@ -773,6 +774,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
list->ms.map, list->ms.sym,
false, NULL, 0, 0);
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -823,7 +825,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
}
node->ip = ip;
- node->map = map;
+ map__zput(node->map);
+ node->map = map__get(map);
node->sym = sym;
node->branch = branch;
node->nr_loop_iter = nr_loop_iter;
@@ -1154,11 +1157,13 @@ static void free_callchain_node(struct callchain_node *node)
list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
list_for_each_entry_safe(list, tmp, &node->val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -1222,6 +1227,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
goto out;
*new = *chain;
new->has_children = false;
+ map__get(new->ms.map);
list_add_tail(&new->list, &head);
}
parent = parent->parent;
@@ -1242,6 +1248,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
out:
list_for_each_entry_safe(chain, new, &head, list) {
list_del(&chain->list);
+ map__zput(chain->ms.map);
free(chain);
}
return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 35c8e37..4f4b60f 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/rbtree.h>
#include "event.h"
+#include "map.h"
#include "symbol.h"
#define HELP_PAD "\t\t\t\t"
@@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
*/
static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
{
+ struct callchain_cursor_node *node;
+
cursor->nr = 0;
cursor->last = &cursor->first;
+
+ for (node = cursor->first; node != NULL; node = node->next)
+ map__zput(node->map);
}
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cff2e90..32c6a93 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -1019,6 +1020,10 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
int max_stack_depth, void *arg)
{
int err, err2;
+ struct map *alm = NULL;
+
+ if (al && al->map)
+ alm = map__get(al->map);
err = sample__resolve_callchain(iter->sample, &callchain_cursor, &iter->parent,
iter->evsel, al, max_stack_depth);
@@ -1058,6 +1063,8 @@ out:
if (!err)
err = err2;
+ map__put(alm);
+
return err;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread* [tip:perf/urgent] perf callchain: Reference count maps
2017-01-06 6:23 ` [PATCH v3 " Krister Johansen
2017-01-21 1:48 ` Krister Johansen
2017-02-01 14:39 ` [tip:perf/core] perf callchain: Reference count maps tip-bot for Krister Johansen
@ 2017-02-03 19:47 ` tip-bot for Krister Johansen
2 siblings, 0 replies; 25+ messages in thread
From: tip-bot for Krister Johansen @ 2017-02-03 19:47 UTC (permalink / raw)
To: linux-tip-commits
Cc: hpa, mingo, mhiramat, acme, namhyung, kjlx, fweisbec, linux-kernel, tglx
Commit-ID: aa33b9b9a2ebb00d33c83a5312d4fbf2d5aeba36
Gitweb: http://git.kernel.org/tip/aa33b9b9a2ebb00d33c83a5312d4fbf2d5aeba36
Author: Krister Johansen <kjlx@templeofstupid.com>
AuthorDate: Thu, 5 Jan 2017 22:23:31 -0800
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 2 Feb 2017 11:39:09 -0300
perf callchain: Reference count maps
If dso__load_kcore frees all of the existing maps, but one has already
been attached to a callchain cursor node, then we can get a SIGSEGV in
any function that happens to try to use this invalid cursor. Use the
existing map refcount mechanism to forestall cleanup of a map until the
cursor iterates past the node.
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: stable@kernel.org
Fixes: 84c2cafa2889 ("perf tools: Reference count struct map")
Link: http://lkml.kernel.org/r/20170106062331.GB2707@templeofstupid.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/callchain.c | 11 +++++++++--
tools/perf/util/callchain.h | 6 ++++++
tools/perf/util/hist.c | 7 +++++++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 4292251..8b610dd 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -437,7 +437,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
}
call->ip = cursor_node->ip;
call->ms.sym = cursor_node->sym;
- call->ms.map = cursor_node->map;
+ call->ms.map = map__get(cursor_node->map);
if (cursor_node->branch) {
call->branch_count = 1;
@@ -477,6 +477,7 @@ add_child(struct callchain_node *parent,
list_for_each_entry_safe(call, tmp, &new->val, list) {
list_del(&call->list);
+ map__zput(call->ms.map);
free(call);
}
free(new);
@@ -761,6 +762,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
list->ms.map, list->ms.sym,
false, NULL, 0, 0);
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -811,7 +813,8 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
}
node->ip = ip;
- node->map = map;
+ map__zput(node->map);
+ node->map = map__get(map);
node->sym = sym;
node->branch = branch;
node->nr_loop_iter = nr_loop_iter;
@@ -1142,11 +1145,13 @@ static void free_callchain_node(struct callchain_node *node)
list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
list_for_each_entry_safe(list, tmp, &node->val, list) {
list_del(&list->list);
+ map__zput(list->ms.map);
free(list);
}
@@ -1210,6 +1215,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
goto out;
*new = *chain;
new->has_children = false;
+ map__get(new->ms.map);
list_add_tail(&new->list, &head);
}
parent = parent->parent;
@@ -1230,6 +1236,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
out:
list_for_each_entry_safe(chain, new, &head, list) {
list_del(&chain->list);
+ map__zput(chain->ms.map);
free(chain);
}
return -ENOMEM;
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 35c8e37..4f4b60f 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/rbtree.h>
#include "event.h"
+#include "map.h"
#include "symbol.h"
#define HELP_PAD "\t\t\t\t"
@@ -184,8 +185,13 @@ int callchain_merge(struct callchain_cursor *cursor,
*/
static inline void callchain_cursor_reset(struct callchain_cursor *cursor)
{
+ struct callchain_cursor_node *node;
+
cursor->nr = 0;
cursor->last = &cursor->first;
+
+ for (node = cursor->first; node != NULL; node = node->next)
+ map__zput(node->map);
}
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6770a96..7d1b7d3 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1,6 +1,7 @@
#include "util.h"
#include "build-id.h"
#include "hist.h"
+#include "map.h"
#include "session.h"
#include "sort.h"
#include "evlist.h"
@@ -1019,6 +1020,10 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
int max_stack_depth, void *arg)
{
int err, err2;
+ struct map *alm = NULL;
+
+ if (al && al->map)
+ alm = map__get(al->map);
err = sample__resolve_callchain(iter->sample, &callchain_cursor, &iter->parent,
iter->evsel, al, max_stack_depth);
@@ -1058,6 +1063,8 @@ out:
if (!err)
err = err2;
+ map__put(alm);
+
return err;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread