| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20220211103415.2737789-22-irogers@google.com>
Date: Fri, 11 Feb 2022 02:34:14 -0800
From: Ian Rogers <irogers@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Darren Hart <dvhart@...radead.org>,
Davidlohr Bueso <dave@...olabs.net>,
"André Almeida" <andrealmeid@...labora.com>,
James Clark <james.clark@....com>,
John Garry <john.garry@...wei.com>,
Riccardo Mancini <rickyman7@...il.com>,
Yury Norov <yury.norov@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Jin Yao <yao.jin@...ux.intel.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Leo Yan <leo.yan@...aro.org>, Andi Kleen <ak@...ux.intel.com>,
Thomas Richter <tmricht@...ux.ibm.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Madhavan Srinivasan <maddy@...ux.ibm.com>,
Shunsuke Nakamura <nakamura.shun@...itsu.com>,
Song Liu <song@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Miaoqian Lin <linmq006@...il.com>,
Stephen Brennan <stephen.s.brennan@...cle.com>,
Kajol Jain <kjain@...ux.ibm.com>,
Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
German Gomez <german.gomez@....com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>, Hao Luo <haoluo@...gle.com>
Cc: eranian@...gle.com, Ian Rogers <irogers@...gle.com>
Subject: [PATCH v3 21/22] 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.
Signed-off-by: Ian Rogers <irogers@...gle.com>
---
tools/perf/tests/thread-maps-share.c | 29 ++++++-------
tools/perf/util/maps.c | 53 +++++++++++++-----------
tools/perf/util/maps.h | 17 ++++----
tools/perf/util/symbol.c | 10 ++---
tools/perf/util/unwind-libunwind-local.c | 2 +-
tools/perf/util/unwind-libunwind.c | 2 +-
6 files changed, 60 insertions(+), 53 deletions(-)
diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
index 84edd82c519e..dfe51b21bd7d 100644
--- a/tools/perf/tests/thread-maps-share.c
+++ b/tools/perf/tests/thread-maps-share.c
@@ -43,12 +43,12 @@ 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(&RC_CHK_ACCESS(maps)->refcnt), 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.
@@ -71,25 +71,26 @@ 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(&RC_CHK_ACCESS(other_maps)->refcnt), 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);
- TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 3);
-
- thread__put(t1);
- TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 2);
+ thread__put(t3);
+ TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->refcnt), 3);
thread__put(t2);
- TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 1);
+ TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->refcnt), 2);
- thread__put(t3);
+ thread__put(t1);
+ TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->refcnt), 1);
+
+ thread__put(leader);
/* 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(&RC_CHK_ACCESS(other_maps)->refcnt), 1);
thread__put(other);
diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 6efbcb79131c..da59204cb9bb 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -12,13 +12,13 @@
static void maps__init(struct maps *maps, struct machine *machine)
{
- maps->entries = RB_ROOT;
+ RC_CHK_ACCESS(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;
- refcount_set(&maps->refcnt, 1);
+ 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;
+ refcount_set(&RC_CHK_ACCESS(maps)->refcnt, 1);
}
static void __maps__free_maps_by_name(struct maps *maps)
@@ -26,8 +26,8 @@ static void __maps__free_maps_by_name(struct maps *maps)
/*
* Free everything to try to do it from the rbtree in the next search
*/
- 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 struct map *__maps__insert(struct maps *maps, struct map *map)
@@ -69,7 +69,7 @@ int maps__insert(struct maps *maps, struct map *map)
goto out;
}
- ++maps->nr_maps;
+ ++RC_CHK_ACCESS(maps)->nr_maps;
if (map__dso(map) && map__dso(map)->kernel) {
struct kmap *kmap = map__kmap(map);
@@ -86,7 +86,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));
@@ -97,8 +97,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;
__maps__sort_by_name(maps);
@@ -120,13 +120,13 @@ 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);
- --maps->nr_maps;
+ --RC_CHK_ACCESS(maps)->nr_maps;
if (maps__maps_by_name(maps))
__maps__free_maps_by_name(maps);
up_write(maps__lock(maps));
@@ -157,33 +157,38 @@ bool maps__empty(struct maps *maps)
struct maps *maps__new(struct machine *machine)
{
- struct maps *maps = zalloc(sizeof(*maps));
+ struct maps *res;
+ RC_STRUCT(maps) *maps = zalloc(sizeof(*maps));
- if (maps != NULL)
- maps__init(maps, machine);
+ if (ADD_RC_CHK(res, maps))
+ maps__init(res, machine);
- return maps;
+ return res;
}
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)
- refcount_inc(&maps->refcnt);
+ struct maps *result;
- return maps;
+ if (RC_CHK_GET(result, maps))
+ refcount_inc(&RC_CHK_ACCESS(maps)->refcnt);
+
+ return result;
}
void maps__put(struct maps *maps)
{
- if (maps && refcount_dec_and_test(&maps->refcnt))
+ if (maps && refcount_dec_and_test(&RC_CHK_ACCESS(maps)->refcnt))
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 bde3390c7096..0af4b7e42fca 100644
--- 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,38 +66,38 @@ 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;
}
#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 6289b3028b91..fdaeeebd6050 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2025,8 +2025,8 @@ static int map__groups__sort_by_name_from_rbtree(struct maps *maps)
if (maps_by_name == NULL)
return -1;
- 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++] = rb_node->map;
@@ -2057,9 +2057,9 @@ struct map *maps__find_by_name(struct maps *maps, const char *name)
down_read(maps__lock(maps));
- if (maps->last_search_by_name &&
+ if (RC_CHK_ACCESS(maps)->last_search_by_name &&
strcmp(map__dso(maps->last_search_by_name)->short_name, name) == 0) {
- map = maps->last_search_by_name;
+ map = RC_CHK_ACCESS(maps)->last_search_by_name;
goto out_unlock;
}
/*
@@ -2075,7 +2075,7 @@ struct map *maps__find_by_name(struct maps *maps, const char *name)
maps__for_each_entry(maps, rb_node) {
map = rb_node->map;
if (strcmp(map__dso(map)->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-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 841ac84a93ab..e86a6e594017 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -620,7 +620,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 cece1ee89031..973eaa18ec75 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)
--
2.35.1.265.g69c8d7142f-goog
Powered by blists - more mailing lists