Re: [PATCH v6] livepatch: Clear relocation targets on a module removal

[Song Liu <song@kernel.org>]

On Fri, Dec 9, 2022 at 5:54 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/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.
>
> Do I still need to explain how bad idea was the code duplication,
> please?

No objections that code duplication is not ideal. It is just that sometimes
it is hard to follow the difference between functions with similar names.
Well, it is probably my own problem.

Thanks,
Song