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/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
