Message-ID: <CAP-5=fWfLqk0naUJGRKBEN93_xy4SXSdjF24Z01FGnGhJf0pJQ@mail.gmail.com>
Date: Wed, 7 Aug 2024 09:03:41 -0700
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: "Liang, Kan" <kan.liang@...ux.intel.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Sun Haiyong <sunhaiyong@...ngson.cn>, 
	Yanteng Si <siyanteng@...ngson.cn>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf hist: Fix reference counting of branch_info

On Wed, Aug 7, 2024 at 8:42 AM Ian Rogers <irogers@...gle.com> wrote:
>
> On Wed, Aug 7, 2024 at 8:07 AM Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> >
> > On Wed, Aug 07, 2024 at 09:27:02AM -0400, Liang, Kan wrote:
> > >
> > >
> > > On 2024-08-07 2:51 a.m., Ian Rogers wrote:
> > > > iter_finish_branch_entry doesn't put the branch_info from/to map
> > > > elements creating memory leaks. This can be seen with:
> > > >
> > > > ```
> > > > $ perf record -e cycles -b perf test -w noploop
> > > > $ perf report -D
> > > > ...
> > > > Direct leak of 984344 byte(s) in 123043 object(s) allocated from:
> > > >     #0 0x7fb2654f3bd7 in malloc libsanitizer/asan/asan_malloc_linux.cpp:69
> > > >     #1 0x564d3400d10b in map__get util/map.h:186
> > > >     #2 0x564d3400d10b in ip__resolve_ams util/machine.c:1981
> > > >     #3 0x564d34014d81 in sample__resolve_bstack util/machine.c:2151
> > > >     #4 0x564d34094790 in iter_prepare_branch_entry util/hist.c:898
> > > >     #5 0x564d34098fa4 in hist_entry_iter__add util/hist.c:1238
> > > >     #6 0x564d33d1f0c7 in process_sample_event tools/perf/builtin-report.c:334
> > > >     #7 0x564d34031eb7 in perf_session__deliver_event util/session.c:1655
> > > >     #8 0x564d3403ba52 in do_flush util/ordered-events.c:245
> > > >     #9 0x564d3403ba52 in __ordered_events__flush util/ordered-events.c:324
> > > >     #10 0x564d3402d32e in perf_session__process_user_event util/session.c:1708
> > > >     #11 0x564d34032480 in perf_session__process_event util/session.c:1877
> > > >     #12 0x564d340336ad in reader__read_event util/session.c:2399
> > > >     #13 0x564d34033fdc in reader__process_events util/session.c:2448
> > > >     #14 0x564d34033fdc in __perf_session__process_events util/session.c:2495
> > > >     #15 0x564d34033fdc in perf_session__process_events util/session.c:2661
> > > >     #16 0x564d33d27113 in __cmd_report tools/perf/builtin-report.c:1065
> > > >     #17 0x564d33d27113 in cmd_report tools/perf/builtin-report.c:1805
> > > >     #18 0x564d33e0ccb7 in run_builtin tools/perf/perf.c:350
> > > >     #19 0x564d33e0d45e in handle_internal_command tools/perf/perf.c:403
> > > >     #20 0x564d33cdd827 in run_argv tools/perf/perf.c:447
> > > >     #21 0x564d33cdd827 in main tools/perf/perf.c:561
> > > > ...
> > > > ```
> > > >
> > > > Clearing up the map_symbols properly creates maps reference count
> > > > issues so resolve those. Resolving this issue doesn't improve peak
> > > > heap consumption for the test above.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > >
> > >
> > > Reviewed-by: Kan Liang <kan.liang@...ux.intel.com>
> >
> > Thanks, applying.
> >
> > While trying to test it:
> >
> > make -k CORESIGHT=1 EXTRA_CFLAGS="-fsanitize=memory" CC=clang HOSTCC=clang NO_LIBTRACEEVENT=1 NO_LIBELF=1 BUILD_BPF_SKEL=0 NO_LIBPFM=1 O=/tmp/build/$(basename $PWD)/ -C tools/perf install-bin
> >
> > Used that from a previous patch description from Ian to get rid of some
> > other problems with those other libraries:
> >
> > ⬢[acme@...lbox perf-tools-next]$  perf record -e cycles -b perf test -w noploop
> > Uninitialized bytes in fopen64 at offset 46 inside [0x7fff1077e890, 52)
> > ==1948231==WARNING: MemorySanitizer: use-of-uninitialized-value
> >     #0 0x7921df in perf_pmu_format__load pmu.c
> >     #1 0x791f3e in perf_pmu__warn_invalid_formats (/home/acme/bin/perf+0x791f3e) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #2 0x6f62d0 in __add_event parse-events.c
> >     #3 0x6fa681 in __parse_events_add_numeric parse-events.c
> >     #4 0x6fa3e4 in parse_events_add_numeric (/home/acme/bin/perf+0x6fa3e4) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #5 0x78c6ca in parse_events_parse (/home/acme/bin/perf+0x78c6ca) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #6 0x6fd8eb in __parse_events (/home/acme/bin/perf+0x6fd8eb) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #7 0x6ff232 in parse_events_option (/home/acme/bin/perf+0x6ff232) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #8 0x5be82f in get_value /home/acme/git/perf-tools-next/tools/lib/subcmd/parse-options.c
> >     #9 0x5ba474 in parse_short_opt /home/acme/git/perf-tools-next/tools/lib/subcmd/parse-options.c:351:11
> >     #10 0x5ba474 in parse_options_step /home/acme/git/perf-tools-next/tools/lib/subcmd/parse-options.c:539:12
> >     #11 0x5ba474 in parse_options_subcommand /home/acme/git/perf-tools-next/tools/lib/subcmd/parse-options.c:653:10
> >     #12 0x4f089f in cmd_record (/home/acme/bin/perf+0x4f089f) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #13 0x56fda9 in run_builtin perf.c
> >     #14 0x56e9ea in main (/home/acme/bin/perf+0x56e9ea) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >     #15 0x7fbf387ea087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
> >     #16 0x7fbf387ea14a in __libc_start_main@...BC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef)
> >     #17 0x4364e4 in _start (/home/acme/bin/perf+0x4364e4) (BuildId: d7742e31f05abb200493b431a6191afda9ed77c8)
> >
> > SUMMARY: MemorySanitizer: use-of-uninitialized-value pmu.c in perf_pmu_format__load
> > Exiting
> > ⬢[acme@...lbox perf-tools-next]$
> >
> > So I think there is something else nor merged or is this something new?
>
> Taking a look. You are setting HOSTCC here presumably to work past a
> libbpf build error I see. Why are we building libbpf with HOSTCC and
> not CC?

