From fe693d951e3c303b4fb6c712f8affecbe9e8b001 Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Tue, 18 Apr 2023 17:33:48 -0300 Subject: perf maps: Add maps__refcnt() accessor to allow checking maps pointer To remove one more direct access to 'struct maps' so that we can intercept accesses to its instantiations and refcount check it to catch use after free, etc. Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/tests/thread-maps-share.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'tools/perf/tests/thread-maps-share.c') diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c index 84edd82c519e..caf8fbe7c440 100644 --- a/tools/perf/tests/thread-maps-share.c +++ b/tools/perf/tests/thread-maps-share.c @@ -43,7 +43,7 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s leader && t1 && t2 && t3 && other); maps = leader->maps; - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 4); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 4); /* test the maps pointer is shared */ TEST_ASSERT_VAL("maps don't match", maps == t1->maps); @@ -71,25 +71,25 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s machine__remove_thread(machine, other_leader); other_maps = other->maps; - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&other_maps->refcnt), 2); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(other_maps)), 2); TEST_ASSERT_VAL("maps don't match", other_maps == other_leader->maps); /* release thread group */ thread__put(leader); - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 3); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3); thread__put(t1); - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 2); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2); thread__put(t2); - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 1); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1); thread__put(t3); /* release other group */ thread__put(other_leader); - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&other_maps->refcnt), 1); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(other_maps)), 1); thread__put(other); -- cgit v1.2.3 From 8f12692b7e61e5fb5d3e4f6692d6675f62eeebdc Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Wed, 19 Apr 2023 10:48:37 -0300 Subject: perf maps: Add reference count checking Add reference count checking to make sure of good use of get and put. Add and use accessors to reduce RC_CHK clutter. The only significant issue was in tests/thread-maps-share.c where reference counts were released in the reverse order to acquisition, leading to a use after put. This was fixed by reversing the put order. Committer notes: Extracted from a larger patch removing bits that were covered by the use of pre-existing maps__ accessors (e.g. maps__nr_maps()) and new ones added (maps__refcnt()) to reduce RC_CHK_ACCESS(maps)-> source code pollution. Signed-off-by: Ian Rogers Cc: Adrian Hunter Cc: Alexey Bayduraev Cc: Dmitriy Vyukov Cc: Jiri Olsa Cc: Namhyung Kim Cc: Riccardo Mancini Cc: Stephane Eranian Cc: Stephen Brennan Link: https://lore.kernel.org/lkml/20230407230405.2931830-5-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/tests/thread-maps-share.c | 8 +++--- tools/perf/util/maps.c | 47 ++++++++++++++++++-------------- tools/perf/util/maps.h | 19 +++++++------ tools/perf/util/symbol.c | 13 +++++---- tools/perf/util/unwind-libdw.c | 2 +- tools/perf/util/unwind-libunwind-local.c | 2 +- tools/perf/util/unwind-libunwind.c | 2 +- 7 files changed, 50 insertions(+), 43 deletions(-) (limited to 'tools/perf/tests/thread-maps-share.c') diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c index caf8fbe7c440..75ce8aedfc78 100644 --- a/tools/perf/tests/thread-maps-share.c +++ b/tools/perf/tests/thread-maps-share.c @@ -46,9 +46,9 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 4); /* test the maps pointer is shared */ - TEST_ASSERT_VAL("maps don't match", maps == t1->maps); - TEST_ASSERT_VAL("maps don't match", maps == t2->maps); - TEST_ASSERT_VAL("maps don't match", maps == t3->maps); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) == RC_CHK_ACCESS(t1->maps)); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) == RC_CHK_ACCESS(t2->maps)); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) == RC_CHK_ACCESS(t3->maps)); /* * Verify the other leader was created by previous call. @@ -73,7 +73,7 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s other_maps = other->maps; TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(other_maps)), 2); - TEST_ASSERT_VAL("maps don't match", other_maps == other_leader->maps); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps)); /* release thread group */ thread__put(leader); diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c index 953dc20d55d7..8a13396acd1a 100644 --- a/tools/perf/util/maps.c +++ b/tools/perf/util/maps.c @@ -13,12 +13,12 @@ static void maps__init(struct maps *maps, struct machine *machine) { refcount_set(maps__refcnt(maps), 1); - maps->entries = RB_ROOT; init_rwsem(maps__lock(maps)); - maps->machine = machine; - maps->last_search_by_name = NULL; - maps->nr_maps = 0; - maps->maps_by_name = NULL; + RC_CHK_ACCESS(maps)->entries = RB_ROOT; + RC_CHK_ACCESS(maps)->machine = machine; + RC_CHK_ACCESS(maps)->last_search_by_name = NULL; + RC_CHK_ACCESS(maps)->nr_maps = 0; + RC_CHK_ACCESS(maps)->maps_by_name = NULL; } static void __maps__free_maps_by_name(struct maps *maps) @@ -29,8 +29,8 @@ static void __maps__free_maps_by_name(struct maps *maps) for (unsigned int i = 0; i < maps__nr_maps(maps); i++) map__put(maps__maps_by_name(maps)[i]); - zfree(&maps->maps_by_name); - maps->nr_maps_allocated = 0; + zfree(&RC_CHK_ACCESS(maps)->maps_by_name); + RC_CHK_ACCESS(maps)->nr_maps_allocated = 0; } static int __maps__insert(struct maps *maps, struct map *map) @@ -71,7 +71,7 @@ int maps__insert(struct maps *maps, struct map *map) if (err) goto out; - ++maps->nr_maps; + ++RC_CHK_ACCESS(maps)->nr_maps; if (dso && dso->kernel) { struct kmap *kmap = map__kmap(map); @@ -88,7 +88,7 @@ int maps__insert(struct maps *maps, struct map *map) * inserted map and resort. */ if (maps__maps_by_name(maps)) { - if (maps__nr_maps(maps) > maps->nr_maps_allocated) { + if (maps__nr_maps(maps) > RC_CHK_ACCESS(maps)->nr_maps_allocated) { int nr_allocate = maps__nr_maps(maps) * 2; struct map **maps_by_name = realloc(maps__maps_by_name(maps), nr_allocate * sizeof(map)); @@ -99,8 +99,8 @@ int maps__insert(struct maps *maps, struct map *map) goto out; } - maps->maps_by_name = maps_by_name; - maps->nr_maps_allocated = nr_allocate; + RC_CHK_ACCESS(maps)->maps_by_name = maps_by_name; + RC_CHK_ACCESS(maps)->nr_maps_allocated = nr_allocate; } maps__maps_by_name(maps)[maps__nr_maps(maps) - 1] = map__get(map); __maps__sort_by_name(maps); @@ -122,15 +122,15 @@ void maps__remove(struct maps *maps, struct map *map) struct map_rb_node *rb_node; down_write(maps__lock(maps)); - if (maps->last_search_by_name == map) - maps->last_search_by_name = NULL; + if (RC_CHK_ACCESS(maps)->last_search_by_name == map) + RC_CHK_ACCESS(maps)->last_search_by_name = NULL; rb_node = maps__find_node(maps, map); assert(rb_node->map == map); __maps__remove(maps, rb_node); if (maps__maps_by_name(maps)) __maps__free_maps_by_name(maps); - --maps->nr_maps; + --RC_CHK_ACCESS(maps)->nr_maps; up_write(maps__lock(maps)); } @@ -162,33 +162,38 @@ bool maps__empty(struct maps *maps) struct maps *maps__new(struct machine *machine) { - struct maps *maps = zalloc(sizeof(*maps)); + struct maps *result; + RC_STRUCT(maps) *maps = zalloc(sizeof(*maps)); - if (maps != NULL) - maps__init(maps, machine); + if (ADD_RC_CHK(result, maps)) + maps__init(result, machine); - return maps; + return result; } void maps__delete(struct maps *maps) { maps__exit(maps); unwind__finish_access(maps); - free(maps); + RC_CHK_FREE(maps); } struct maps *maps__get(struct maps *maps) { - if (maps) + struct maps *result; + + if (RC_CHK_GET(result, maps)) refcount_inc(maps__refcnt(maps)); - return maps; + return result; } void maps__put(struct maps *maps) { if (maps && refcount_dec_and_test(maps__refcnt(maps))) maps__delete(maps); + else + RC_CHK_PUT(maps); } struct symbol *maps__find_symbol(struct maps *maps, u64 addr, struct map **mapp) diff --git a/tools/perf/util/maps.h b/tools/perf/util/maps.h index cfb1b79d1671..d2963456cfbe 100644 --- a/tools/perf/util/maps.h +++ b/tools/perf/util/maps.h @@ -8,6 +8,7 @@ #include #include #include "rwsem.h" +#include struct ref_reloc_sym; struct machine; @@ -32,7 +33,7 @@ struct map *maps__find(struct maps *maps, u64 addr); for (map = maps__first(maps), next = map_rb_node__next(map); map; \ map = next, next = map_rb_node__next(map)) -struct maps { +DECLARE_RC_STRUCT(maps) { struct rb_root entries; struct rw_semaphore lock; struct machine *machine; @@ -65,43 +66,43 @@ void maps__put(struct maps *maps); static inline struct rb_root *maps__entries(struct maps *maps) { - return &maps->entries; + return &RC_CHK_ACCESS(maps)->entries; } static inline struct machine *maps__machine(struct maps *maps) { - return maps->machine; + return RC_CHK_ACCESS(maps)->machine; } static inline struct rw_semaphore *maps__lock(struct maps *maps) { - return &maps->lock; + return &RC_CHK_ACCESS(maps)->lock; } static inline struct map **maps__maps_by_name(struct maps *maps) { - return maps->maps_by_name; + return RC_CHK_ACCESS(maps)->maps_by_name; } static inline unsigned int maps__nr_maps(const struct maps *maps) { - return maps->nr_maps; + return RC_CHK_ACCESS(maps)->nr_maps; } static inline refcount_t *maps__refcnt(struct maps *maps) { - return &maps->refcnt; + return &RC_CHK_ACCESS(maps)->refcnt; } #ifdef HAVE_LIBUNWIND_SUPPORT static inline void *maps__addr_space(struct maps *maps) { - return maps->addr_space; + return RC_CHK_ACCESS(maps)->addr_space; } static inline const struct unwind_libunwind_ops *maps__unwind_libunwind_ops(const struct maps *maps) { - return maps->unwind_libunwind_ops; + return RC_CHK_ACCESS(maps)->unwind_libunwind_ops; } #endif diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c index e7f63670688e..01fa5560a0bb 100644 --- a/tools/perf/util/symbol.c +++ b/tools/perf/util/symbol.c @@ -2096,8 +2096,8 @@ static int map__groups__sort_by_name_from_rbtree(struct maps *maps) up_read(maps__lock(maps)); down_write(maps__lock(maps)); - maps->maps_by_name = maps_by_name; - maps->nr_maps_allocated = maps__nr_maps(maps); + RC_CHK_ACCESS(maps)->maps_by_name = maps_by_name; + RC_CHK_ACCESS(maps)->nr_maps_allocated = maps__nr_maps(maps); maps__for_each_entry(maps, rb_node) maps_by_name[i++] = map__get(rb_node->map); @@ -2132,11 +2132,12 @@ struct map *maps__find_by_name(struct maps *maps, const char *name) down_read(maps__lock(maps)); - if (maps->last_search_by_name) { - const struct dso *dso = map__dso(maps->last_search_by_name); + + if (RC_CHK_ACCESS(maps)->last_search_by_name) { + const struct dso *dso = map__dso(RC_CHK_ACCESS(maps)->last_search_by_name); if (strcmp(dso->short_name, name) == 0) { - map = maps->last_search_by_name; + map = RC_CHK_ACCESS(maps)->last_search_by_name; goto out_unlock; } } @@ -2156,7 +2157,7 @@ struct map *maps__find_by_name(struct maps *maps, const char *name) map = rb_node->map; dso = map__dso(map); if (strcmp(dso->short_name, name) == 0) { - maps->last_search_by_name = map; + RC_CHK_ACCESS(maps)->last_search_by_name = map; goto out_unlock; } } diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c index 9565f9906e5d..bdccfc511b7e 100644 --- a/tools/perf/util/unwind-libdw.c +++ b/tools/perf/util/unwind-libdw.c @@ -230,7 +230,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, struct unwind_info *ui, ui_buf = { .sample = data, .thread = thread, - .machine = thread->maps->machine, + .machine = RC_CHK_ACCESS(thread->maps)->machine, .cb = cb, .arg = arg, .max_stack = max_stack, diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c index f9a52af48de4..83dd79dcd597 100644 --- a/tools/perf/util/unwind-libunwind-local.c +++ b/tools/perf/util/unwind-libunwind-local.c @@ -677,7 +677,7 @@ static int _unwind__prepare_access(struct maps *maps) { void *addr_space = unw_create_addr_space(&accessors, 0); - maps->addr_space = addr_space; + RC_CHK_ACCESS(maps)->addr_space = addr_space; if (!addr_space) { pr_err("unwind: Can't create unwind address space.\n"); return -ENOMEM; diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c index 4378daaafcd3..b54968e6a4e4 100644 --- a/tools/perf/util/unwind-libunwind.c +++ b/tools/perf/util/unwind-libunwind.c @@ -14,7 +14,7 @@ struct unwind_libunwind_ops __weak *arm64_unwind_libunwind_ops; static void unwind__register_ops(struct maps *maps, struct unwind_libunwind_ops *ops) { - maps->unwind_libunwind_ops = ops; + RC_CHK_ACCESS(maps)->unwind_libunwind_ops = ops; } int unwind__prepare_access(struct maps *maps, struct map *map, bool *initialized) -- cgit v1.2.3 From edd4cab2d492daa5e45bd45e44084a2b9880f224 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Wed, 19 Apr 2023 20:04:30 -0700 Subject: perf test: Fix maps use after put Fix a use after put reference count issue. maps is copied from leader, but the leader is put on line 79 and then maps is used to read the reference count below - so a use after put, with the put of maps happening within thread__put. Fix by reversing the order of puts so that the leader is put last. To explain the reference count checker, I wrote this up as a little example here: https://perf.wiki.kernel.org/index.php/Reference_Count_Checking Note, the bug was introduced by the committer and wasn't present in the original reference count patch set. Committer notes: Yes, the bug predated your patch and is detected by the reference count checking you contributed. This was just part of splitting up your series into smaller chunks, in this case either we fix the problem detected while developing this reference counting infrastructure before the patch introducing REFCNT_CHECKING or fix it later after the merged infrastructure, when built with EXTRA_CFLAGS="-DREFCNT_CHECKING=1" detects it when running 'perf test', which is what this patch does. Signed-off-by: Ian Rogers Cc: Adrian Hunter Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lore.kernel.org/lkml/20230420030430.489243-1-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/tests/thread-maps-share.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tools/perf/tests/thread-maps-share.c') diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c index 75ce8aedfc78..858e725318a9 100644 --- a/tools/perf/tests/thread-maps-share.c +++ b/tools/perf/tests/thread-maps-share.c @@ -76,16 +76,16 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps)); /* release thread group */ - thread__put(leader); + thread__put(t3); TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3); - thread__put(t1); + thread__put(t2); TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2); - thread__put(t2); + thread__put(t1); TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1); - thread__put(t3); + thread__put(leader); /* release other group */ thread__put(other_leader); -- cgit v1.2.3