Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc

[Petr Mladek <pmladek@suse.com>]

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.

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

Best Regards,
Petr