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 <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
Cc: Dmitriy Vyukov <dvyukov@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Riccardo Mancini <rickyman7@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Stephen Brennan <stephen.s.brennan@oracle.com>
Link: https://lore.kernel.org/lkml/20230407230405.2931830-5-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
index caf8fbe7c4409100c5a22bc3ddf9ae5d260f5843..75ce8aedfc789d4287fca437d81a818de25d334f 100644 (file)
--- 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 953dc20d55d7d96b78f9fc58bf6fa737379497b5..8a13396acd1a058f82662b54e2cb2976778bf5af 100644 (file)
--- 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 cfb1b79d16713b27d8590185fc6ea92268783ab7..d2963456cfbe78d0771d41b26bc9060eb3b64c39 100644 (file)
--- a/tools/perf/util/maps.h
+++ b/tools/perf/util/maps.h
@@ -8,6 +8,7 @@
 #include <stdbool.h>
 #include <linux/types.h>
 #include "rwsem.h"
+#include <internal/rc_check.h>
 
 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 e7f63670688e8e594aa5683fe6df600794481671..01fa5560a0bbe366aecd2a70f20246499296a912 100644 (file)
--- 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 9565f9906e5da9d4e3bbfedab2526e3bb38ebd7f..bdccfc511b7e206d3ead206911a9bf56060221f2 100644 (file)
--- 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 f9a52af48de452b383dc0bf505daf6bbf7f6fa0f..83dd79dcd597e59573e4093a9c1d471b3b0d6661 100644 (file)
--- 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 4378daaafcd3b87557ba702719ff982fa652cedd..b54968e6a4e4cbe00486f0720e343630ac78e9f3 100644 (file)
--- 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)