Re: [PATCH v4 17/22] perf map: Changes to reference counting

[Adrian Hunter <adrian.hunter@intel.com>]

On 20/03/23 05:38, Ian Rogers wrote:
> When a pointer to a map exists do a get, when that pointer is
> overwritten or freed, put the map. This avoids issues with gets and
> puts being inconsistently used causing, use after puts, etc. For
> example, the map in struct addr_location is changed to hold a
> reference count.

I am not sure I understand the reason for that.  A thread's address
mappings are inherently transitory with respect to perf (MMAP) event
processing.  Holding on to a reference to a thread's map is not a
valid thing to do if more perf events can be processed in the meantime.
Reference counting seems to hide the problem in that case, since
the solution is: don't keep the reference.

>                  Reference count checking and address sanitizer were
> used to identify issues.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/code-reading.c       |  1 +
>  tools/perf/tests/hists_cumulate.c     | 10 ++++
>  tools/perf/tests/hists_filter.c       | 10 ++++
>  tools/perf/tests/hists_link.c         | 18 +++++-
>  tools/perf/tests/hists_output.c       | 10 ++++
>  tools/perf/tests/mmap-thread-lookup.c |  1 +
>  tools/perf/util/callchain.c           |  9 +--
>  tools/perf/util/event.c               |  8 ++-
>  tools/perf/util/hist.c                | 10 ++--
>  tools/perf/util/machine.c             | 79 ++++++++++++++++-----------
>  tools/perf/util/map.c                 |  2 +-
>  11 files changed, 114 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 1545fcaa95c6..efe026a35010 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -366,6 +366,7 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
>  	}
>  	pr_debug("Bytes read match those read by objdump\n");
>  out:
> +	map__put(al.map);

Given that it is 'al' that is going away, wouldn't it be
more logical to have here:

	addr_location__exit(&al);

where:

void addr_location__exit(struct addr_location *al)
{
	if (al->map) {
		map__put(al->map);
		al->map = NULL;
	}
}


