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

Miroslav Benes
2022-01-03T13:46:08Z

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name,
> args->count++;
>
> /*
> - * Finish the search when the symbol is found for the desired position
> - * or the position is not defined for a non-unique symbol.
> + * Finish the search when unique symbol names are enabled
> + * or the symbol is found for the desired position or the
> + * position is not defined for a non-unique symbol.
> */
> - if ((args->pos && (args->count == args->pos)) ||
> - (!args->pos && (args->count > 1)))
> + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) ||
> + (args->pos && args->count == args->pos) ||
> + (!args->pos && args->count > 1))
> return 1;
>
> return 0;
> @@ -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.
> */
> - 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;
> + } 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;
> }

I think it would be better to return to something like

if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL))
sympos = 0;

in klp_find_object_symbol() just after kallsyms search is performed.

You would not need the above changes at all. I did not like it before when
sympos clearing was proposed, but the situation was different back then.
'-z unique-symbol' was not available. It has changed and we have a
guarantee that symbols are unique now. There would not be any impact on
the user.

What do you think?

Miroslav