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

[Josh Poimboeuf <jpoimboe@redhat.com>]

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 <pmladek@suse.com>
> 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 <pmladek@suse.com>
> ---
>  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