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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
