Re: [PATCH 3/4 v5] livepatch: add sympos as disambiguator field to klp_reloc

[Petr Mladek <pmladek@suse.com>]

On Thu 2015-11-12 10:59:52, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default sympos will be 0, and patching will only succeed if
> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol in kallsyms for the patched object will be used
> for patching if it is valid.
> 
> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> as they are no longer used.
> 
> >From the klp_reloc structure remove val, as it can be refactored as a
> local variable in klp_write_object_relocations.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 4cd079c..071895f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -275,29 +237,30 @@ static int klp_write_object_relocations(struct module *pmod,
>  		return -EINVAL;
>  
>  	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		if (!klp_is_module(obj)) {
> -			ret = klp_verify_vmlinux_symbol(reloc->name,
> -							reloc->val);
> -			if (ret)
> -				return ret;
> -		} else {
> -			/* module, reloc->val needs to be discovered */
> -			if (reloc->external)
> -				ret = klp_find_external_symbol(pmod,
> -							       reloc->name,
> -							       &reloc->val);
> -			else
> -				ret = klp_find_object_symbol(obj->mod->name,
> -							     reloc->name,
> -							     &reloc->val, 0);
> -			if (ret)
> -				return ret;
> -		}
> +		/* discover the address of the referenced symbol */
> +		if (reloc->external) {
> +			/*
> +			 * external symbols aren't specified by the user
> +			 * therefore sympos should not be used
> +			 */
> +			if (reloc->sympos) {
> +				pr_err("symbol '%s' is external and has sympos %lu\n",
> +				       reloc->name, reloc->sympos);
> +				return -EINVAL;
> +			}

To be precise, reloc->sympos == 1 is a valid value. Note that it will show
"symbol,1" in sysfs. I suggest the following to keep it simple:

			if (reloc->sympos && (reloc->sympos != 1)) {
				pr_err("relocated external symbols must be unique but '%s' has sympos %lu\n",
				       reloc->name, reloc->sympos);
				return -EINVAL;
			}

Then we could even get rid of the comment.

Huh, I am looking forward to get rid of the external symbols ;-)

Thank you,
Petr