Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 30 Apr 2022 01:03:01 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH v2 2/2] ftrace: recordmcount: Handle sections with no
 non-weak symbols
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        llvm@lists.linux.dev, Michael Ellerman <mpe@ellerman.id.au>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>
References: <cover.1651166001.git.naveen.n.rao@linux.vnet.ibm.com>
        <126aca34935cf1c7168e17970c706e36577094e7.1651166001.git.naveen.n.rao@linux.vnet.ibm.com>
        <20220428184212.18fbf438@gandalf.local.home>
        <1651252324.js9790ngjg.naveen@linux.ibm.com>
        <20220429135916.47c3e623@gandalf.local.home>
In-Reply-To: <20220429135916.47c3e623@gandalf.local.home>
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1651257788.xtscezsfky.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Steven Rostedt wrote:
> On Fri, 29 Apr 2022 23:09:19 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> If I'm understanding your suggestion right:
>> - we now create a new section in each object file: __mcount_loc_weak,=20
>>   and capture such relocations using weak symbols there.
>=20
> Yes, but it would be putting the same information it puts into __mcount_l=
oc
> but also add it here too. That is, it will duplicate the data.
>=20
>> - we then ask the linker to put these separately between, say,=20
>>   __start_mcount_loc_weak and __stop_mcount_loc_weak
>=20
> Yes, but it will also go in the location between __start_mcount_loc and
> __stop_mcount_loc.
>=20
>> - on ftrace init, we go through entries in this range, but discard those=
=20
>>   that belong to functions that also have an entry between=20
>>   __start_mcount_loc and __stop_mcount loc.
>=20
> 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 th=
em
> to make sure that they are correct.
>=20
>>=20
>> The primary issue I see here is that the mcount locations within the new=
=20
>> weak section will end up being offsets from a different function in=20
>> vmlinux, since the linker does not create a symbol for the weak=20
>> functions that were over-ridden.
>=20
> 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=20
functions were overridden by looking at entries in the __mcount_loc_weak=20
section?

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

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

Did you have a different approach in mind?

>=20
>>=20
>> As an example, in the issue described in this patch set, if powerpc=20
>> starts over-riding kexec_arch_apply_relocations(), then the current weak=
=20
>> implementation in kexec_file.o gets carried over to the final vmlinux,=20
>> but the instructions will instead appear under the previous function in=
=20
>> kexec_file.o: crash_prepare_elf64_headers(). This function may or may=20
>> not be traced to begin with, so we won't be able to figure out if this=20
>> is valid or not.
>=20
> 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 t=
he
> new function, and one that was overridden.

In the final vmlinux, we will have two entries: one pointing at the=20
correct function, while the other will point to some other function=20
name. So, at least from kallsym perspective, duplicate mcount entries=20
won't be for the function that was overridden, but some arbitrary=20
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.
>=20
> I was thinking of doing this just so that we know what functions are weak
> and perhaps need extra processing.
>=20
> Another issue with weak functions and ftrace just came up here:
>=20
>   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=20
just wasn't sure if all of those were.

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


Thanks,
Naveen