Date: Fri, 13 Nov 2015 10:59:34 -0600
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Chris J Arges <chris.j.arges@canonical.com>, 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 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
Content-Length: 3602
Lines: 84

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
--
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/