From nobody Tue Feb 10 02:59:38 2026 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 7D9D4C77B6C for ; Fri, 7 Apr 2023 23:05:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229580AbjDGXF2 (ORCPT ); Fri, 7 Apr 2023 19:05:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58554 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229561AbjDGXFZ (ORCPT ); Fri, 7 Apr 2023 19:05:25 -0400 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 4394FE059 for ; Fri, 7 Apr 2023 16:05:07 -0700 (PDT) Received: by mail-yb1-xb49.google.com with SMTP id f66-20020a255145000000b00b714602d43fso43848673ybb.10 for ; Fri, 07 Apr 2023 16:05:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680908706; h=cc:to:from:subject:references:mime-version:message-id:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=/9OxK9O+214Ftf+emLqWQX0BdscBsNuKoaQhFz4awdI=; b=LIOvfvs+TGmGTVeJEWXBv464qGMyynjYkPyh9q1i4FcTu+BbqB14qBQYVxZo/idxYT MwwOZ9Q9cfrvg35DFin1M4Mku/iF6o1hY8v24+8D/pLnNgGz+Phk9dMxdPp4EBF/3R3m Q2kbsBxhikq0RgsMSemv9AuKVs/zXZw1Zeii/vtYjsKWL65jHp0yo5rtb8q2WqtYooNZ wlRpmTDg6cZnaFMm+ysN8/e56b1ItP1O0lsFJimB2dQy3UyCTb2lTR/4MKzO/cxyvxPu Se1YG/EPWFvI+7XogijDDmBo+azmNIZ8ugvLk3qvwppfe9iKewMAnF+CTJvqknXKRAUA gCfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680908706; h=cc:to:from:subject:references:mime-version:message-id:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/9OxK9O+214Ftf+emLqWQX0BdscBsNuKoaQhFz4awdI=; b=jFd+b6meKUaOg1s08TWQ2xeJzXy1e+DhHe6VuQ3BEVc6NVSXyToMi6Hj/f9oE9kkcL DW9he3vGvTbomLwgExiAF5Be6IX8bZqUBufEn2kFJQfFsQPyr+de78B0ND4qrTnxDfyB X1w4bV/ETD1H6pj6Wm6jSISfTZj5O285arfNeWbEM6G2u67UtioGsw0/d5WqcDEhoP23 XV8VzCytaPWyAk1nMgtGbkG5Z1nu3FFCYfVJe90DreO99QXdynCAGwGLc57/Xu2Zf2Q7 HXKTl1niMMLxcWeTjHFgmxiQeB1fyAe5K8fnqjCh69bUQj+P+XOJrPSNhHnax6IwVhS/ 12qQ== X-Gm-Message-State: AAQBX9c95pJXh4CQfC8GWcGqe/Zj19tsKbKxMtESIyZIZGPWx7BZ/gm1 z40AeJfSnPtSyvc9rCNqXY3aarizqD+u X-Google-Smtp-Source: AKy350bqU/u3LkAM2Zl5P0gS5bWJTxR36K5/ojhBUTzBLhsfw+rRpaGZFuULjQPUqBp12wO0Dl7A0Tnbef8+ X-Received: from irogers.svl.corp.google.com ([2620:15c:2d4:203:b240:9cdf:7861:b23e]) (user=irogers job=sendgmr) by 2002:a81:eb12:0:b0:545:1d7f:acbf with SMTP id n18-20020a81eb12000000b005451d7facbfmr84341ywm.10.1680908706095; Fri, 07 Apr 2023 16:05:06 -0700 (PDT) Date: Fri, 7 Apr 2023 16:04:04 -0700 In-Reply-To: <20230407230405.2931830-1-irogers@google.com> Message-Id: <20230407230405.2931830-5-irogers@google.com> Mime-Version: 1.0 References: <20230407230405.2931830-1-irogers@google.com> X-Mailer: git-send-email 2.40.0.577.gac1e443424-goog Subject: [PATCH v7 4/5] 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 , James Clark , John Garry , Riccardo Mancini , Yury Norov , Andy Shevchenko , Andrew Morton , 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: Stephane Eranian , Ian Rogers Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" 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/machine.c | 2 +- tools/perf/util/maps.c | 53 +++++++++++++----------- tools/perf/util/maps.h | 17 ++++---- 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 +- 8 files changed, 64 insertions(+), 56 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 *t= est __maybe_unused, int s leader && t1 && t2 && t3 && other); =20 maps =3D leader->maps; - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 4); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->ref= cnt), 4); =20 /* test the maps pointer is shared */ - TEST_ASSERT_VAL("maps don't match", maps =3D=3D t1->maps); - TEST_ASSERT_VAL("maps don't match", maps =3D=3D t2->maps); - TEST_ASSERT_VAL("maps don't match", maps =3D=3D t3->maps); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) =3D=3D RC_CHK_ACC= ESS(t1->maps)); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) =3D=3D RC_CHK_ACC= ESS(t2->maps)); + TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(maps) =3D=3D RC_CHK_ACC= ESS(t3->maps)); =20 /* * Verify the other leader was created by previous call. @@ -71,25 +71,26 @@ static int test__thread_maps_share(struct test_suite *t= est __maybe_unused, int s machine__remove_thread(machine, other_leader); =20 other_maps =3D 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); =20 - TEST_ASSERT_VAL("maps don't match", other_maps =3D=3D other_leader->maps); + TEST_ASSERT_VAL("maps don't match", + RC_CHK_ACCESS(other_maps) =3D=3D RC_CHK_ACCESS(other_leader->maps)); =20 /* 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)->ref= cnt), 3); =20 thread__put(t2); - TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&maps->refcnt), 1); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->ref= cnt), 2); =20 - thread__put(t3); + thread__put(t1); + TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(&RC_CHK_ACCESS(maps)->ref= cnt), 1); + + thread__put(leader); =20 /* 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); =20 thread__put(other); =20 diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 25738775834e..2d9ce6966238 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -435,7 +435,7 @@ static struct thread *findnew_guest_code(struct machine= *machine, return NULL; =20 /* Assume maps are set up if there are any */ - if (thread->maps->nr_maps) + if (RC_CHK_ACCESS(thread->maps)->nr_maps) return thread; =20 host_thread =3D machine__find_thread(host_machine, -1, pid); diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c index 5afed53ea0b4..567952587247 100644 --- a/tools/perf/util/maps.c +++ b/tools/perf/util/maps.c @@ -12,13 +12,13 @@ =20 static void maps__init(struct maps *maps, struct machine *machine) { - maps->entries =3D RB_ROOT; + RC_CHK_ACCESS(maps)->entries =3D RB_ROOT; init_rwsem(maps__lock(maps)); - maps->machine =3D machine; - maps->last_search_by_name =3D NULL; - maps->nr_maps =3D 0; - maps->maps_by_name =3D NULL; - refcount_set(&maps->refcnt, 1); + RC_CHK_ACCESS(maps)->machine =3D machine; + RC_CHK_ACCESS(maps)->last_search_by_name =3D NULL; + RC_CHK_ACCESS(maps)->nr_maps =3D 0; + RC_CHK_ACCESS(maps)->maps_by_name =3D NULL; + refcount_set(&RC_CHK_ACCESS(maps)->refcnt, 1); } =20 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 =3D 0; i < maps__nr_maps(maps); i++) map__put(maps__maps_by_name(maps)[i]); =20 - zfree(&maps->maps_by_name); - maps->nr_maps_allocated =3D 0; + zfree(&RC_CHK_ACCESS(maps)->maps_by_name); + RC_CHK_ACCESS(maps)->nr_maps_allocated =3D 0; } =20 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; =20 - ++maps->nr_maps; + ++RC_CHK_ACCESS(maps)->nr_maps; =20 if (dso && dso->kernel) { struct kmap *kmap =3D 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 =3D maps__nr_maps(maps) * 2; struct map **maps_by_name =3D 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; } =20 - maps->maps_by_name =3D maps_by_name; - maps->nr_maps_allocated =3D nr_allocate; + RC_CHK_ACCESS(maps)->maps_by_name =3D maps_by_name; + RC_CHK_ACCESS(maps)->nr_maps_allocated =3D nr_allocate; } maps__maps_by_name(maps)[maps__nr_maps(maps) - 1] =3D 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; =20 down_write(maps__lock(maps)); - if (maps->last_search_by_name =3D=3D map) - maps->last_search_by_name =3D NULL; + if (RC_CHK_ACCESS(maps)->last_search_by_name =3D=3D map) + RC_CHK_ACCESS(maps)->last_search_by_name =3D NULL; =20 rb_node =3D maps__find_node(maps, map); assert(rb_node->map =3D=3D 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)); } =20 @@ -162,33 +162,38 @@ bool maps__empty(struct maps *maps) =20 struct maps *maps__new(struct machine *machine) { - struct maps *maps =3D zalloc(sizeof(*maps)); + struct maps *res; + RC_STRUCT(maps) *maps =3D zalloc(sizeof(*maps)); =20 - if (maps !=3D NULL) - maps__init(maps, machine); + if (ADD_RC_CHK(res, maps)) + maps__init(res, machine); =20 - return maps; + return res; } =20 void maps__delete(struct maps *maps) { maps__exit(maps); unwind__finish_access(maps); - free(maps); + RC_CHK_FREE(maps); } =20 struct maps *maps__get(struct maps *maps) { - if (maps) - refcount_inc(&maps->refcnt); + struct maps *result; =20 - return maps; + if (RC_CHK_GET(result, maps)) + refcount_inc(&RC_CHK_ACCESS(maps)->refcnt); + + return result; } =20 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); } =20 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 =20 struct ref_reloc_sym; struct machine; @@ -32,7 +33,7 @@ struct map *maps__find(struct maps *maps, u64 addr); for (map =3D maps__first(maps), next =3D map_rb_node__next(map); map; \ map =3D next, next =3D map_rb_node__next(map)) =20 -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); =20 static inline struct rb_root *maps__entries(struct maps *maps) { - return &maps->entries; + return &RC_CHK_ACCESS(maps)->entries; } =20 static inline struct machine *maps__machine(struct maps *maps) { - return maps->machine; + return RC_CHK_ACCESS(maps)->machine; } =20 static inline struct rw_semaphore *maps__lock(struct maps *maps) { - return &maps->lock; + return &RC_CHK_ACCESS(maps)->lock; } =20 static inline struct map **maps__maps_by_name(struct maps *maps) { - return maps->maps_by_name; + return RC_CHK_ACCESS(maps)->maps_by_name; } =20 static inline unsigned int maps__nr_maps(const struct maps *maps) { - return maps->nr_maps; + return RC_CHK_ACCESS(maps)->nr_maps; } =20 #ifdef HAVE_LIBUNWIND_SUPPORT static inline void *maps__addr_space(struct maps *maps) { - return maps->addr_space; + return RC_CHK_ACCESS(maps)->addr_space; } =20 static inline const struct unwind_libunwind_ops *maps__unwind_libunwind_op= s(const struct maps *maps) { - return maps->unwind_libunwind_ops; + return RC_CHK_ACCESS(maps)->unwind_libunwind_ops; } #endif =20 diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c index 639343d5577c..6993b51b9416 100644 --- a/tools/perf/util/symbol.c +++ b/tools/perf/util/symbol.c @@ -2097,8 +2097,8 @@ static int map__groups__sort_by_name_from_rbtree(stru= ct maps *maps) up_read(maps__lock(maps)); down_write(maps__lock(maps)); =20 - maps->maps_by_name =3D maps_by_name; - maps->nr_maps_allocated =3D maps__nr_maps(maps); + RC_CHK_ACCESS(maps)->maps_by_name =3D maps_by_name; + RC_CHK_ACCESS(maps)->nr_maps_allocated =3D maps__nr_maps(maps); =20 maps__for_each_entry(maps, rb_node) maps_by_name[i++] =3D map__get(rb_node->map); @@ -2133,11 +2133,12 @@ struct map *maps__find_by_name(struct maps *maps, c= onst char *name) =20 down_read(maps__lock(maps)); =20 - if (maps->last_search_by_name) { - const struct dso *dso =3D map__dso(maps->last_search_by_name); + + if (RC_CHK_ACCESS(maps)->last_search_by_name) { + const struct dso *dso =3D map__dso(RC_CHK_ACCESS(maps)->last_search_by_n= ame); =20 if (strcmp(dso->short_name, name) =3D=3D 0) { - map =3D maps->last_search_by_name; + map =3D RC_CHK_ACCESS(maps)->last_search_by_name; goto out_unlock; } } @@ -2157,7 +2158,7 @@ struct map *maps__find_by_name(struct maps *maps, con= st char *name) map =3D rb_node->map; dso =3D map__dso(map); if (strcmp(dso->short_name, name) =3D=3D 0) { - maps->last_search_by_name =3D map; + RC_CHK_ACCESS(maps)->last_search_by_name =3D 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 =3D { .sample =3D data, .thread =3D thread, - .machine =3D thread->maps->machine, + .machine =3D RC_CHK_ACCESS(thread->maps)->machine, .cb =3D cb, .arg =3D arg, .max_stack =3D max_stack, diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unw= ind-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 =3D unw_create_addr_space(&accessors, 0); =20 - maps->addr_space =3D addr_space; + RC_CHK_ACCESS(maps)->addr_space =3D 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-li= bunwind.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_libunwin= d_ops; =20 static void unwind__register_ops(struct maps *maps, struct unwind_libunwin= d_ops *ops) { - maps->unwind_libunwind_ops =3D ops; + RC_CHK_ACCESS(maps)->unwind_libunwind_ops =3D ops; } =20 int unwind__prepare_access(struct maps *maps, struct map *map, bool *initi= alized) --=20 2.40.0.577.gac1e443424-goog