| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <32654889-2e76-1a09-acef-e9b4378f855f@intel.com>
Date: Mon, 20 Mar 2023 13:18:44 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Ian Rogers <irogers@...gle.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Darren Hart <dvhart@...radead.org>,
Davidlohr Bueso <dave@...olabs.net>,
André Almeida <andrealmeid@...labora.com>,
James Clark <james.clark@....com>,
John Garry <john.g.garry@...cle.com>,
Riccardo Mancini <rickyman7@...il.com>,
Yury Norov <yury.norov@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Leo Yan <leo.yan@...aro.org>, Andi Kleen <ak@...ux.intel.com>,
Thomas Richter <tmricht@...ux.ibm.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Madhavan Srinivasan <maddy@...ux.ibm.com>,
Shunsuke Nakamura <nakamura.shun@...itsu.com>,
Song Liu <song@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Miaoqian Lin <linmq006@...il.com>,
Stephen Brennan <stephen.s.brennan@...cle.com>,
Kajol Jain <kjain@...ux.ibm.com>,
Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
German Gomez <german.gomez@....com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>, Hao Luo <haoluo@...gle.com>
Cc: Stephane Eranian <eranian@...gle.com>
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@...gle.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;
Powered by blists - more mailing lists