linux-kernel - Re: [PATCH v2 0/6] maps memory improvements and fixes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUVkaq3dDoeMYYEN1N-ghnL-GiP8PV3N3pWpjQKpDTCHw@mail.gmail.com>
Date: Fri, 9 Feb 2024 18:46:31 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: 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>, 
	Adrian Hunter <adrian.hunter@...el.com>, Song Liu <song@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Liam Howlett <liam.howlett@...cle.com>, 
	Colin Ian King <colin.i.king@...il.com>, K Prateek Nayak <kprateek.nayak@....com>, 
	Artem Savkov <asavkov@...hat.com>, Changbin Du <changbin.du@...wei.com>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, 
	Yang Jihong <yangjihong1@...wei.com>, Tiezhu Yang <yangtiezhu@...ngson.cn>, 
	James Clark <james.clark@....com>, Leo Yan <leo.yan@...aro.org>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [PATCH v2 0/6] maps memory improvements and fixes

On Thu, Feb 8, 2024 at 9:44 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hi Ian,
>
> On Wed, Feb 7, 2024 at 2:37 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > First 6 patches from:
> > https://lore.kernel.org/lkml/20240202061532.1939474-1-irogers@google.com/
> >
> > v2. Fix NO_LIBUNWIND=1 build issue.
> >
> > Ian Rogers (6):
> >   perf maps: Switch from rbtree to lazily sorted array for addresses
> >   perf maps: Get map before returning in maps__find
> >   perf maps: Get map before returning in maps__find_by_name
> >   perf maps: Get map before returning in maps__find_next_entry
> >   perf maps: Hide maps internals
> >   perf maps: Locking tidy up of nr_maps
>
> Now I see a perf test failure on the vmlinux test:
>
> $ sudo ./perf test -v vmlinux
>   1: vmlinux symtab matches kallsyms                                 :
> --- start ---
> test child forked, pid 4164115
> /proc/{kallsyms,modules} inconsistency while looking for
> "[__builtin__kprobes]" module!
> /proc/{kallsyms,modules} inconsistency while looking for
> "[__builtin__kprobes]" module!
> /proc/{kallsyms,modules} inconsistency while looking for
> "[__builtin__ftrace]" module!
> Looking at the vmlinux_path (8 entries long)
> Using /usr/lib/debug/boot/vmlinux-6.5.13-1rodete2-amd64 for symbols
> perf: Segmentation fault
> Obtained 16 stack frames.
> ./perf(+0x1b7dcd) [0x55c40be97dcd]
> ./perf(+0x1b7eb7) [0x55c40be97eb7]
> /lib/x86_64-linux-gnu/libc.so.6(+0x3c510) [0x7f33d7a5a510]
> ./perf(+0x1c2e9c) [0x55c40bea2e9c]
> ./perf(+0x1c43f6) [0x55c40bea43f6]
> ./perf(+0x1c4649) [0x55c40bea4649]
> ./perf(+0x1c46d3) [0x55c40bea46d3]
> ./perf(+0x1c7303) [0x55c40bea7303]
> ./perf(+0x1c70b5) [0x55c40bea70b5]
> ./perf(+0x1c73e6) [0x55c40bea73e6]
> ./perf(+0x11833e) [0x55c40bdf833e]
> ./perf(+0x118f78) [0x55c40bdf8f78]
> ./perf(+0x103d49) [0x55c40bde3d49]
> ./perf(+0x103e75) [0x55c40bde3e75]
> ./perf(+0x1044c0) [0x55c40bde44c0]
> ./perf(+0x104de0) [0x55c40bde4de0]
> test child interrupted
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!

Ah, tripped over a latent bug summarized in this part of an asan stack trace:
```
freed by thread T0 here:
   #0 0x7fa13bcd74b5 in __interceptor_realloc
./../../../src/libsanitizer/asan/asan_malloc_linux.cpp:85
   #1 0x561d66377713 in __maps__insert util/maps.c:353
   #2 0x561d66377b89 in maps__insert util/maps.c:413
   #3 0x561d6652911d in dso__process_kernel_symbol util/symbol-elf.c:1460
   #4 0x561d6652aaae in dso__load_sym_internal util/symbol-elf.c:1675
   #5 0x561d6652b6dc in dso__load_sym util/symbol-elf.c:1771
   #6 0x561d66321a4e in dso__load util/symbol.c:1914
   #7 0x561d66372cd9 in map__load util/map.c:353
   #8 0x561d663730e7 in map__find_symbol_by_name_idx util/map.c:397
   #9 0x561d663731e7 in map__find_symbol_by_name util/map.c:410
   #10 0x561d66378208 in maps__find_symbol_by_name_cb util/maps.c:524
   #11 0x561d66377f49 in maps__for_each_map util/maps.c:471
   #12 0x561d663784a0 in maps__find_symbol_by_name util/maps.c:546
   #13 0x561d662093e8 in machine__find_kernel_symbol_by_name util/machine.h:243
   #14 0x561d6620abbd in test__vmlinux_matches_kallsyms
tests/vmlinux-kallsyms.c:330
..
```
dso__process_kernel_symbol rewrites the kernel maps here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/symbol-elf.c#n1378
which resizes the maps_by_address array causing the maps__for_each_map
iteration in frame 11 to be iterating over a stale/freed value.

The most correct solutions would be to clone the maps_by_address array
prior to iteration, or reference count maps_by_address and its size.
Neither of these solutions particularly appeal, so just reloading the
maps_by_address and size on each iteration also fixes the problem, but
possibly causes some maps to be skipped/repeated. I think this is
acceptable correctness for the performance.

Thanks,
Ian

> Thanks,
> Namhyung