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

Borislav Petkov
2021-12-30T11:10:37Z

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.