Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756026AbcKVTB3 (ORCPT ); Tue, 22 Nov 2016 14:01:29 -0500 Received: from mail.kernel.org ([198.145.29.136]:40266 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752126AbcKVTB2 (ORCPT ); Tue, 22 Nov 2016 14:01:28 -0500 Date: Tue, 22 Nov 2016 16:01:06 -0300 From: Arnaldo Carvalho de Melo To: Krister Johansen Cc: Namhyung Kim , Masami Hiramatsu , =?iso-8859-1?Q?Fr=E9d=E9ric?= Weisbecker , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 perf/core] perf script: fix a use after free crash. Message-ID: <20161122190106.GE5390@kernel.org> References: <20161007022200.GB31113@sejong> <20161011092839.GC7837@templeofstupid.com> <20161026002010.GD2525@templeofstupid.com> <20161026134453.GA4936@kernel.org> <20161111004046.GA2185@templeofstupid.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161111004046.GA2185@templeofstupid.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7912 Lines: 211 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 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 Cc: Frederic Weisbecker Cc: Masami Hiramatsu Cc: Namhyung Kim Link: http://lkml.kernel.org/r/20161011092839.GC7837@templeofstupid.com Signed-off-by: Arnaldo Carvalho de Melo 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 #include #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; }