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.
Signed-off-by: Krister Johansen <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Wed, 5 Oct 2016 08:45:24 -0300
Arnaldo Carvalho de Melo <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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
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 <[email protected]>
> > ---
> > 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
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
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
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 <[email protected]>
---
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
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
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
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
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 <[email protected]>
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
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;
}
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
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 <[email protected]>
> 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 <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
>
> 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