On Mon, Dec 12, 2022 at 9:12 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Fri 2022-12-09 11:59:35, Song Liu wrote:
> > On Fri, Dec 9, 2022 at 3:41 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > > On Mon 2022-11-28 17:57:06, Song Liu wrote:
> > > > On Fri, Nov 18, 2022 at 8:24 AM Petr Mladek <pmladek@xxxxxxxx> 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().
>
> Please, avoid code duplication as much as possible. Especially,
> the two checks is_mprofile_ftrace_call() and
> instr_is_relative_link_branch() must be shared. IMHO, it is
> the only way to keep the code maintainable. We must make sure that
> we will clear the instructions only when they were patched. And
> copy pasting various tricky exceptions is a way to hell.
>
>
> > > 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,
> > > sechdrs[relsec].sh_info);
> > > } else {
> > > pr_debug("Clearing ADD relocate section %u\n", relsec");
> > > }
> > >
> > > for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
> > > /* This is where to make the change */
> > > location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
> > > + rela[i].r_offset;
> > > /* This is the symbol it is referring to */
> > > sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
> > > + ELF64_R_SYM(rela[i].r_info);
> > >
> > > r_type = ELF64_R_TYPE(rela[i].r_info);
> > >
> > > if (apply) {
> > > apply_relocate_location(sym, location, r_type, rela[i].r_addend);
> > > } else {
> > > clear_relocate_location(sym, location, r_type);
> > > }
> >
> > I personally don't like too many "if (apply) {...} else {...}" patterns in
> > a function. And these new functions confuse me sometimes:
> >
> > update_relocate_add(..., apply);
> > apply_relocate_location();
> > clear_relocate_location();
>
> Feel free to come up with another way how to avoid code duplication.
>
> > And I did think there wasn't too much duplicated code.
>
> I think that it looks very different when you are writing or reading
> or mantainting the code. It might be easier to write code and modify
> it. It is more complicated to find the differences later. Also it is
> more complicated to do the same changes many times when the common
> code is updated later.
Agreed that we need to keep the code understandable and maintainable.
And I need to try harder to understand how these codes work as-is.
Thanks,
Song
