Re: [PATCH v9 02/15] livepatch: use `-z unique-symbol` if available to nuke pos-based search

[Fāng-ruì Sòng]

On Thu, Dec 30, 2021 at 3:11 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Thu, Dec 23, 2021 at 01:21:56AM +0100, Alexander Lobakin wrote:
> > [PATCH v9 02/15] livepatch: use `-z unique-symbol` if available to nuke pos-based search
>
> nuke?
>
> I think you wanna say something about avoiding position-based search if
> toolchain supports -z ...
>
> > Position-based search, which means that if we have several symbols
> > with the same name, we additionally need to provide an "index" of
> > the desired symbol, is fragile. Par exemple, it breaks when two
>                                   ^^^^^^^^^^^^
>
> We already have hard time with the English in commit messages, let's
> avoid the French pls.
>
> > symbols with the same name are located in different sections.
> >
> > Since a while, LD has a flag `-z unique-symbol` which appends
> > numeric suffixes to the functions with the same name (in symtab
> > and strtab).
> > Check for its availability and always prefer when the livepatching
> > is on.
>
> Why only then?
>
> It looks to me like we want this unconditionally, no?
>
> > This needs a little adjustment to the modpost to make it
> > strip suffixes before adding exports.
> >
> > depmod needs some treatment as well, tho its false-positibe warnings
>
> Unknown word [false-positibe] in commit message, suggestions:
>         ['false-positive', 'false-positioned', 'prepositional']
>
> Please introduce a spellchecker into your patch creation workflow.
>
> > about unknown symbols are harmless and don't alter the return code.
> > And there is a bunch more livepatch code to optimize-out after
> > introducing this, but let's leave it for later.
>
> ...
>
> > @@ -171,17 +173,21 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> >
> >       /*
> >        * Ensure an address was found. If sympos is 0, ensure symbol is unique;
> > -      * otherwise ensure the symbol position count matches sympos.
> > +      * otherwise ensure the symbol position count matches sympos. If the LD
> > +      * `-z unique` flag is enabled, sympos checks are not relevant.
>            ^^^^^^^^^^^
>
> -z unique-symbol
>
> >        */
> > -     if (args.addr == 0)
> > +     if (args.addr == 0) {
> >               pr_err("symbol '%s' not found in symbol table\n", name);
> > -     else if (args.count > 1 && sympos == 0) {
> > +     } else if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) {
> > +             goto out_ok;
>
> This is silly - just do it all here.
>
> > +     } else if (args.count > 1 && sympos == 0) {
> >               pr_err("unresolvable ambiguity for symbol '%s' in object '%s'\n",
> >                      name, objname);
> >       } else if (sympos != args.count && sympos > 0) {
> >               pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
> >                      sympos, name, objname ? objname : "vmlinux");
> >       } else {
> > +out_ok:
> >               *addr = args.addr;
> >               return 0;
> >       }
>
> Looks straight-forward otherwise but I'm no livepatcher so I'd prefer if
> they have a look too.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>

Apologies since I haven't read the patch series.

The option does not exist in ld.lld and I am a bit concerning about
its semantics: https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol

I thought that someone forwarded my comments (originally posted months
on a feature request ago) here but seems not.
(I am a ld.lld maintainer.)