Date: Thu, 12 Nov 2015 13:45:49 -0600
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Chris J Arges <chris.j.arges@canonical.com>
Cc: live-patching@vger.kernel.org, jeyu@redhat.com,
        Seth Jennings <sjenning@redhat.com>, Jiri Kosina <jikos@kernel.org>,
        Vojtech Pavlik <vojtech@suse.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4 v5] livepatch: add old_sympos as disambiguator field
 to klp_func
Message-ID: <20151112194549.GK4038@treble.hsd1.ky.comcast.net>
References: <1447259366-7055-1-git-send-email-chris.j.arges@canonical.com>
 <1447347595-30728-1-git-send-email-chris.j.arges@canonical.com>
 <1447347595-30728-2-git-send-email-chris.j.arges@canonical.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1447347595-30728-2-git-send-email-chris.j.arges@canonical.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4760
Lines: 121

On Thu, Nov 12, 2015 at 10:59:50AM -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 in kallsyms for the patched object
> will be used for patching if it is valid.
> 
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  include/linux/livepatch.h | 20 ++++++++-------
>  kernel/livepatch/core.c   | 65 ++++++++++++++++++++---------------------------
>  2 files changed, 38 insertions(+), 47 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..3d18dff 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 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..479d75e 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -135,13 +135,8 @@ struct klp_find_arg {
>  	const char *objname;
>  	const char *name;
>  	unsigned long addr;
> -	/*
> -	 * 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.
> -	 */
>  	unsigned long count;
> +	unsigned long pos;
>  };
>  
>  static int klp_find_callback(void *data, const char *name,
> @@ -159,36 +154,46 @@ 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 the address. Return 1 when the desired symbol
> +	 * position is found or the search for a unique symbol fails.
>  	 */
>  	args->addr = addr;
>  	args->count++;
> +	if (((args->pos > 0) && (args->count == args->pos)) ||
> +	    ((args->pos == 0) && (args->count > 1)))
> +		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)

One nit about this interface.  I think it would be clearer if the sympos
argument came before the addr argument:

static int klp_find_object_symbol(const char *objname, const char *name,
				  unsigned long sympos, unsigned long addr)

Because:

1. it puts all input arguments before the output argument; and

2. it puts the sympos in its logical place immediately after the
   function name.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/