From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933369AbbKMQ7h (ORCPT ); Fri, 13 Nov 2015 11:59:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59491 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932444AbbKMQ7g (ORCPT ); Fri, 13 Nov 2015 11:59:36 -0500 Date: Fri, 13 Nov 2015 10:59:34 -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: <20151113165934.GA22014@treble.redhat.com> 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> <20151112191917.GJ4038@treble.hsd1.ky.comcast.net> <20151113135442.GU2599@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151113135442.GU2599@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 Fri, Nov 13, 2015 at 02:54:42PM +0100, Petr Mladek wrote: > On Thu 2015-11-12 13:19:17, Josh Poimboeuf wrote: > > 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; > > > > > > > > > > > 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. > > Mirek helped me to understand it. The symbol is exported if you > compile the above patch from sources. kpatch produces the patch by > pecking out the newly created symbols without looking if they > are newly exported. I hope that we got it right. Hm, I don't really follow what you're saying. Are we using different definitions of 'exported'? By exported, I mean the use of the EXPORT_SYMBOL macro which makes the symbol available for use by other modules. The above patch doesn't use the EXPORT_SYMBOL macro, so the kpatch_string symbol isn't exported, and can't be used by other kernel modules. However, the symbol *is* global and can be used by other .o files within the patch module. > > 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. > > That would be lovely. > > > > 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. > > Fair enough. > > > > Then at that point we can just remove all the 'external' stuff. > > I see. Each symbol is part of an object. Even the exported symbols > need to be listed for the related object. We do not need external at > all if the patch is compiled from sources or if we check for newly exported > symbols in the binaries. -- Josh