Commit 8f12692b authored by Ian Rogers's avatar Ian Rogers Committed by Arnaldo Carvalho de Melo
Browse files

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: default avatarIan 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: default avatarArnaldo Carvalho de Melo <acme@redhat.com>
parent a07dacad

tools/perf/tests/thread-maps-share.c

+4 −4
Original line number Diff line number Diff line
@@ -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);

tools/perf/util/maps.c

+26 −21
Original line number Diff line number Diff line
@@ -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)

tools/perf/util/maps.h

+10 −9
Original line number Diff line number Diff line
@@ -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

tools/perf/util/symbol.c

+7 −6
Original line number Diff line number Diff line
@@ -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;

		}

	}

tools/perf/util/unwind-libdw.c

+1 −1
Original line number Diff line number Diff line
@@ -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,