Messages in this thread Patch in this message | | | Date | Mon, 20 Mar 2023 13:18:44 +0200 | From | Adrian Hunter <> | Subject | Re: [PATCH v4 17/22] perf map: Changes to reference counting |
| |
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;
| |