powerpc-part: was: Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
Hi,
this reply is only about the powerpc-specific part.
Also adding Kamalesh and Michael into Cc who worked on the related
commit a443bf6e8a7674b86221f49 ("powerpc/modules: Add REL24 relocation
support of livepatch symbols").
On Mon 2022-11-28 17:57:06, Song Liu wrote:
> On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote:
> >
> > > --- a/arch/powerpc/kernel/module_64.c
> > > +++ b/arch/powerpc/kernel/module_64.c
I put back the name of the modified file so that it is easier
to know what changes we are talking about.
[...]
> > > +#ifdef CONFIG_LIVEPATCH
> > > +void clear_relocate_add(Elf64_Shdr *sechdrs,
> > > +const char *strtab,
> > > +unsigned int symindex,
> > > +unsigned int relsec,
> > > +struct module *me)
> > > +{
> > > + unsigned int i;
> > > + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> > > + Elf64_Sym *sym;
> > > + unsigned long *location;
> > > + const char *symname;
> > > + u32 *instruction;
> > > +
> > > + pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
> > > + sechdrs[relsec].sh_info);
> > > +
> > > + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
> > > + location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
> > > + + rela[i].r_offset;
> > > + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
> > > + + ELF64_R_SYM(rela[i].r_info);
> > > + symname = me->core_kallsyms.strtab
> > > + + sym->st_name;
The above calculation is quite complex. It seems to be copied from
apply_relocate_add(). If I maintained this code I would want to avoid
the duplication. definitely.
> > > +
> > > + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
> > > + continue;
Why are we interested only into R_PPC_REL24 relocation types, please?
The code for generating the special SHN_LIVEPATCH section is not in
the mainline so it is not well defined.
I guess that R_PPC_REL24 relocation type is used by kPatch. Are we
sure that other relocation types wont be needed?
Anyway, we must warn when an unsupported type is used in SHN_LIVEPATCH
section here.
> > > + /*
> > > + * reverse the operations in apply_relocate_add() for case
> > > + * R_PPC_REL24.
> > > + */
> > > + if (sym->st_shndx != SHN_UNDEF &&
Do we want to handle SHN_UNDEF symbols here?
The commit a443bf6e8a7674b86221f49 ("powerpc/modules: Add REL24
relocation support of livepatch symbols") explains that
R_PPC_REL24 relocations in SHN_LIVEPATCH section are handled
__like__ relocations in SHN_UNDEF sections.
My understanding is that R_PPC_REL24 reallocation type has
two variants. Where the variant used in SHN_UNDEF and
SHN_LIVEPATCH sections need some preprocessing.
Anyway, if this function is livepatch-specific that we should
clear only symbols from SHN_LIVEPATCH sections. I mean that
we should probably ignore SHN_UNDEF here.
> > > + sym->st_shndx != SHN_LIVEPATCH)
> > > + continue;
> > > +
> > > +
> > > + 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.
> > > + instruction += 1;
> > > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > > + }
> > > +
> > > +}
> >
> > This looks like a lot of duplicated code. Isn't it?
>
> TBH, I think the duplicated code is not really bad.
How exactly do you mean it, please?
Do you think that the amount of duplicated code is small enough?
Or that the new function looks better that updating the existing one?
> apply_relocate_add() is a much more complicated function, I would
> rather not mess it up to make this function a little simpler.
IMHO, the duplicated code is quite tricky. And if we really do
not need to clear all relocation types then we could avoid
the duplication another way, for example:
int update_relocate_add(Elf64_Shdr *sechdrs,
const char *strtab,
unsigned int symindex,
unsigned int relsec,
struct module *me,
bool apply)
{
unsigned int i;
Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
Elf64_Sym *sym;
Elf64_Xword r_type;
unsigned long *location;
if (apply) {
pr_debug("Applying ADD relocate section %u to %u\n", relsec,
x86 part: was: Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Mon 2022-11-28 17:57:06, Song Liu wrote:
> On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote:
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> > This duplicates a lot of code. Please, rename apply_relocate_add() the
> > same way as __apply_clear_relocate_add() and add the "apply" parameter.
> > Then add the wrappers for this:
> >
> > int write_relocate_add(Elf64_Shdr *sechdrs,
> >const char *strtab,
> >unsigned int symindex,
> >unsigned int relsec,
> >struct module *me,
> >bool apply)
> > {
> > int ret;
> > bool early = me->state == MODULE_STATE_UNFORMED;
> > void *(*write)(void *, const void *, size_t) = memcpy;
> >
> > if (!early) {
> > write = text_poke;
> > mutex_lock(&text_mutex);
> > }
>
> How about we move the "early" logic into __write_relocate_add()?
If I get it correctly then __write_relocate_add() has three different
return paths. I am not sure if this could be moved there a reasonable
way.
Anyway, I do not resist on the above proposal. Feel free to find
another solution that reduces the duplicated code and looks
reasonable. I am sure that there are more possibilities.
Best Regards,
Petr
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod,
> > > Elf_Shdr *sechdrs,
> > > return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > > }
> > >
> > > +static void klp_clear_object_relocations(struct module *pmod,
> > > + struct klp_object *obj)
> > > +{
> > > + int i, cnt;
> > > + const char *objname, *secname;
> > > + char sec_objname[MODULE_NAME_LEN];
> > > + Elf_Shdr *sec;
> > > +
> > > + objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > +
> > > + /* For each klp relocation section */
> > > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > > + sec = pmod->klp_info->sechdrs + i;
> > > + secname = pmod->klp_info->secstrings + sec->sh_name;
> > > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> > > + continue;
> > > +
> > > + /*
> > > + * Format: .klp.rela.sec_objname.section_name
> > > + * See comment in klp_resolve_symbols() for an explanation
> > > + * of the selected field width value.
> > > + */
> > > + secname = pmod->klp_info->secstrings + sec->sh_name;
> > > + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
> > > + if (cnt != 1) {
> > > + pr_err("section %s has an incorrectly formatted
> > > name\n",
> > > +secname);
> > > + continue;
> > > + }
>
> Actually, I think we don't need the cnt check here. Once it is removed,
> there isn't much duplicated logic.
Because it must have passed the check once already during
klp_apply_section_relocs()? Yes, perhaps.
Miroslav
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
Hi,
first thank you for taking over and I also appologize for not replying
much sooner.
On Thu, 1 Sep 2022, Song Liu wrote:
> From: Miroslav Benes
>
> Josh reported a bug:
>
> When the object to be patched is a module, and that module is
> rmmod'ed and reloaded, it fails to load with:
>
> module: x86/modules: Skipping invalid relocation target, existing value is
> nonzero for type 2, loc ba0302e9, val a03e293c
> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
> (-8)
> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
> load module 'nfsd'
>
> The livepatch module has a relocation which references a symbol
> in the _previous_ loading of nfsd. When apply_relocate_add()
> tries to replace the old relocation with a new one, it sees that
> the previous one is nonzero and it errors out.
>
> On ppc64le, we have a similar issue:
>
> module_64: livepatch_nfsd: Expected nop after call, got e8410018 at
> e_show+0x60/0x548 [livepatch_nfsd]
> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
> (-8)
> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
> load module 'nfsd'
>
> He also proposed three different solutions. We could remove the error
> check in apply_relocate_add() introduced by commit eda9cec4c9a1
> ("x86/module: Detect and skip invalid relocations"). However the check
> is useful for detecting corrupted modules.
>
> We could also deny the patched modules to be removed. If it proved to be
> a major drawback for users, we could still implement a different
> approach. The solution would also complicate the existing code a lot.
>
> We thus decided to reverse the relocation patching (clear all relocation
> targets on x86_64). The solution is not
> universal and is too much arch-specific, but it may prove to be simpler
> in the end.
>
> Reported-by: Josh Poimboeuf
> Signed-off-by: Miroslav Benes
> Signed-off-by: Song Liu
Petr has commented on the code aspects. I will just add that s390x was not
dealt with at the time because there was no live patching support for
s390x back then if I remember correctly and my notes do not lie. The same
applies to powerpc32. I think that both should be fixed as well with this
patch. It might also help to clean up the ifdeffery in the patch a bit.
Miroslav
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Mon 2022-11-28 17:57:06, Song Liu wrote:
> On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote:
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod,
> > > Elf_Shdr *sechdrs,
> > > return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > > }
> > >
> > > +static void klp_clear_object_relocations(struct module *pmod,
> > > + struct klp_object *obj)
> > > +{
> > > + int i, cnt;
> > > + const char *objname, *secname;
> > > + char sec_objname[MODULE_NAME_LEN];
> > > + Elf_Shdr *sec;
> > > +
> > > + objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > +
> > > + /* For each klp relocation section */
> > > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > > + sec = pmod->klp_info->sechdrs + i;
> > > + secname = pmod->klp_info->secstrings + sec->sh_name;
secname = ...
> > > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> > > + continue;
> > > +
> > > + /*
> > > + * Format: .klp.rela.sec_objname.section_name
> > > + * See comment in klp_resolve_symbols() for an explanation
> > > + * of the selected field width value.
> > > + */
> > > + secname = pmod->klp_info->secstrings + sec->sh_name;
secname = ...same a above.
> > > + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
> > > + if (cnt != 1) {
> > > + pr_err("section %s has an incorrectly formatted
> > > name\n",
> > > +secname);
> > > + continue;
> > > + }
>
> Actually, I think we don't need the cnt check here. Once it is removed,
> there isn't much duplicated logic.
Seriously?
A section with this error was skipped in klp_apply_section_relocs().
I did not cause rejecting the module! Why is it suddenly safe to
process it, please?
I see that you removed also:
if (strcmp(objname ? objname : "vmlinux", sec_objname))
return 0;
This is another bug in your "simplification".
> > > +
> > > + if (strcmp(objname, sec_objname))
> > > + continue;
> > > +
> > > + clear_relocate_add(pmod->klp_info->sechdrs,
> > > +pmod->core_kallsyms.strtab,
> > > +pmod->klp_info->symndx, i, pmod);
> > > + }
> > > +}
> >
> > Huh, this duplicates a lot of tricky code.
> >
> > It is even worse because this squashed code from two functions
> > klp_apply_section_relocs() and klp_apply_object_relocs()
> > into a single function. As a result, the code duplication is not
> > even obvious.
> >
> > Also the suffix "_reloacations() does not match the suffix of
> > the related funciton:
> >
> > + klp_apply_object_relocs() (existing)
> > + klp_clear_object_relocations()(new)
> >
> > This all would complicate maintenance of the code.
> >
> > Please, implement a common:
> >
> > int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> > const char *shstrtab, const char *strtab,
> > unsigned int symndx, unsigned int secndx,
> > const char *objname, bool apply);
> >
> > and
> >
> > int klp_write_object_relocs(struct klp_patch *patch,
> > struct klp_object *obj,
> > bool apply);
> >
> > and add the respective wrappers:
> >
> > int klp_apply_section_relocs(); /* also needed in module/main.c */
> > int klp_apply_object_relocs();
> > void klp_clear_object_relocs();
>
> With the above simplification (removing cnt check), do we still need
> all these wrappers? Personally, I think they will make the code more
> difficult to follow..
Sigh.
klp_apply_section_relocs() has 25 lines of code.
klp_apply_object_relocs() has 18 lines of code.
The only difference should be that klp_clear_object_relocs():
+ does not need to call klp_resolve_symbols()
+ need to call clear_relocate_add() instead of apply_relocate_add()
It is 7 different lines from in the existing 25 + 18 = 43 lines.
The duplication does not look like a good deal even from this POV.
If we introduce update_relocate_add(..., bool apply) parameter
then we could call update_relocate_add() in both situations.
Let me repeat from the last mail. klp_clear_object_relocs() even
reshuffled the duplicated code. It was even harder to find differences.
Do I still need to explain how bad idea was the code duplication,
please?
Best Regards,
Petr
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Fri 2022-12-09 14:54:10, Petr Mladek wrote: > On Mon 2022-11-28 17:57:06, Song Liu wrote: > > On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote: > > > > --- a/kernel/livepatch/core.c > > > > +++ b/kernel/livepatch/core.c > I see that you removed also: > > if (strcmp(objname ? objname : "vmlinux", sec_objname)) > return 0; > > This is another bug in your "simplification". This actually is not a bug. It was replaced by the strcmp() check below. > > > > + > > > > + if (strcmp(objname, sec_objname)) > > > > + continue; But this works only because the function is not called for "vmlinux". It can't be unloaded. Well, this optimization is not worth it. IMHO, it is better when the API is able to handle "vmlinux" object a safe way. We always try to make the livepatch API as error-proof as possible. It is the main idea of livepatching. It should fix bugs without breaking the running system. Best Regards, Petr
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Fri, Dec 9, 2022 at 5:54 AM Petr Mladek wrote:
>
> On Mon 2022-11-28 17:57:06, Song Liu wrote:
> > On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote:
> > > > --- a/kernel/livepatch/core.c
> > > > +++ b/kernel/livepatch/core.c
> > > > @@ -316,6 +316,45 @@ int klp_apply_section_relocs(struct module *pmod,
> > > > Elf_Shdr *sechdrs,
> > > > return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > > > }
> > > >
> > > > +static void klp_clear_object_relocations(struct module *pmod,
> > > > + struct klp_object *obj)
> > > > +{
> > > > + int i, cnt;
> > > > + const char *objname, *secname;
> > > > + char sec_objname[MODULE_NAME_LEN];
> > > > + Elf_Shdr *sec;
> > > > +
> > > > + objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > > +
> > > > + /* For each klp relocation section */
> > > > + for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > > > + sec = pmod->klp_info->sechdrs + i;
> > > > + secname = pmod->klp_info->secstrings + sec->sh_name;
>
> secname = ...
>
> > > > + if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> > > > + continue;
> > > > +
> > > > + /*
> > > > + * Format: .klp.rela.sec_objname.section_name
> > > > + * See comment in klp_resolve_symbols() for an explanation
> > > > + * of the selected field width value.
> > > > + */
> > > > + secname = pmod->klp_info->secstrings + sec->sh_name;
>
> secname = ...same a above.
>
> > > > + cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
> > > > + if (cnt != 1) {
> > > > + pr_err("section %s has an incorrectly formatted
> > > > name\n",
> > > > +secname);
> > > > + continue;
> > > > + }
> >
> > Actually, I think we don't need the cnt check here. Once it is removed,
> > there isn't much duplicated logic.
>
> Seriously?
>
> A section with this error was skipped in klp_apply_section_relocs().
> I did not cause rejecting the module! Why is it suddenly safe to
> process it, please?
Hmm.. I think you are right, we still need the check.
>
>
> I see that you removed also:
>
> if (strcmp(objname ? objname : "vmlinux", sec_objname))
> return 0;
>
> This is another bug in your "simplification".
>
>
> > > > +
> > > > + if (strcmp(objname, sec_objname))
> > > > + continue;
> > > > +
> > > > + clear_relocate_add(pmod->klp_info->sechdrs,
> > > > +pmod->core_kallsyms.strtab,
> > > > +pmod->klp_info->symndx, i, pmod);
> > > > + }
> > > > +}
> > >
> > > Huh, this duplicates a lot of tricky code.
> > >
> > > It is even worse because this squashed code from two functions
> > > klp_apply_section_relocs() and klp_apply_object_relocs()
> > > into a single function. As a result, the code duplication is not
> > > even obvious.
> > >
> > > Also the suffix "_reloacations() does not match the suffix of
> > > the related funciton:
> > >
> > > + klp_apply_object_relocs() (existing)
> > > + klp_clear_object_relocations()(new)
> > >
> > > This all would complicate maintenance of the code.
> > >
> > > Please, implement a common:
> > >
> > > int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> > > const char *shstrtab, const char *strtab,
> > > unsigned int symndx, unsigned int secndx,
> > > const char *objname, bool apply);
> > >
> > > and
> > >
> > > int klp_write_object_relocs(struct klp_patch *patch,
> > > struct klp_object *obj,
> > > bool apply);
> > >
> > > and add the respective wrappers:
> > >
> > > int klp_apply_section_relocs(); /* also needed in module/main.c */
> > > int klp_apply_object_relocs();
> > > void klp_clear_object_relocs();
> >
> > With the above simplification (removing cnt check), do we still need
> > all these wrappers? Personally, I think they will make the code more
> > difficult to follow..
>
> Sigh.
>
> klp_apply_section_relocs() has 25 lines of code.
> klp_apply_object_relocs() has 18 lines of code.
>
> The only difference should be that klp_clear_object_relocs():
>
>+ does not need to call klp_resolve_symbols()
>+ need to call clear_relocate_add() instead of apply_relocate_add()
>
> It is 7 different lines from in the existing 25 + 18 = 43 lines.
> The duplication does not look like a good deal even from this POV.
>
> If we introduce update_relocate_add(..., bool apply) parameter
> then we could call update_relocate_add() in both situations.
>
> Let me repeat from the last mail. klp_clear_object_relocs() even
> reshuffled the duplicated code. It was even harder to find differences.
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Fri, Dec 9, 2022 at 4:55 AM Miroslav Benes wrote:
>
> Hi,
>
> first thank you for taking over and I also appologize for not replying
> much sooner.
>
> On Thu, 1 Sep 2022, Song Liu wrote:
>
> > From: Miroslav Benes
> >
> > Josh reported a bug:
> >
> > When the object to be patched is a module, and that module is
> > rmmod'ed and reloaded, it fails to load with:
> >
> > module: x86/modules: Skipping invalid relocation target, existing value
> > is nonzero for type 2, loc ba0302e9, val a03e293c
> > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
> > (-8)
> > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
> > load module 'nfsd'
> >
> > The livepatch module has a relocation which references a symbol
> > in the _previous_ loading of nfsd. When apply_relocate_add()
> > tries to replace the old relocation with a new one, it sees that
> > the previous one is nonzero and it errors out.
> >
> > On ppc64le, we have a similar issue:
> >
> > module_64: livepatch_nfsd: Expected nop after call, got e8410018 at
> > e_show+0x60/0x548 [livepatch_nfsd]
> > livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
> > (-8)
> > livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
> > load module 'nfsd'
> >
> > He also proposed three different solutions. We could remove the error
> > check in apply_relocate_add() introduced by commit eda9cec4c9a1
> > ("x86/module: Detect and skip invalid relocations"). However the check
> > is useful for detecting corrupted modules.
> >
> > We could also deny the patched modules to be removed. If it proved to be
> > a major drawback for users, we could still implement a different
> > approach. The solution would also complicate the existing code a lot.
> >
> > We thus decided to reverse the relocation patching (clear all relocation
> > targets on x86_64). The solution is not
> > universal and is too much arch-specific, but it may prove to be simpler
> > in the end.
> >
> > Reported-by: Josh Poimboeuf
> > Signed-off-by: Miroslav Benes
> > Signed-off-by: Song Liu
>
> Petr has commented on the code aspects. I will just add that s390x was not
> dealt with at the time because there was no live patching support for
> s390x back then if I remember correctly and my notes do not lie. The same
> applies to powerpc32. I think that both should be fixed as well with this
> patch. It might also help to clean up the ifdeffery in the patch a bit.
I don't have test environments for s390 and powerpc, so I really don't know
whether I am doing something sane for them.
Would you have time to finish these parts? (Or maybe the whole patch..)
Thanks,
Song
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
Le 09/12/2022 à 19:30, Song Liu a écrit :
> On Fri, Dec 9, 2022 at 4:55 AM Miroslav Benes wrote:
>>
>> Hi,
>>
>> first thank you for taking over and I also appologize for not replying
>> much sooner.
>>
>> On Thu, 1 Sep 2022, Song Liu wrote:
>>
>>> From: Miroslav Benes
>>>
>>> Josh reported a bug:
>>>
>>>When the object to be patched is a module, and that module is
>>>rmmod'ed and reloaded, it fails to load with:
>>>
>>>module: x86/modules: Skipping invalid relocation target, existing value
>>> is nonzero for type 2, loc ba0302e9, val a03e293c
>>>livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
>>> (-8)
>>>livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
>>> load module 'nfsd'
>>>
>>>The livepatch module has a relocation which references a symbol
>>>in the _previous_ loading of nfsd. When apply_relocate_add()
>>>tries to replace the old relocation with a new one, it sees that
>>>the previous one is nonzero and it errors out.
>>>
>>>On ppc64le, we have a similar issue:
>>>
>>>module_64: livepatch_nfsd: Expected nop after call, got e8410018 at
>>> e_show+0x60/0x548 [livepatch_nfsd]
>>>livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd'
>>> (-8)
>>>livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to
>>> load module 'nfsd'
>>>
>>> He also proposed three different solutions. We could remove the error
>>> check in apply_relocate_add() introduced by commit eda9cec4c9a1
>>> ("x86/module: Detect and skip invalid relocations"). However the check
>>> is useful for detecting corrupted modules.
>>>
>>> We could also deny the patched modules to be removed. If it proved to be
>>> a major drawback for users, we could still implement a different
>>> approach. The solution would also complicate the existing code a lot.
>>>
>>> We thus decided to reverse the relocation patching (clear all relocation
>>> targets on x86_64). The solution is not
>>> universal and is too much arch-specific, but it may prove to be simpler
>>> in the end.
>>>
>>> Reported-by: Josh Poimboeuf
>>> Signed-off-by: Miroslav Benes
>>> Signed-off-by: Song Liu
>>
>> Petr has commented on the code aspects. I will just add that s390x was not
>> dealt with at the time because there was no live patching support for
>> s390x back then if I remember correctly and my notes do not lie. The same
>> applies to powerpc32. I think that both should be fixed as well with this
>> patch. It might also help to clean up the ifdeffery in the patch a bit.
>
> I don't have test environments for s390 and powerpc, so I really don't know
> whether I am doing something sane for them.
>
> Would you have time to finish these parts? (Or maybe the whole patch..)
Setting up a powerpc test environment is fairly easy with QEMU.
Some information below:
- https://github.com/linuxppc/wiki/wiki
- https://wiki.qemu.org/Documentation/Platforms/PowerPC
Christophe
Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Fri, Dec 9, 2022 at 10:52 AM Christophe Leroy
wrote:
>
>
>
> Le 09/12/2022 à 19:30, Song Liu a écrit :
> > On Fri, Dec 9, 2022 at 4:55 AM Miroslav Benes wrote:
> >>
> >> Hi,
> >>
> >> first thank you for taking over and I also appologize for not replying
> >> much sooner.
> >>
> >> On Thu, 1 Sep 2022, Song Liu wrote:
> >>
> >>> From: Miroslav Benes
> >>>
> >>> Josh reported a bug:
> >>>
> >>>When the object to be patched is a module, and that module is
> >>>rmmod'ed and reloaded, it fails to load with:
> >>>
> >>>module: x86/modules: Skipping invalid relocation target, existing
> >>> value is nonzero for type 2, loc ba0302e9, val a03e293c
> >>>livepatch: failed to initialize patch 'livepatch_nfsd' for module
> >>> 'nfsd' (-8)
> >>>livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing
> >>> to load module 'nfsd'
> >>>
> >>>The livepatch module has a relocation which references a symbol
> >>>in the _previous_ loading of nfsd. When apply_relocate_add()
> >>>tries to replace the old relocation with a new one, it sees that
> >>>the previous one is nonzero and it errors out.
> >>>
> >>>On ppc64le, we have a similar issue:
> >>>
> >>>module_64: livepatch_nfsd: Expected nop after call, got e8410018 at
> >>> e_show+0x60/0x548 [livepatch_nfsd]
> >>>livepatch: failed to initialize patch 'livepatch_nfsd' for module
> >>> 'nfsd' (-8)
> >>>livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing
> >>> to load module 'nfsd'
> >>>
> >>> He also proposed three different solutions. We could remove the error
> >>> check in apply_relocate_add() introduced by commit eda9cec4c9a1
> >>> ("x86/module: Detect and skip invalid relocations"). However the check
> >>> is useful for detecting corrupted modules.
> >>>
> >>> We could also deny the patched modules to be removed. If it proved to be
> >>> a major drawback for users, we could still implement a different
> >>> approach. The solution would also complicate the existing code a lot.
> >>>
> >>> We thus decided to reverse the relocation patching (clear all relocation
> >>> targets on x86_64). The solution is not
> >>> universal and is too much arch-specific, but it may prove to be simpler
> >>> in the end.
> >>>
> >>> Reported-by: Josh Poimboeuf
> >>> Signed-off-by: Miroslav Benes
> >>> Signed-off-by: Song Liu
> >>
> >> Petr has commented on the code aspects. I will just add that s390x was not
> >> dealt with at the time because there was no live patching support for
> >> s390x back then if I remember correctly and my notes do not lie. The same
> >> applies to powerpc32. I think that both should be fixed as well with this
> >> patch. It might also help to clean up the ifdeffery in the patch a bit.
> >
> > I don't have test environments for s390 and powerpc, so I really don't know
> > whether I am doing something sane for them.
> >
> > Would you have time to finish these parts? (Or maybe the whole patch..)
>
> Setting up a powerpc test environment is fairly easy with QEMU.
>
> Some information below:
> - https://github.com/linuxppc/wiki/wiki
> - https://wiki.qemu.org/Documentation/Platforms/PowerPC
Thanks for these pointers! I will give it a try.
Song
PS: Sometimes I am just lazy, you know..
Re: powerpc-part: was: Re: [PATCH v6] livepatch: Clear relocation targets on a module removal
On Fri, Dec 9, 2022 at 3:41 AM Petr Mladek wrote:
>
> Hi,
>
> this reply is only about the powerpc-specific part.
>
> Also adding Kamalesh and Michael into Cc who worked on the related
> commit a443bf6e8a7674b86221f49 ("powerpc/modules: Add REL24 relocation
> support of livepatch symbols").
>
>
> On Mon 2022-11-28 17:57:06, Song Liu wrote:
> > On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek wrote:
> > >
> > > > --- a/arch/powerpc/kernel/module_64.c
> > > > +++ b/arch/powerpc/kernel/module_64.c
>
> I put back the name of the modified file so that it is easier
> to know what changes we are talking about.
>
> [...]
> > > > +#ifdef CONFIG_LIVEPATCH
> > > > +void clear_relocate_add(Elf64_Shdr *sechdrs,
> > > > +const char *strtab,
> > > > +unsigned int symindex,
> > > > +unsigned int relsec,
> > > > +struct module *me)
> > > > +{
> > > > + unsigned int i;
> > > > + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> > > > + Elf64_Sym *sym;
> > > > + unsigned long *location;
> > > > + const char *symname;
> > > > + u32 *instruction;
> > > > +
> > > > + pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
> > > > + sechdrs[relsec].sh_info);
> > > > +
> > > > + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
> > > > + location = (void
> > > > *)sechdrs[sechdrs[relsec].sh_info].sh_addr
> > > > + + rela[i].r_offset;
> > > > + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
> > > > + + ELF64_R_SYM(rela[i].r_info);
> > > > + symname = me->core_kallsyms.strtab
> > > > + + sym->st_name;
>
> The above calculation is quite complex. It seems to be copied from
> apply_relocate_add(). If I maintained this code I would want to avoid
> the duplication. definitely.
>
>
> > > > +
> > > > + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
> > > > + continue;
>
> Why are we interested only into R_PPC_REL24 relocation types, please?
>
> The code for generating the special SHN_LIVEPATCH section is not in
> the mainline so it is not well defined.
>
> I guess that R_PPC_REL24 relocation type is used by kPatch. Are we
> sure that other relocation types wont be needed?
>
> Anyway, we must warn when an unsupported type is used in SHN_LIVEPATCH
> section here.
>
>
> > > > + /*
> > > > + * reverse the operations in apply_relocate_add() for case
> > > > + * R_PPC_REL24.
> > > > + */
> > > > + if (sym->st_shndx != SHN_UNDEF &&
>
> Do we want to handle SHN_UNDEF symbols here?
>
> The commit a443bf6e8a7674b86221f49 ("powerpc/modules: Add REL24
> relocation support of livepatch symbols") explains that
> R_PPC_REL24 relocations in SHN_LIVEPATCH section are handled
> __like__ relocations in SHN_UNDEF sections.
>
> My understanding is that R_PPC_REL24 reallocation type has
> two variants. Where the variant used in SHN_UNDEF and
> SHN_LIVEPATCH sections need some preprocessing.
>
> Anyway, if this function is livepatch-specific that we should
> clear only symbols from SHN_LIVEPATCH sections. I mean that
> we should probably ignore SHN_UNDEF here.
>
> > > > + sym->st_shndx != SHN_LIVEPATCH)
> > > > + continue;
> > > > +
> > > > +
> > > > + 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..).
>
> > > > + instruction += 1;
> > > > + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> > > > + }
> > > > +
> > > > +}
> > >
> > > This looks like a lot of duplicated code. Isn't it?
> >
> > TBH, I think the duplicated code is not really bad.
>
> How exactly do you mean it, please?
>
> Do you think that the amount of duplicated code is small enough?
> Or that the new function looks better that updating the existing one?
>
> > apply_relocate_add() is a much more complicated function, I would
> > rather not mess it up to make this function a little simpler.
>
> IMHO, the duplicated code is quite tricky. And if we really do
> not need to clear all relocation types then we could avoid
> the duplication another way, for example:
>
> int update_relocate_add(Elf64_Shdr *sechdrs,
>const
Re: [PATCH v8 3/3] ASoC: SOF: Fix deadlock when shutdown a frozen userspace
Hi,
On Thu, 1 Dec 2022, Ricardo Ribalda wrote:
> On Thu, 1 Dec 2022 at 14:22, 'Oliver Neukum' via Chromeos Kdump
> wrote:
> >
> > On 01.12.22 14:03, Ricardo Ribalda wrote:
> > > This patchset does not modify this behaviour. It simply fixes the
> > > stall for kexec().
> > >
> > > The patch that introduced the stall:
> > > 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers
> > > in .shutdown")
> >
> > That patch is problematic. I would go as far as saying that
> > it needs to be reverted.
>
> It fixes a real issue. We have not had any complaints until we tried
> to kexec in the platform.
> I wont recommend reverting it until we have an alternative implementation.
>
> kexec is far less common than suspend/reboot.
I've posted an alternative to ALSA list that reverts the problematic
patch and fixes the problem (the patch was originally addressing)
in a different way:
https://mailman.alsa-project.org/pipermail/alsa-devel/2022-December/209776.html
No changes outside sound/soc/ are needed with this approach.
Br, Kai
Re: (subset) [PATCH 000/606] i2c: Complete conversion to i2c_probe_new
On Fri, 18 Nov 2022 23:35:34 +0100, Uwe Kleine-König wrote:
> since commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new()
> call-back type") from 2016 there is a "temporary" alternative probe
> callback for i2c drivers.
>
> This series completes all drivers to this new callback (unless I missed
> something). It's based on current next/master.
> A part of the patches depend on commit 662233731d66 ("i2c: core:
> Introduce i2c_client_get_device_id helper function"), there is a branch that
> you can pull into your tree to get it:
>
> [...]
Applied all patches that build.
Patches excluded:
- ps8622
- ti-sn65dsi83
- adv7511
Repo: https://cgit.freedesktop.org/drm/drm-misc/
[014/606] drm/bridge: adv7511: Convert to i2c's .probe_new()
(no commit info)
[015/606] drm/bridge/analogix/anx6345: Convert to i2c's .probe_new()
(no commit info)
[016/606] drm/bridge/analogix/anx78xx: Convert to i2c's .probe_new()
(no commit info)
[017/606] drm/bridge: anx7625: Convert to i2c's .probe_new()
(no commit info)
[018/606] drm/bridge: icn6211: Convert to i2c's .probe_new()
(no commit info)
[019/606] drm/bridge: chrontel-ch7033: Convert to i2c's .probe_new()
commit: 8dc6de280f01c0f7b8d40435736f3c975368ad70
[020/606] drm/bridge: it6505: Convert to i2c's .probe_new()
(no commit info)
[021/606] drm/bridge: it66121: Convert to i2c's .probe_new()
(no commit info)
[022/606] drm/bridge: lt8912b: Convert to i2c's .probe_new()
(no commit info)
[023/606] drm/bridge: lt9211: Convert to i2c's .probe_new()
(no commit info)
[024/606] drm/bridge: lt9611: Convert to i2c's .probe_new()
(no commit info)
[025/606] drm/bridge: lt9611uxc: Convert to i2c's .probe_new()
(no commit info)
[026/606] drm/bridge: megachips: Convert to i2c's .probe_new()
(no commit info)
[027/606] drm/bridge: nxp-ptn3460: Convert to i2c's .probe_new()
(no commit info)
[028/606] drm/bridge: parade-ps8622: Convert to i2c's .probe_new()
(no commit info)
[029/606] drm/bridge: sii902x: Convert to i2c's .probe_new()
(no commit info)
[030/606] drm/bridge: sii9234: Convert to i2c's .probe_new()
(no commit info)
[031/606] drm/bridge: sii8620: Convert to i2c's .probe_new()
(no commit info)
[032/606] drm/bridge: tc358767: Convert to i2c's .probe_new()
(no commit info)
[033/606] drm/bridge: tc358768: Convert to i2c's .probe_new()
(no commit info)
[034/606] drm/bridge/tc358775: Convert to i2c's .probe_new()
(no commit info)
[035/606] drm/bridge: ti-sn65dsi83: Convert to i2c's .probe_new()
(no commit info)
[037/606] drm/bridge: tfp410: Convert to i2c's .probe_new()
(no commit info)
rob
