linux-kernel - Re: [PATCH v2 2/2] ftrace: recordmcount: Handle sections with no non-weak symbols

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: <1651257788.xtscezsfky.naveen@linux.ibm.com>
Date:   Sat, 30 Apr 2022 01:03:01 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        llvm@...ts.linux.dev, Michael Ellerman <mpe@...erman.id.au>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 2/2] ftrace: recordmcount: Handle sections with no
 non-weak symbols

Steven Rostedt wrote:
> On Fri, 29 Apr 2022 23:09:19 +0530
> "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:
> 
>> If I'm understanding your suggestion right:
>> - we now create a new section in each object file: __mcount_loc_weak, 
>>   and capture such relocations using weak symbols there.
> 
> Yes, but it would be putting the same information it puts into __mcount_loc
> but also add it here too. That is, it will duplicate the data.
> 
>> - we then ask the linker to put these separately between, say, 
>>   __start_mcount_loc_weak and __stop_mcount_loc_weak
> 
> Yes, but it will also go in the location between __start_mcount_loc and
> __stop_mcount_loc.
> 
>> - on ftrace init, we go through entries in this range, but discard those 
>>   that belong to functions that also have an entry between 
>>   __start_mcount_loc and __stop_mcount loc.
> 
> But we should be able to know if it was overridden or not, by seeing if
> there's another function that was called. Or at least, we can validate them
> to make sure that they are correct.
> 
>> 
>> The primary issue I see here is that the mcount locations within the new 
>> weak section will end up being offsets from a different function in 
>> vmlinux, since the linker does not create a symbol for the weak 
>> functions that were over-ridden.
> 
> The point of this section is to know which functions in __mcount_loc may
> have been overridden, as they would be found in the __mcount_loc_weak
> section. And then we can do something "special" to them.

I'm not sure I follow that. How are you intending to figure out which 
functions were overridden by looking at entries in the __mcount_loc_weak 
section?

In the final vmlinux image, we only get offsets into .text for all 
mcount locations, but no symbol information. The only hint is the fact 
that a single kallsym symbol has multiple mcount locations within it. 
Even then, the symbol with duplicate mcount entries won't be the 
function that was overridden.

We could do a kallsyms_lookup() on each entry and consult the 
__mcount_loc_weak section to identify duplicates, but that looks to be 
very expensive.

Did you have a different approach in mind?

> 
>> 
>> As an example, in the issue described in this patch set, if powerpc 
>> starts over-riding kexec_arch_apply_relocations(), then the current weak 
>> implementation in kexec_file.o gets carried over to the final vmlinux, 
>> but the instructions will instead appear under the previous function in 
>> kexec_file.o: crash_prepare_elf64_headers(). This function may or may 
>> not be traced to begin with, so we won't be able to figure out if this 
>> is valid or not.
> 
> If it was overridden, then there would be two entries for function that
> overrides the weak function in the __mcount_loc section, right? One for the
> new function, and one that was overridden.

In the final vmlinux, we will have two entries: one pointing at the 
correct function, while the other will point to some other function 
name. So, at least from kallsym perspective, duplicate mcount entries 
won't be for the function that was overridden, but some arbitrary 
function that came before the weak function in the object file.

> Of course this could be more
> complex if the new function had been marked notrace.
> 
> I was thinking of doing this just so that we know what functions are weak
> and perhaps need extra processing.
> 
> Another issue with weak functions and ftrace just came up here:
> 
>   https://lore.kernel.org/all/20220428095803.66c17c32@gandalf.local.home/

I noticed this just yesterday:

  # cat available_filter_functions | sort | uniq -d | wc -l
  430

I'm fairly certain that some of those are due to weak functions -- I 
just wasn't sure if all of those were.

I suppose this will now also be a problem with ftrace_location(), given 
that it was recently changed to look at an entire function for mcount 
locations?


Thanks,
Naveen