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

On Thu, Dec 30, 2021 at 3:11 AM Borislav Petkov [off-list ref] wrote:

On Thu, Dec 23, 2021 at 01:21:56AM +0100, Alexander Lobakin wrote:

quoted

[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 ...

quoted

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.

quoted

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?

quoted

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.

quoted

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.

...

quoted

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

quoted

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

quoted

+     } 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.)