From: Joe Lawrence <joe.lawrence@redhat.com>
To: Petr Mladek <pmladek@suse.com>, Song Liu <song@kernel.org>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
jpoimboe@kernel.org, jikos@kernel.org, mbenes@suse.cz,
x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
Josh Poimboeuf <jpoimboe@redhat.com>,
Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>,
Michael Ellerman <mpe@ellerman.id.au>,
Russell Currey <ruscur@russell.cc>
Subject: Re: powerpc-part: was: Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
Date: Tue, 13 Dec 2022 17:19:22 -0500 [thread overview]
Message-ID: <21984193-f9a5-1a7d-ecb7-0ae74b8cf202@redhat.com> (raw)
In-Reply-To: <Y5h+PX6a1a9yjQPp@alley>
On 12/13/22 8:29 AM, Petr Mladek wrote:
> On Tue 2022-12-13 00:13:46, Song Liu wrote:
>> )() ()On Mon, Dec 12, 2022 at 9:12 AM Petr Mladek <pmladek@suse.com> wrote:
>>>
>>> On Fri 2022-12-09 11:59:35, Song Liu wrote:
>>>> On Fri, Dec 9, 2022 at 3:41 AM Petr Mladek <pmladek@suse.com> wrote:
>>>>> On Mon 2022-11-28 17:57:06, Song Liu wrote:
>>>>>> On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek <pmladek@suse.com> wrote:
>>>>>>>
>>>>>>>> --- a/arch/powerpc/kernel/module_64.c
>>>>>>>> +++ b/arch/powerpc/kernel/module_64.c
>>>>>>>> +#ifdef CONFIG_LIVEPATCH
>>>>>>>> +void clear_relocate_add(Elf64_Shdr *sechdrs,
>>>>>>>> + const char *strtab,
>>>>>>>> + unsigned int symindex,
>>>>>>>> + unsigned int relsec,
>>>>>>>> + struct module *me)
>>>>>>>> +{
>>>
>>> [...]
>>>
>>>>>>>> +
>>>>>>>> + instruction = (u32 *)location;
>>>>>>>> + if (is_mprofile_ftrace_call(symname))
>>>>>>>> + continue;
>>>>>
>>>>> Why do we ignore these symbols?
>>>>>
>>>>> I can't find any counter-part in apply_relocate_add(). It looks super
>>>>> tricky. It would deserve a comment.
>>>>>
>>>>> And I have no idea how we could maintain these exceptions.
>>>>>
>>>>>>>> + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
>>>>>>>> + continue;
>>>>>
>>>>> Same here. It looks super tricky and there is no explanation.
>>>>
>>>> The two checks are from restore_r2(). But I cannot really remember
>>>> why we needed them. It is probably an updated version from an earlier
>>>> version (3 year earlier..).
>>>
>>> This is a good sign that it has to be explained in a comment.
>>> Or even better, it should not by copy pasted.
>>>
>>>>>>>> + instruction += 1;
>>>>>>>> + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
>>>
>>> I believe that this is not enough. apply_relocate_add() does this:
>>>
>>> int apply_relocate_add(Elf64_Shdr *sechdrs,
>>> [...]
>>> struct module *me)
>>> {
>>> [...]
>>> case R_PPC_REL24:
>>> /* FIXME: Handle weak symbols here --RR */
>>> if (sym->st_shndx == SHN_UNDEF ||
>>> sym->st_shndx == SHN_LIVEPATCH) {
>>> [...]
>>> if (!restore_r2(strtab + sym->st_name,
>>> (u32 *)location + 1, me))
>>> [...] return -ENOEXEC;
>>>
>>> ---> if (patch_instruction((u32 *)location, ppc_inst(value)))
>>> return -EFAULT;
>>>
>>> , where restore_r2() does:
>>>
>>> static int restore_r2(const char *name, u32 *instruction, struct module *me)
>>> {
>>> [...]
>>> /* ld r2,R2_STACK_OFFSET(r1) */
>>> ---> if (patch_instruction(instruction, ppc_inst(PPC_INST_LD_TOC)))
>>> return 0;
>>> [...]
>>> }
>>>
>>> By other words, apply_relocate_add() modifies two instructions:
>>>
>>> + patch_instruction() called in restore_r2() writes into "location + 1"
>>> + patch_instruction() called in apply_relocate_add() writes into "location"
>>>
>>> IMHO, we have to clear both.
>>>
>>> IMHO, we need to implement a function that reverts the changes done
>>> in restore_r2(). Also we need to revert the changes done in
>>> apply_relocate_add().
>>
>> I finally got time to read all the details again and recalled what
>> happened with the code.
>>
>> The failure happens when we
>> 1) call apply_relocate_add() on klp load (or module first load,
>> if klp was loaded first);
>> 2) do nothing when the module is unloaded;
>> 3) call apply_relocate_add() on module reload, which failed.
>>
>> The failure happens at this check in restore_r2():
>>
>> if (*instruction != PPC_RAW_NOP()) {
>> pr_err("%s: Expected nop after call, got %08x at %pS\n",
>> me->name, *instruction, instruction);
>> return 0;
>> }
>>
>> Therefore, apply_relocate_add only fails when "location + 1"
>> is not NOP. And to make it not fail, we only need to write NOP to
>> "location + 1" in clear_relocate_add().
>
> Yes, this should be enough to pass the existing check.
>
>> IIUC, you want clear_relocate_add() to undo everything we did
>> in apply_relocate_add(); while I was writing clear_relocate_add()
>> to make the next apply_relocate_add() not fail.
>>
>> I agree that, based on the name, clear_relocate_add() should
>> undo everything by apply_relocate_add(). But I am not sure how
>> to handle some cases. For example, how do we undo
>>
>> case R_PPC64_ADDR32:
>> /* Simply set it */
>> *(u32 *)location = value;
>> break;
>>
>> Shall we just write zeros? I don't think this matters.
>
> I guess that it would be zeros as we do in x86_64.
>
>
>> I think this is the question we should answer first:
>> What shall clear_relocate_add() do?
>> 1) undo everything by apply_relocate_add();
>> 2) only do things needed to make the next
>> apply_relocate_add succeed;
>> 3) something between 1) and 2).
>
> Good question.
>
> Hmm, the commit a443bf6e8a7674b86221f49 ("powerpc/modules: Add REL24
> relocation support of livepatch symbols") suggests that all symbols
> in the section SHN_LIVEPATCH have the type R_PPC_REL24. AFAIK, the
> kernel livepatches are the only user of the clear_relocate_add()
> feature.
>
> If the above is correct then it might be enough to clear only
> R_PPC_REL24 type. And it might be enough to warn when clear_relocate_add()
> is called for another type so that we know when the relocations
> were not cleared properly.
>
> Good question. We might need some input from people familiar
> with the architecture and creating the livepatches.
>
Adding Russell to the to CC list as he worked some of recent ppc64le
livepatch klp-relocation threads [1] [2].
Maybe it would simpler to first organize a cleanup of the code, then add
the capability to undo the relocations? According to [2] and the last
comment on [3], it sounded like the Power folks had a "full"(er)
solution in mind depending on our requirements.
Finally, I'll try to finish my v6.1 rebase of the klp-convert patchset
this week. That includes a bunch of kselftests that generate all manner
of klp-relocation types and sections. (More than I've ever seen out of
kpatch-build.)
[1] https://lore.kernel.org/linuxppc-dev/YX9UUBeudSUuJs01@redhat.com/
[2] https://lore.kernel.org/linuxppc-dev/YxAc87dTmclHGCUy@redhat.com/
[3] https://github.com/linuxppc/issues/issues/375
--
Joe
next prev parent reply other threads:[~2022-12-13 22:20 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20220901171252.2148348-1-song@kernel.org>
2022-11-17 22:06 ` [PATCH v6] livepatch: Clear relocation targets on a module removal Song Liu
2022-11-18 16:24 ` Petr Mladek
2022-11-18 17:14 ` Song Liu
2022-11-21 15:32 ` Joe Lawrence
2022-11-21 16:32 ` Song Liu
2022-11-29 1:57 ` Song Liu
2022-12-09 11:41 ` powerpc-part: was: " Petr Mladek
2022-12-09 19:59 ` Song Liu
2022-12-12 17:11 ` Petr Mladek
2022-12-12 22:22 ` Song Liu
2022-12-13 8:13 ` Song Liu
2022-12-13 13:29 ` Petr Mladek
2022-12-13 22:19 ` Joe Lawrence [this message]
2022-12-13 19:31 ` Song Liu
2022-12-09 12:36 ` x86 part: " Petr Mladek
2022-12-09 12:49 ` Miroslav Benes
2022-12-09 13:54 ` Petr Mladek
2022-12-09 14:20 ` Petr Mladek
2022-12-09 18:21 ` Song Liu
2022-12-09 12:55 ` Miroslav Benes
2022-12-09 18:30 ` Song Liu
2022-12-09 18:51 ` Christophe Leroy
2022-12-09 19:24 ` Song Liu
2022-12-12 8:16 ` Miroslav Benes
2022-12-13 8:28 ` Song Liu
2022-12-13 14:37 ` Petr Mladek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=21984193-f9a5-1a7d-ecb7-0ae74b8cf202@redhat.com \
--to=joe.lawrence@redhat.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=kamalesh@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=mpe@ellerman.id.au \
--cc=pmladek@suse.com \
--cc=ruscur@russell.cc \
--cc=song@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).