Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Nov 11, 2015 at 10:28:59AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. 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. Finally, old_addr is now an internal structure element and not to
> be specified by the user.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  include/linux/livepatch.h | 20 ++++++++++--------
>  kernel/livepatch/core.c   | 53 +++++++++++++++++++++++------------------------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..df7b752 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -37,8 +37,9 @@ enum klp_state {
>   * struct klp_func - function structure for live patching
>   * @old_name:	name of the function to be patched
>   * @new_func:	pointer to the patched function code
> - * @old_addr:	a hint conveying at what address the old function
> - *		can be found (optional, vmlinux patches only)
> + * @old_sympos: a hint indicating which symbol position the old function
> + *		can be found (optional)
> + * @old_addr:	the address of the function being patched
>   * @kobj:	kobject for sysfs resources
>   * @state:	tracks function-level patch application state
>   * @stack_node:	list node for klp_ops func_stack list
> @@ -47,17 +48,18 @@ struct klp_func {
>  	/* external */
>  	const char *old_name;
>  	void *new_func;
> +
>  	/*
> -	 * The old_addr field is optional and can be used to resolve
> -	 * duplicate symbol names in the vmlinux object.  If this
> -	 * information is not present, the symbol is located by name
> -	 * with kallsyms. If the name is not unique and old_addr is
> -	 * not provided, the patch application fails as there is no
> -	 * way to resolve the ambiguity.
> +	 * The old_sympos field is optional and can be used to resolve
> +	 * duplicate symbol names in livepatch objects. If this field is zero,
> +	 * it is expected the symbol is unique, otherwise patching fails. If
> +	 * this value is greater than zero then that occurrence of the symbol
> +	 * in kallsyms is used.

I would clarify this:

...occurrence of the symbol in kallsyms *for the given object* is used.

>  	 */
> -	unsigned long old_addr;
> +	unsigned long old_sympos;
>  
>  	/* internal */
> +	unsigned long old_addr;
>  	struct kobject kobj;
>  	enum klp_state state;
>  	struct list_head stack_node;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..26f9778 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,7 @@ struct klp_find_arg {
>  	 * name in the same object.
>  	 */
>  	unsigned long count;
> +	unsigned long pos;

There's a comment above this that says:

	/*
	 * If count == 0, the symbol was not found. If count == 1, a unique
	 * match was found and addr is set.  If count > 1, there is
	 * unresolvable ambiguity among "count" number of symbols with the same
	 * name in the same object.
	 */

That comment is no longer accurate and can probably be removed since IMO
the purpose of 'count' is obvious.

>  };
>  
>  static int klp_find_callback(void *data, const char *name,
> @@ -159,36 +160,45 @@ static int klp_find_callback(void *data, const char *name,
>  		return 0;
>  
>  	/*
> -	 * args->addr might be overwritten if another match is found
> -	 * but klp_find_object_symbol() handles this and only returns the
> -	 * addr if count == 1.
> +	 * increment and assign address, return only if checking pos and
> +	 * it matches count.
>  	 */
> -	args->addr = addr;
>  	args->count++;
> +	args->addr = addr;
> +	if ((args->pos > 0) && (args->count == args->pos))
> +		return 1;
>  
>  	return 0;
>  }
>  
>  static int klp_find_object_symbol(const char *objname, const char *name,
> -				  unsigned long *addr)
> +				  unsigned long *addr, unsigned long sympos)
>  {
>  	struct klp_find_arg args = {
>  		.objname = objname,
>  		.name = name,
>  		.addr = 0,
> -		.count = 0
> +		.count = 0,
> +		.pos = sympos,
>  	};
>  
>  	mutex_lock(&module_mutex);
>  	kallsyms_on_each_symbol(klp_find_callback, &args);
>  	mutex_unlock(&module_mutex);
>  
> -	if (args.count == 0)
> +	/*
> +	 * Ensure an address was found. If sympos is 0, ensure symbol is unique;
> +	 * otherwise ensure the symbol position count matches sympos.
> +	 */
> +	if (args.addr == 0)
>  		pr_err("symbol '%s' not found in symbol table\n", name);
> -	else if (args.count > 1)
> +	else if (args.count > 1 && sympos == 0) {
>  		pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
>  		       args.count, name, objname);
> -	else {
> +	} 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 {
>  		*addr = args.addr;
>  		return 0;
>  	}
> @@ -239,22 +249,11 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
>  static int klp_find_verify_func_addr(struct klp_object *obj,
>  				     struct klp_func *func)
>  {
> -	int ret;
> -
> -#if defined(CONFIG_RANDOMIZE_BASE)
> -	/* If KASLR has been enabled, adjust old_addr accordingly */
> -	if (kaslr_enabled() && func->old_addr)
> -		func->old_addr += kaslr_offset();
> -#endif
> -
> -	if (!func->old_addr || klp_is_module(obj))
> -		ret = klp_find_object_symbol(obj->name, func->old_name,
> -					     &func->old_addr);
> -	else
> -		ret = klp_verify_vmlinux_symbol(func->old_name,
> -						func->old_addr);
> -
> -	return ret;
> +	/*
> +	 * Verify the symbol, find old_addr, and write it to the structure.
> +	 */
> +	return klp_find_object_symbol(obj->name, func->old_name,
> +				      &func->old_addr, func->old_sympos);

klp_find_verify_func_addr() is no longer correctly named and can
probably be removed since klp_init_object_loaded() can call
klp_find_object_symbol() directly.

>  }
>  
>  /*
> @@ -277,7 +276,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);
> +	return klp_find_object_symbol(pmod->name, name, addr, 0);
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +306,7 @@ static int klp_write_object_relocations(struct module *pmod,
>  			else
>  				ret = klp_find_object_symbol(obj->mod->name,
>  							     reloc->name,
> -							     &reloc->val);
> +							     &reloc->val, 0);
>  			if (ret)
>  				return ret;
>  		}
> -- 
> 1.9.1
> 

-- 
Josh