From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932117AbbKLTpw (ORCPT ); Thu, 12 Nov 2015 14:45:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49254 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753163AbbKLTpv (ORCPT ); Thu, 12 Nov 2015 14:45:51 -0500 Date: Thu, 12 Nov 2015 13:45:49 -0600 From: Josh Poimboeuf To: Chris J Arges Cc: live-patching@vger.kernel.org, jeyu@redhat.com, Seth Jennings , Jiri Kosina , Vojtech Pavlik , 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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