From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98E1CC433F5 for ; Fri, 11 Feb 2022 10:35:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1349082AbiBKKf4 (ORCPT ); Fri, 11 Feb 2022 05:35:56 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:32832 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346717AbiBKKfW (ORCPT ); Fri, 11 Feb 2022 05:35:22 -0500 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9876CEBC for ; Fri, 11 Feb 2022 02:35:13 -0800 (PST) Received: by mail-yb1-xb49.google.com with SMTP id b64-20020a256743000000b0061e169a5f19so17496304ybc.11 for ; Fri, 11 Feb 2022 02:35:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=Oum2Nhoc7NbISccsgG5Jn7Q0d1b5FELJ2y1ISHsilug=; b=SqNd7nr8OnsH2fcSqFE+jD1XVYfw9oiItQEVbt8h2Ej1MxbdnQlpID8Zo7sh4l5QIp l7FRUjL0aJT+2yi0L1w61ooJj3Vq16s4HRL3wKgVOZvr4WxkGxCqh4kx7Hx/EPY5oZ+B CS6PRzyEntI2Rt4g+krkTRIb2yYWKsJ/NBQZ8R65ZrU1vjx1s0MW3lm1GY648jIYVCal 0Tn9Lf64mN1xD1QNO4R/P/yZDHY8ChwRrIOqHtBj7jjlnl7Dj7h7oB6K3vLpqIeUWqVs 0R9QzuJcfDiSiFYSmum8dhyorJKYnOGm38oDKemgtpCFeiXrOy+uC8mHNtBcjkwLVohh HeFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=Oum2Nhoc7NbISccsgG5Jn7Q0d1b5FELJ2y1ISHsilug=; b=gpJ9lyoKCZamC3+Rdl54KQtHYXl/ARHQ0QMow6TAFR+GH/qpl8wm2etXNKgRmZMNvU 8SUElc3TTPDQ+UxqFIfkcXeZi64KZMi0HFGxuNP+rI3JQEVOQbYyQ4wvl76YJlL9fZ/t yxhS+HEm7nSYbjDLIBg8XNGv2Hw6tLZ5nW+FUqE33SU/uDS5rSsVtJTMBMwwJ68POPsX xM+X0teQ+or/GUj+E9XszDiHppsmdNfdlKFZKyJSM9EK2jYCmJsPWeFQVo5gCpNpJeTU cI+I3Pe8OJkYaE2G3H20u52pEUjPRfFS19qX0C2RMA/LXqBaEzR6cUfyy5FLEIyZZ+BH MoRg== X-Gm-Message-State: AOAM5303sR3MjNuY5WyCTWVAjwOHKVWg5y6MwAO9RHMQgU8a3ZcB6K2H YywYIOUQuIcx+zbknut8okmMHdAvdw17 X-Google-Smtp-Source: ABdhPJw45mHHWKIdSKrq1PeFf1JkKXQfDYORTcl8GBaes9P3lVQEtUrG6rPUQgh5wQqXIouuUQUUp08Dm6ty X-Received: from irogers.svl.corp.google.com ([2620:15c:2cd:202:2d98:3ad9:1d8a:fb9b]) (user=irogers job=sendgmr) by 2002:a25:4144:: with SMTP id o65mr650609yba.113.1644575712950; Fri, 11 Feb 2022 02:35:12 -0800 (PST) Date: Fri, 11 Feb 2022 02:34:14 -0800 In-Reply-To: <20220211103415.2737789-1-irogers@google.com> Message-Id: <20220211103415.2737789-22-irogers@google.com> Mime-Version: 1.0 References: <20220211103415.2737789-1-irogers@google.com> X-Mailer: git-send-email 2.35.1.265.g69c8d7142f-goog Subject: [PATCH v3 21/22] perf maps: Add reference count checking. From: Ian Rogers To: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Thomas Gleixner , Darren Hart , Davidlohr Bueso , "=?UTF-8?q?Andr=C3=A9=20Almeida?=" , James Clark , John Garry , Riccardo Mancini , Yury Norov , Andy Shevchenko , Andrew Morton , Jin Yao , Adrian Hunter , Leo Yan , Andi Kleen , Thomas Richter , Kan Liang , Madhavan Srinivasan , Shunsuke Nakamura , Song Liu , Masami Hiramatsu , Steven Rostedt , Miaoqian Lin , Stephen Brennan , Kajol Jain , Alexey Bayduraev , German Gomez , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Dumazet , Dmitry Vyukov , Hao Luo Cc: eranian@google.com, Ian Rogers Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- 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 #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,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