The memory sanitizer error is a false positive. The msan interceptor
hasn't worked for vfscanf so it doesn't understand that type was
written to as an out argument. We know it must have been as the result
of the vscanf is compared with 1 to make sure it was read:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n1105
To work around the false positive you can change "__u32 type;" to be
"__u32 type = 0;".

To diagnose this I added to EXTRA_CFLAGS
-fsanitize-memory-track-origins=2. This yielded:
```
==1069181==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x556c8f187881 in __perf_pmus__find_by_type tools/perf/util/pmus.c:245:7
    #1 0x556c8f186f6e in perf_pmus__find_by_type tools/perf/util/pmus.c:258:25
    #2 0x556c8ed2c3ea in parse_events_add_numeric
tools/perf/util/parse-events.c:1398:55
    #3 0x556c8f129795 in parse_events_parse tools/perf/util/parse-events.y:348:8
    #4 0x556c8ed3e5fe in parse_events__scanner
tools/perf/util/parse-events.c:1871:8
    #5 0x556c8ed3f85b in __parse_events tools/perf/util/parse-events.c:2140:8
    #6 0x556c8ed48553 in parse_events_option
tools/perf/util/parse-events.c:2343:8
    #7 0x556c8e6720cc in get_value tools/lib/subcmd/parse-options.c:251:10
    #8 0x556c8e668a63 in parse_short_opt tools/lib/subcmd/parse-options.c:351:11
    #9 0x556c8e65c55f in parse_options_step
tools/lib/subcmd/parse-options.c:539:12
    #10 0x556c8e659066 in parse_options_subcommand
tools/lib/subcmd/parse-options.c:653:10
    #11 0x556c8e65fd07 in parse_options tools/lib/subcmd/parse-options.c:691:9
    #12 0x556c8e07fe11 in cmd_record tools/perf/builtin-record.c:3995:9
    #13 0x556c8e5cf19b in run_builtin tools/perf/perf.c:350:11
    #14 0x556c8e5caa3a in handle_internal_command tools/perf/perf.c:403:8
    #15 0x556c8e5ce677 in run_argv tools/perf/perf.c:447:2
    #16 0x556c8e5c8e77 in main tools/perf/perf.c:561:3
    #17 0x7f4218443b89 in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #18 0x7f4218443c44 in __libc_start_main csu/../csu/libc-start.c:360:3

  Uninitialized value was stored to memory at
    #0 0x556c8f1447f9 in perf_pmu__lookup tools/perf/util/pmu.c:1123:12
    #1 0x556c8f198a68 in perf_pmu__find2 tools/perf/util/pmus.c:184:9
    #2 0x556c8f186b26 in pmu_read_sysfs tools/perf/util/pmus.c:223:3
    #3 0x556c8f18995b in perf_pmus__scan_core tools/perf/util/pmus.c:295:3
    #4 0x556c8f196347 in perf_pmus__num_core_pmus tools/perf/util/pmus.c:628:17
    #5 0x556c8f1999ad in __perf_pmus__supports_extended_type
tools/perf/util/pmus.c:638:6
    #6 0x556c8f196599 in perf_pmus__init_supports_extended_type
tools/perf/util/pmus.c:653:40
    #7 0x7f42184aa276 in __pthread_once_slow nptl/pthread_once.c:116:7

  Uninitialized value was created by an allocation of 'type' in the stack frame
    #0 0x556c8f143d50 in perf_pmu__lookup tools/perf/util/pmu.c:1091:2

SUMMARY: MemorySanitizer: use-of-uninitialized-value
tools/perf/util/pmus.c:245:7 in __perf_pmus__find_by_type
    #0 0x556c8f143d50 in perf_pmu__lookup
/home/irogers/kernel.org2/tools/perf/util/pmu.c:1091:2

SUMMARY: MemorySanitizer: use-of-uninitialized-value
/home/irogers/kernel.org2/tools/perf/util/pmus.c:245:7 in
__perf_pmus__find_by_type
```

As I say, there's no way for type to be uninitialized, the vscanf
ensures it, so this is an msan interceptor issue. Fwiw, it can be
difficult to use msan without fully recompiling things like libc, this
is how I test perf inside of Google's monorepo with msan. Address
sanitizer, that includes leak sanitizer, is less fussy but it lacks
interceptors for things like the BPF system call. Ideally distros
would ship sanitizer builds of things like libc in the same way they
ship developer/debug packages.

Thanks,
Ian

Powered by blists - more mailing lists