Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Nov 11, 2015 at 10:29:00AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default old_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 will be used for patching if it is valid.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  include/linux/livepatch.h |  5 ++--
>  kernel/livepatch/core.c   | 74 ++++++++++-------------------------------------
>  2 files changed, 18 insertions(+), 61 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index df7b752..fb968a2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -68,8 +68,8 @@ struct klp_func {
>  /**
>   * struct klp_reloc - relocation structure for live patching
>   * @loc:	address where the relocation will be written
> - * @val:	address of the referenced symbol (optional,
> - *		vmlinux	patches only)
> + * @val:	address of the referenced symbol
> + * @sympos:     position in kallsyms to disambiguate symbols (optional)
>   * @type:	ELF relocation type
>   * @name:	name of the referenced symbol (for lookup/verification)
>   * @addend:	offset from the referenced symbol
> @@ -78,6 +78,7 @@ struct klp_func {
>  struct klp_reloc {
>  	unsigned long loc;
>  	unsigned long val;
> +	unsigned long sympos;
>  	unsigned long type;
>  	const char *name;
>  	int addend;

I think 'val' can be removed from this struct, since it's now basically
just a private variable of klp_write_object_relocations().

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 26f9778..4eb8691 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -207,45 +207,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -struct klp_verify_args {
> -	const char *name;
> -	const unsigned long addr;
> -};
> -
> -static int klp_verify_callback(void *data, const char *name,
> -			       struct module *mod, unsigned long addr)
> -{
> -	struct klp_verify_args *args = data;
> -
> -	if (!mod &&
> -	    !strcmp(args->name, name) &&
> -	    args->addr == addr)
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
> -{
> -	struct klp_verify_args args = {
> -		.name = name,
> -		.addr = addr,
> -	};
> -	int ret;
> -
> -	mutex_lock(&module_mutex);
> -	ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> -	mutex_unlock(&module_mutex);
> -
> -	if (!ret) {
> -		pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
> -			name, addr);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  static int klp_find_verify_func_addr(struct klp_object *obj,
>  				     struct klp_func *func)
>  {
> @@ -261,7 +222,7 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
>   * object is either vmlinux or the kmod being patched).
>   */
>  static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +				    unsigned long *addr, unsigned long sympos)
>  {
>  	const struct kernel_symbol *sym;
>  

For "external" symbols, the object isn't specified by the user, and
since sympos is per-object, the value of sympos is undefined.   Instead
I think it should always pass 0 to klp_find_object_symbol() below.

In line with that, since reloc->external and reloc->sympos don't mix,
maybe klp_write_object_relocations() should return -EINVAL if external
is set and sympos is non-zero.

> @@ -276,7 +237,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>  	preempt_enable();
>  
>  	/* otherwise check if it's in another .o within the patch module */
> -	return klp_find_object_symbol(pmod->name, name, addr, 0);
> +	return klp_find_object_symbol(pmod->name, name, addr, sympos);
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> @@ -292,24 +253,19 @@ 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;
> -		}
> +		/* reloc->val needs to be discovered */
> +		if (reloc->external)
> +			ret = klp_find_external_symbol(pmod,
> +						       reloc->name,
> +						       &reloc->val,
> +						       reloc->sympos);
> +		else
> +			ret = klp_find_object_symbol(obj->mod->name,
> +						     reloc->name,
> +						     &reloc->val,
> +						     reloc->sympos);
> +		if (ret)
> +			return ret;
>  		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>  					     reloc->val + reloc->addend);
>  		if (ret) {
> -- 
> 1.9.1

-- 
Josh