| 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: <CAPp4yYxripyJvr87TtVqNMi5=K+UQGeKTomQiJarFnVGyjtofA@mail.gmail.com>
Date: Wed, 24 Sep 2014 17:28:24 -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 Mon, Sep 15, 2014 at 11:06 AM, Tong Shen <endlessroad@...gle.com> wrote:
> 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
--
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