>  	return err;
>  }
>  
> diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
> index f00ec9abdbcd..8c0e3f334747 100644
> --- a/tools/perf/tests/hists_cumulate.c
> +++ b/tools/perf/tests/hists_cumulate.c
> @@ -112,6 +112,7 @@ static int add_hist_entries(struct hists *hists, struct machine *machine)
>  		}
>  
>  		fake_samples[i].thread = al.thread;
> +		map__put(fake_samples[i].map);
>  		fake_samples[i].map = al.map;
>  		fake_samples[i].sym = al.sym;
>  	}
> @@ -147,6 +148,14 @@ static void del_hist_entries(struct hists *hists)
>  	}
>  }
>  
> +static void put_fake_samples(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fake_samples); i++)
> +		map__put(fake_samples[i].map);
> +}
> +
>  typedef int (*test_fn_t)(struct evsel *, struct machine *);
>  
>  #define COMM(he)  (thread__comm_str(he->thread))
> @@ -733,6 +742,7 @@ static int test__hists_cumulate(struct test_suite *test __maybe_unused, int subt
>  	/* tear down everything */
>  	evlist__delete(evlist);
>  	machines__exit(&machines);
> +	put_fake_samples();
>  
>  	return err;
>  }
> diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
> index 7c552549f4a4..98eff5935a1c 100644
> --- a/tools/perf/tests/hists_filter.c
> +++ b/tools/perf/tests/hists_filter.c
> @@ -89,6 +89,7 @@ static int add_hist_entries(struct evlist *evlist,
>  			}
>  
>  			fake_samples[i].thread = al.thread;
> +			map__put(fake_samples[i].map);
>  			fake_samples[i].map = al.map;
>  			fake_samples[i].sym = al.sym;
>  		}
> @@ -101,6 +102,14 @@ static int add_hist_entries(struct evlist *evlist,
>  	return TEST_FAIL;
>  }
>  
> +static void put_fake_samples(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fake_samples); i++)
> +		map__put(fake_samples[i].map);
> +}
> +
>  static int test__hists_filter(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>  {
>  	int err = TEST_FAIL;
> @@ -322,6 +331,7 @@ static int test__hists_filter(struct test_suite *test __maybe_unused, int subtes
>  	evlist__delete(evlist);
>  	reset_output_field();
>  	machines__exit(&machines);
> +	put_fake_samples();
>  
>  	return err;
>  }
> diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
> index e7e4ee57ce04..64ce8097889c 100644
> --- a/tools/perf/tests/hists_link.c
> +++ b/tools/perf/tests/hists_link.c
> @@ -6,6 +6,7 @@
>  #include "evsel.h"
>  #include "evlist.h"
>  #include "machine.h"
> +#include "map.h"
>  #include "parse-events.h"
>  #include "hists_common.h"
>  #include "util/mmap.h"
> @@ -94,6 +95,7 @@ static int add_hist_entries(struct evlist *evlist, struct machine *machine)
>  			}
>  
>  			fake_common_samples[k].thread = al.thread;
> +			map__put(fake_common_samples[k].map);
>  			fake_common_samples[k].map = al.map;
>  			fake_common_samples[k].sym = al.sym;
>  		}
> @@ -126,11 +128,24 @@ static int add_hist_entries(struct evlist *evlist, struct machine *machine)
>  	return -1;
>  }
>  
> +static void put_fake_samples(void)
> +{
> +	size_t i, j;
> +
> +	for (i = 0; i < ARRAY_SIZE(fake_common_samples); i++)
> +		map__put(fake_common_samples[i].map);
> +	for (i = 0; i < ARRAY_SIZE(fake_samples); i++) {
> +		for (j = 0; j < ARRAY_SIZE(fake_samples[0]); j++)
> +			map__put(fake_samples[i][j].map);
> +	}
> +}
> +
>  static int find_sample(struct sample *samples, size_t nr_samples,
>  		       struct thread *t, struct map *m, struct symbol *s)
>  {
>  	while (nr_samples--) {
> -		if (samples->thread == t && samples->map == m &&
> +		if (samples->thread == t &&
> +		    samples->map == m &&
>  		    samples->sym == s)
>  			return 1;
>  		samples++;
> @@ -336,6 +351,7 @@ static int test__hists_link(struct test_suite *test __maybe_unused, int subtest
>  	evlist__delete(evlist);
>  	reset_output_field();
>  	machines__exit(&machines);
> +	put_fake_samples();
>  
>  	return err;
>  }
> diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
> index 428d11a938f2..cebd5226bb12 100644
> --- a/tools/perf/tests/hists_output.c
> +++ b/tools/perf/tests/hists_output.c
> @@ -78,6 +78,7 @@ static int add_hist_entries(struct hists *hists, struct machine *machine)
>  		}
>  
>  		fake_samples[i].thread = al.thread;
> +		map__put(fake_samples[i].map);
>  		fake_samples[i].map = al.map;
>  		fake_samples[i].sym = al.sym;
>  	}
> @@ -113,6 +114,14 @@ static void del_hist_entries(struct hists *hists)
>  	}
>  }
>  
> +static void put_fake_samples(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fake_samples); i++)
> +		map__put(fake_samples[i].map);
> +}
> +
>  typedef int (*test_fn_t)(struct evsel *, struct machine *);
>  
>  #define COMM(he)  (thread__comm_str(he->thread))
> @@ -620,6 +629,7 @@ static int test__hists_output(struct test_suite *test __maybe_unused, int subtes
>  	/* tear down everything */
>  	evlist__delete(evlist);
>  	machines__exit(&machines);
> +	put_fake_samples();
>  
>  	return err;
>  }
> diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
> index 5cc4644e353d..898eda55b7a8 100644
> --- a/tools/perf/tests/mmap-thread-lookup.c
> +++ b/tools/perf/tests/mmap-thread-lookup.c
> @@ -203,6 +203,7 @@ static int mmap_events(synth_cb synth)
>  		}
>  
>  		pr_debug("map %p, addr %" PRIx64 "\n", al.map, map__start(al.map));
> +		map__put(al.map);
>  	}
>  
>  	machine__delete_threads(machine);
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 9e9c39dd9d2b..78dc7b6f7ff7 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -589,7 +589,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
>  		}
>  		call->ip = cursor_node->ip;
>  		call->ms = cursor_node->ms;
> -		map__get(call->ms.map);
> +		call->ms.map = map__get(call->ms.map);
>  		call->srcline = cursor_node->srcline;
>  
>  		if (cursor_node->branch) {
> @@ -1067,7 +1067,7 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
>  	node->ip = ip;
>  	map__zput(node->ms.map);
>  	node->ms = *ms;
> -	map__get(node->ms.map);
> +	node->ms.map = map__get(node->ms.map);
>  	node->branch = branch;
>  	node->nr_loop_iter = nr_loop_iter;
>  	node->iter_cycles = iter_cycles;
> @@ -1115,7 +1115,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
>  	struct machine *machine = maps__machine(node->ms.maps);
>  
>  	al->maps = node->ms.maps;
> -	al->map = node->ms.map;
> +	map__put(al->map);
> +	al->map = map__get(node->ms.map);
>  	al->sym = node->ms.sym;
>  	al->srcline = node->srcline;
>  	al->addr = node->ip;
> @@ -1528,7 +1529,7 @@ int callchain_node__make_parent_list(struct callchain_node *node)
>  				goto out;
>  			*new = *chain;
>  			new->has_children = false;
> -			map__get(new->ms.map);
> +			new->ms.map = map__get(new->ms.map);
>  			list_add_tail(&new->list, &head);
>  		}
>  		parent = parent->parent;
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 2712d1a8264e..8293c8a3406b 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -485,13 +485,14 @@ size_t perf_event__fprintf_text_poke(union perf_event *event, struct machine *ma
>  	if (machine) {
>  		struct addr_location al;
>  
> -		al.map = maps__find(machine__kernel_maps(machine), tp->addr);
> +		al.map = map__get(maps__find(machine__kernel_maps(machine), tp->addr));
>  		if (al.map && map__load(al.map) >= 0) {
>  			al.addr = map__map_ip(al.map, tp->addr);
>  			al.sym = map__find_symbol(al.map, al.addr);
>  			if (al.sym)
>  				ret += symbol__fprintf_symname_offs(al.sym, &al, fp);
>  		}
> +		map__put(al.map);
>  	}
>  	ret += fprintf(fp, " old len %u new len %u\n", tp->old_len, tp->new_len);
>  	old = true;
> @@ -582,6 +583,7 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  	al->filtered = 0;
>  
>  	if (machine == NULL) {
> +		map__put(al->map);

Often as not, addr_location is not initialized before a call
to thread__find_map(), so this is dereferencing an uninitialized
pointer in those cases.

>  		al->map = NULL;
>  		return NULL;
>  	}
> @@ -600,6 +602,7 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  		al->level = 'u';
>  	} else {
>  		al->level = 'H';
> +		map__put(al->map);

As above

>  		al->map = NULL;
>  
>  		if ((cpumode == PERF_RECORD_MISC_GUEST_USER ||
> @@ -614,7 +617,7 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  		return NULL;
>  	}
>  
> -	al->map = maps__find(maps, al->addr);
> +	al->map = map__get(maps__find(maps, al->addr));

The map__put() above suggest there should one here before
overwriting al->map?

In general, it looks like there are callers of thread__find_map()
that are not covered by this patch?

>  	if (al->map != NULL) {
>  		/*
>  		 * Kernel maps might be changed when loading symbols so loading
> @@ -773,6 +776,7 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
>   */
>  void addr_location__put(struct addr_location *al)
>  {
> +	map__zput(al->map);
>  	thread__zput(al->thread);
>  }
>  
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index fdf0562d2fd3..02b4bf31b1a7 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -450,7 +450,7 @@ static int hist_entry__init(struct hist_entry *he,
>  			memset(&he->stat, 0, sizeof(he->stat));
>  	}
>  
> -	map__get(he->ms.map);
> +	he->ms.map = map__get(he->ms.map);
>  
>  	if (he->branch_info) {
>  		/*
> @@ -465,13 +465,13 @@ static int hist_entry__init(struct hist_entry *he,
>  		memcpy(he->branch_info, template->branch_info,
>  		       sizeof(*he->branch_info));
>  
> -		map__get(he->branch_info->from.ms.map);
> -		map__get(he->branch_info->to.ms.map);
> +		he->branch_info->from.ms.map = map__get(he->branch_info->from.ms.map);
> +		he->branch_info->to.ms.map = map__get(he->branch_info->to.ms.map);
>  	}
>  
>  	if (he->mem_info) {
> -		map__get(he->mem_info->iaddr.ms.map);
> -		map__get(he->mem_info->daddr.ms.map);
> +		he->mem_info->iaddr.ms.map = map__get(he->mem_info->iaddr.ms.map);
> +		he->mem_info->daddr.ms.map = map__get(he->mem_info->daddr.ms.map);
>  	}
>  
>  	if (hist_entry__has_callchains(he) && symbol_conf.use_callchain)
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 916d98885128..502e97010a3c 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -880,21 +880,29 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  	struct symbol *sym;
>  	struct dso *dso;
>  	struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> +	bool put_map = false;
> +	int err = 0;
>  
>  	if (!map) {
> -		int err;
> -
>  		dso = dso__new(event->ksymbol.name);
> -		if (dso) {
> -			dso->kernel = DSO_SPACE__KERNEL;
> -			map = map__new2(0, dso);
> -			dso__put(dso);
> -		}
>  
> -		if (!dso || !map) {
> -			return -ENOMEM;
> +		if (!dso) {
> +			err = -ENOMEM;
> +			goto out;
>  		}
> -
> +		dso->kernel = DSO_SPACE__KERNEL;
> +		map = map__new2(0, dso);
> +		dso__put(dso);
> +		if (!map) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +		/*
> +		 * The inserted map has a get on it, we need to put to release
> +		 * the reference count here, but do it after all accesses are
> +		 * done.
> +		 */
> +		put_map = true;
>  		if (event->ksymbol.ksym_type == PERF_RECORD_KSYMBOL_TYPE_OOL) {
>  			dso->binary_type = DSO_BINARY_TYPE__OOL;
>  			dso->data.file_size = event->ksymbol.len;
> @@ -904,9 +912,10 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  		map->start = event->ksymbol.addr;
>  		map->end = map__start(map) + event->ksymbol.len;
>  		err = maps__insert(machine__kernel_maps(machine), map);
> -		map__put(map);
> -		if (err)
> -			return err;
> +		if (err) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
>  
>  		dso__set_loaded(dso);
>  
> @@ -921,10 +930,15 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  	sym = symbol__new(map__map_ip(map, map__start(map)),
>  			  event->ksymbol.len,
>  			  0, 0, event->ksymbol.name);
> -	if (!sym)
> -		return -ENOMEM;
> +	if (!sym) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>  	dso__insert_symbol(dso, sym);
> -	return 0;
> +out:
> +	if (put_map)
> +		map__put(map);
> +	return err;
>  }
>  
>  static int machine__process_ksymbol_unregister(struct machine *machine,
> @@ -1026,13 +1040,11 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
>  		goto out;
>  
>  	err = maps__insert(machine__kernel_maps(machine), map);
> -
> -	/* Put the map here because maps__insert already got it */
> -	map__put(map);
> -
>  	/* If maps__insert failed, return NULL. */
> -	if (err)
> +	if (err) {
> +		map__put(map);
>  		map = NULL;
> +	}
>  out:
>  	/* put the dso here, corresponding to  machine__findnew_module_dso */
>  	dso__put(dso);
> @@ -1324,6 +1336,7 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
>  	/* In case of renewal the kernel map, destroy previous one */
>  	machine__destroy_kernel_maps(machine);
>  
> +	map__put(machine->vmlinux_map);
>  	machine->vmlinux_map = map__new2(0, kernel);
>  	if (machine->vmlinux_map == NULL)
>  		return -ENOMEM;
> @@ -1612,7 +1625,7 @@ static int machine__create_module(void *arg, const char *name, u64 start,
>  	map->end = start + size;
>  
>  	dso__kernel_module_get_build_id(map__dso(map), machine->root_dir);
> -
> +	map__put(map);
>  	return 0;
>  }
>  
> @@ -1658,16 +1671,18 @@ static void machine__set_kernel_mmap(struct machine *machine,
>  static int machine__update_kernel_mmap(struct machine *machine,
>  				     u64 start, u64 end)
>  {
> -	struct map *map = machine__kernel_map(machine);
> +	struct map *orig, *updated;
>  	int err;
>  
> -	map__get(map);
> -	maps__remove(machine__kernel_maps(machine), map);
> +	orig = machine->vmlinux_map;
> +	updated = map__get(orig);
>  
> +	machine->vmlinux_map = updated;
>  	machine__set_kernel_mmap(machine, start, end);
> +	maps__remove(machine__kernel_maps(machine), orig);
> +	err = maps__insert(machine__kernel_maps(machine), updated);
> +	map__put(orig);
>  
> -	err = maps__insert(machine__kernel_maps(machine), map);
> -	map__put(map);
>  	return err;
>  }
>  
> @@ -2294,7 +2309,7 @@ static int add_callchain_ip(struct thread *thread,
>  {
>  	struct map_symbol ms;
>  	struct addr_location al;
> -	int nr_loop_iter = 0;
> +	int nr_loop_iter = 0, err;
>  	u64 iter_cycles = 0;
>  	const char *srcline = NULL;
>  
> @@ -2355,9 +2370,11 @@ static int add_callchain_ip(struct thread *thread,
>  	ms.map = al.map;
>  	ms.sym = al.sym;
>  	srcline = callchain_srcline(&ms, al.addr);
> -	return callchain_cursor_append(cursor, ip, &ms,
> -				       branch, flags, nr_loop_iter,
> -				       iter_cycles, branch_from, srcline);
> +	err = callchain_cursor_append(cursor, ip, &ms,
> +				      branch, flags, nr_loop_iter,
> +				      iter_cycles, branch_from, srcline);
> +	map__put(al.map);
> +	return err;
>  }
>  
>  struct branch_info *sample__resolve_bstack(struct perf_sample *sample,
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 1fe367e2cf19..acbc37359e06 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -410,7 +410,7 @@ struct map *map__clone(struct map *from)
>  	map = memdup(from, size);
>  	if (map != NULL) {
>  		refcount_set(&map->refcnt, 1);
> -		dso__get(dso);
> +		map->dso = dso__get(dso);
>  	}
>  
>  	return map;