| 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: <CAP-5=fVKkd92FP9D2qccmRHMJHbYNNpVC-XAuh5i7jrrTWHg8g@mail.gmail.com>
Date: Sun, 31 Jul 2022 09:51:53 -0700
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Fangrui Song <maskray@...gle.com>,
Chang Rui <changruinj@...il.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols
On Sun, Jul 31, 2022 at 5:37 AM Leo Yan <leo.yan@...aro.org> wrote:
>
> On Sat, Jul 30, 2022 at 08:21:10AM -0700, Ian Rogers wrote:
>
> [...]
>
> > > On the other hand, I can accept to simply change pr_warning() to
> > > pr_debug4() to avoid warning flood, the log still can help us to find
> > > potential symbol parsing issue, so far they are not false-positive
> > > reporting.
> >
> > Thanks, I suspect the ELF that the Java agent has created isn't good.
> > The Java agent is part of perf as and so is the ELF file generation
> > code:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/genelf.c?h=perf/core#n367
>
> I think it's no need to change the function jit_write_elf(), please see
> below comment.
>
> > I took a quick look but most of the logic is in libelf - it seems less
> > obvious the bug would be there rather than perf. Could you take a
> > look? Ideally there'd be a quick fix that keeps all the benefits of
> > your change and the jvmti code working.
>
> A bit more finding for java JIT symbols. Let's see below two samples:
>
> 6.72% java /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-29.so 0x5b54 B [.] Interpreter
> 0.90% java /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-3475.so 0x4fc B [.] jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long, int, boolean)
>
> I can see the DSO "jitted-214330-29.so" only contains a java JIT symbol
> "Interpreter", and I also observed the same behavior for other JIT
> symbols, e.g. jitted-214330-3475.so only contains the symbol
> "jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long,
> int, boolean)".
>
> We always see these JIT symbols always have the consistent start file
> address, but this would not introduce conflit since every JIT symbol has
> its own DSO rather than share the same DSO.
>
> I think now I understand your meaning, and your below change is fine for
> me, I tested it and confirmed it can show up java JIT symbols properly.
> Just a comment, it would be better to add a comment for why we need to
> add symbols when failed to find program header, like:
>
> if (elf_read_program_header(syms_ss->elf,
> (u64)sym.st_value, &phdr)) {
> pr_debug4("%s: failed to find program header for "
> "symbol: %s\n", __func__, elf_name);
> pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
> "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n",
> __func__, (u64)sym.st_value, (u64)shdr.sh_addr,
> (u64)shdr.sh_offset);
> /*
> * Fail to find program header, let's rollback to use shdr.sh_addr
> * and shdr.sh_offset to calibrate symbol's file address, though
> * this is not necessary for normal C ELF file, we still need to
> * handle java JIT symbols in this case.
> */
> sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> } else {
> ...
> }
>
> Could you send a formal patch for this? Thanks!
Done. Thanks for sanity checking all of this! Please double check and
add the reviewed/acked-by:
https://lore.kernel.org/lkml/20220731164923.691193-1-irogers@google.com/
Thanks,
Ian
> Leo
>
> > > Thanks,
> > > Leo
> > >
> > > > ```
> > > > --- a/tools/perf/util/symbol-elf.c
> > > > +++ b/tools/perf/util/symbol-elf.c
> > > > @@ -1305,16 +1305,21 @@ dso__load_sym_internal(struct dso *dso, struct
> > > > map *map, struct symsrc *syms
> > > > _ss,
> > > >
> > > > if (elf_read_program_header(syms_ss->elf,
> > > > (u64)sym.st_value, &phdr)) {
> > > > - pr_warning("%s: failed to find program
> > > > header for "
> > > > + pr_debug4("%s: failed to find program
> > > > header for "
> > > > "symbol: %s st_value: %#" PRIx64 "\n",
> > > > __func__, elf_name,
> > > > (u64)sym.st_value);
> > > > - continue;
> > > > + pr_debug4("%s: adjusting symbol:
> > > > st_value: %#" PRIx64 " "
> > > > + "sh_addr: %#" PRIx64 "
> > > > sh_offset: %#" PRIx64 "\n",
> > > > + __func__, (u64)sym.st_value,
> > > > (u64)shdr.sh_addr,
> > > > + (u64)shdr.sh_offset);
> > > > + sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> > > > + } else {
> > > > + pr_debug4("%s: adjusting symbol:
> > > > st_value: %#" PRIx64 " "
> > > > + "p_vaddr: %#" PRIx64 "
> > > > p_offset: %#" PRIx64 "\n",
> > > > + __func__, (u64)sym.st_value,
> > > > (u64)phdr.p_vaddr,
> > > > + (u64)phdr.p_offset);
> > > > + sym.st_value -= phdr.p_vaddr - phdr.p_offset;
> > > > }
> > > > - pr_debug4("%s: adjusting symbol: st_value: %#"
> > > > PRIx64 " "
> > > > - "p_vaddr: %#" PRIx64 " p_offset: %#"
> > > > PRIx64 "\n",
> > > > - __func__, (u64)sym.st_value,
> > > > (u64)phdr.p_vaddr,
> > > > - (u64)phdr.p_offset);
> > > > - sym.st_value -= phdr.p_vaddr - phdr.p_offset;
> > > > }
> > > >
> > > > demangled = demangle_sym(dso, kmodule, elf_name);
> > > > ```
> > > >
> > > > Thanks,
> > > > Ian
> > > >
> > > > >
> > > > > demangled = demangle_sym(dso, kmodule, elf_name);
> > > > > --
> > > > > 2.25.1
> > > > >
Powered by blists - more mailing lists