From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753800AbbKLTTU (ORCPT ); Thu, 12 Nov 2015 14:19:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60894 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbbKLTTT (ORCPT ); Thu, 12 Nov 2015 14:19:19 -0500 Date: Thu, 12 Nov 2015 13:19:17 -0600 From: Josh Poimboeuf To: Petr Mladek Cc: Chris J Arges , live-patching@vger.kernel.org, jeyu@redhat.com, Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Message-ID: <20151112191917.GJ4038@treble.hsd1.ky.comcast.net> References: <20151103200608.GQ27488@treble.redhat.com> <1447259366-7055-1-git-send-email-chris.j.arges@canonical.com> <1447259366-7055-3-git-send-email-chris.j.arges@canonical.com> <20151111175731.GF5331@treble.redhat.com> <20151112143157.GS2599@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151112143157.GS2599@pathway.suse.cz> 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 03:31:58PM +0100, Petr Mladek wrote: > On Wed 2015-11-11 11:57:31, Josh Poimboeuf wrote: > > On Wed, Nov 11, 2015 at 10:29:00AM -0600, Chris J Arges wrote: > > > 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 > > > @@ -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. > > Heh, I always had troubles to understand the meaning of > this external stuff. > > > 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); > > > } > > Please, do you have an example when this code will be used? > Do we really need to relocate symbols within the patch module this > way? > > My understanding is that it would be used to relocate symbols > between various .o files that are used to produce the patch module. > IMHO, the only situation is that you want to access a static > symbol from another .o file. But this is not used in normal modules. > It does not look like a real life scenario. I think I originated the 'external' concept, but I'm also not a fan of it. If we can find a way to get rid of it or improve it, that would be great. There are two cases for external symbols: 1. Accessing a global symbol in another .o file in the patch module. For an example of a patch which does this, see: https://github.com/dynup/kpatch/blob/master/test/integration/f22/module-call-external.patch In that example, notice that kpatch_string() function is global (not static), and is not exported. It *is* actually a real world scenario. But I do think we're currently handling it wrong. kpatch-build isn't smart enough to determine the difference between the use of an exported symbol and a global one that's in another .o in the module. We can probably fix that by looking at Module.symvers. So I think we can get rid of this case. 2. Accessing an exported symbol which lives in a module. With Chris's patches, we now don't have any ambiguity for specifying module symbols, so I think we can get rid of this case too. So I *think* we can get rid of 'external' completely. But I could be overlooking something. I'd rather implement the change in kpatch-build first to make 100% sure we can actually get rid of it. Also, I'd ask that we hold off on this patch for now until we get a chance to add support for it in kpatch-build. Then at that point we can just remove all the 'external' stuff. > It is indepednt on this patch set but it might make it easier. > What about doing this cleaup, first? > > > From 095f74fb92205177b138bb6215e4e5fd59dca8db Mon Sep 17 00:00:00 2001 > From: Petr Mladek > Date: Thu, 12 Nov 2015 15:11:00 +0100 > Subject: [PATCH] livepatch: Simplify code for relocated external symbols > > The livepatch module might be linked from several .o files. > All symbols that need to be shared between these .o files > should be exported. This is a normal programming practice. > I do not see any reason to access static symbols between > these .o files. > > This patch removes the search for the static symbols within > the livepatch module. It makes it easier to understand > the meaning of the external flag and klp_find_external_symbol() > function. > > Signed-off-by: Petr Mladek > --- > include/linux/livepatch.h | 3 ++- > kernel/livepatch/core.c | 12 +++++------- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 31db7a05dd36..77b84732ee05 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -71,7 +71,8 @@ struct klp_func { > * @type: ELF relocation type > * @name: name of the referenced symbol (for lookup/verification) > * @addend: offset from the referenced symbol > - * @external: symbol is either exported or within the live patch module itself > + * @external: set for external symbols that are accessed from this object > + * but defined outside; they must be exported > */ > struct klp_reloc { > unsigned long loc; > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 6e5344112419..138f11420883 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -258,26 +258,24 @@ static int klp_find_verify_func_addr(struct klp_object *obj, > } > > /* > - * external symbols are located outside the parent object (where the parent > - * object is either vmlinux or the kmod being patched). > + * External symbols are exported symbols that are defined outside both > + * the patched object and the patch. > */ > static int klp_find_external_symbol(struct module *pmod, const char *name, > unsigned long *addr) > { > const struct kernel_symbol *sym; > + int ret = -EINVAL; > > - /* first, check if it's an exported symbol */ > preempt_disable(); > sym = find_symbol(name, NULL, NULL, true, true); > if (sym) { > *addr = sym->value; > - preempt_enable(); > - return 0; > + ret = 0; > } > preempt_enable(); > > - /* otherwise check if it's in another .o within the patch module */ > - return klp_find_object_symbol(pmod->name, name, addr); > + return ret; > } > > static int klp_write_object_relocations(struct module *pmod, > -- > 1.8.5.6 > > -- > To unsubscribe from this list: send the line "unsubscribe live-patching" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Josh