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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Sep 2014 11:06:49 -0700 From: Tong Shen <endlessroad@...gle.com> To: Arnaldo Carvalho de Melo <acme@...nel.org> Cc: linux-perf-users@...r.kernel.org, Linux Kernel Mailing List <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 1/1] perf tools: Fix Symbol Address for ET_DYN Gentle ping :-) On Thu, Sep 11, 2014 at 1:38 PM, Arnaldo Carvalho de Melo <acme@...nel.org> wrote: > Em Thu, Sep 11, 2014 at 11:21:40AM -0700, Tong Shen escreveu: >> Hi Arnaldo, >> >> Thanks a lot for the comment! >> >> I fixed the coding style and added a helper function with comments, as >> you suggested :-) >> See attachment. > > That is much better, I'm adding lkml to the CC list so that we can give > other people the opportunity to chime in. > > - Arnaldo > >> On Thu, Sep 11, 2014 at 8:01 AM, Arnaldo Carvalho de Melo >> <acme@...nel.org> wrote: >> > Em Fri, Sep 05, 2014 at 11:38:02AM -0700, Tong Shen escreveu: >> >> Hi linux-perf-users, >> >> >> >> It's actually a patch, and I didn't find a development mailing list >> >> for perf, so I just post it here. If this is not the right place to >> > >> > You can post it here, but please add the maintainers to the CC list, so >> > that we can more quickly see your message. >> > >> > Comments below: >> > >> >> post patches, could someone direct me to it? Thanks in advance! >> >> >> >> In tools/perf/util/symbol-elf.c, we don't adjust symbol address for >> >> ET_DYN type of ELF files because we think ET_DYN is always >> >> relocatable. >> >> >> >> But that's not necessarily true; there are some non-relocatable ET_DYN >> >> ELF files. For those files, we should still adjust symbol address. >> > >> >> Suggested patch: (HTML subpart now allowed in this mailing list...) >> > >> >> +++ b/perf-3.12.0/tools/perf/util/symbol-elf.c >> >> @@ -915,7 +915,10 @@ int dso__load_sym(struct dso *dso, struct map *map, >> > >> >> if ((used_opd && runtime_ss->adjust_symbols) >> >> - || (!used_opd && syms_ss->adjust_symbols)) { >> >> + || (!used_opd && syms_ss->adjust_symbols) >> >> + || (syms_ss->ehdr.e_type == et_dyn >> >> + && shdr.sh_addr != shdr.sh_offset >> >> + && elf_sec__is_text(&shdr, secstrs))) { >> > >> > Please try to follow kernel coding style in these tools, i.e. the && >> > should be at the end, the expression on each line should be aligned just >> > after the opening parens. >> > >> > If, like in this case, you find something not following that style, take >> > the opportunity to fix it :-) >> > >> > But back to your change, I'll have to look at this code further, just >> > looking at the excerpt above I find it way too confusing, and your patch >> > makes it even more so... >> > >> > Can you introduce some suitably named helper that has those extra >> > conditions you're adding now and then add it to this conditional? >> > >> > I.e. make it somethine like: >> > >> > - if ((used_opd && runtime_ss->adjust_symbols) >> > - || (!used_opd && syms_ss->adjust_symbols)) { >> > + if ((used_opd && runtime_ss->adjust_symbols) || >> > + (!used_opd && syms_ss->adjust_symbols) || >> > + syms__some_suitable_name(syms_ss, &shdr)) { >> > >> > And then add your explanation of these kinds of files as a comment over >> > this new function, please? >> > >> >> pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " " >> >> "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__, >> >> (u64)sym.st_value, (u64)shdr.sh_addr, >> > >> > Best regards, >> > >> > - Arnaldo >> >> >> >> -- >> Best Regards, Tong Shen > >> diff --git a/perf-3.12.0/tools/perf/util/symbol-elf.c b/perf-3.12.0/tools/perf/util/symbol-elf.c >> index a9c829b..f9bd488 100644 >> --- a/perf-3.12.0/tools/perf/util/symbol-elf.c >> +++ b/perf-3.12.0/tools/perf/util/symbol-elf.c >> @@ -673,6 +673,27 @@ static u64 ref_reloc(struct kmap *kmap) >> return 0; >> } >> >> +/** >> + * is_non_relocatable_dyn - check if ELF file is ET_DYN, but not relocatable >> + * @ehdr: ELF header of the ELF file >> + * @shdr: a section header in the ELF file >> + * >> + * For ELF files with type ET_EXEC and ET_REL, we adjust symbol addresses in >> + * symbol table in dso__load_sym() so that later we can always locate symbols >> + * by sym.st_value + map_address_of_ELF_file. >> + * >> + * But ELF files with type ET_DYN may need adjusting as well, if they are >> + * not relocatable. There's no standard way to tell if an ELF file with type >> + * ET_DYN is relocatable. One possible way to do that is checking if >> + * sh_addr == sh_offset for .text section. >> + */ >> +static bool is_non_relocatable_dyn(GElf_Ehdr *ehdr, GElf_Shdr *shdr, >> + Elf_Data *secstrs) { >> + return ehdr->e_type == ET_DYN && >> + elf_sec__is_text(shdr, secstrs) && >> + shdr->sh_offset != shdr->sh_addr; >> +} >> + >> int dso__load_sym(struct dso *dso, struct map *map, >> struct symsrc *syms_ss, struct symsrc *runtime_ss, >> symbol_filter_t filter, int kmodule) >> @@ -914,8 +935,9 @@ int dso__load_sym(struct dso *dso, struct map *map, >> goto new_symbol; >> } >> >> - if ((used_opd && runtime_ss->adjust_symbols) >> - || (!used_opd && syms_ss->adjust_symbols)) { >> + if ((used_opd && runtime_ss->adjust_symbols) || >> + (!used_opd && syms_ss->adjust_symbols) || >> + is_non_relocatable_dyn(&ehdr, &shdr, secstrs)) { >> pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " " >> "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__, >> (u64)sym.st_value, (u64)shdr.sh_addr, > -- Best Regards, Tong Shen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists