Re: [PATCH v10 02/15] livepatch: avoid position-based search if `-z unique-symbol` is available

[Josh Poimboeuf]

On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote:
> Position-based search, which means that if there are several symbols
> with the same name, the user needs to additionally provide the
> "index" of a desired symbol, is fragile. For example, it breaks
> when two 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). It can be used to effectively prevent from having
> any ambiguity when referring to a symbol by its name.

In the patch description can you also give the version of binutils (and
possibly other linkers) which have the flag?

> Check for its availability and always prefer when the livepatching
> is on. It can be used unconditionally later on after broader testing
> on a wide variety of machines, but for now let's stick to the actual
> CONFIG_LIVEPATCH=y case, which is true for most of distro configs
> anyways.

Has anybody objected to just enabling it for *all* configs, not just for
livepatch?

I'd much prefer that: the less "special" livepatch is (and the distros
which enable it), the better.  And I think having unique symbols would
benefit some other components.

> +++ 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;

There's no real need to do this.  The code already works as-is, even if
there are no unique symbols.

Even if there are no duplicates, there's little harm in going through
all the symbols anyway, to check for errors just in case something
unexpected happened with the linking (unexpected duplicate) or the patch
creation (unexpected sympos).  It's not a hot path, so performance isn't
really a concern.

When the old linker versions eventually age out, we can then go strip
out all the sympos stuff.

> @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	else
>  		kallsyms_on_each_symbol(klp_find_callback, &args);
>  
> +	/*
> +	 * If the LD's `-z unique-symbol` flag is available and enabled,
> +	 * sympos checks are not relevant.
> +	 */
> +	if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL))
> +		sympos = 0;
> +

Similarly, I don't see a need for this.  If the patch is legit then
sympos should already be zero.  If not, an error gets reported and the
patch fails to load.

-- 
Josh