lkml.org 
[lkml]   [2023]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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;

    \
     
     \ /
      Last update: 2023-03-27 01:08    [W:2.492 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site