In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. By default old_sympos will be 0, and patching will only succeed if
the symbol is unique. Specifying a positive value will ensure that
occurrence of the symbol will be used for patching if it is valid.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 5 ++--
kernel/livepatch/core.c | 74 ++++++++++-------------------------------------
2 files changed, 18 insertions(+), 61 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index df7b752..fb968a2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -68,8 +68,8 @@ struct klp_func {
/**
* struct klp_reloc - relocation structure for live patching
* @loc: address where the relocation will be written
- * @val: address of the referenced symbol (optional,
- * vmlinux patches only)
+ * @val: address of the referenced symbol
+ * @sympos: position in kallsyms to disambiguate symbols (optional)
* @type: ELF relocation type
* @name: name of the referenced symbol (for lookup/verification)
* @addend: offset from the referenced symbol
@@ -78,6 +78,7 @@ struct klp_func {
struct klp_reloc {
unsigned long loc;
unsigned long val;
+ unsigned long sympos;
unsigned long type;
const char *name;
int addend;
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
@@ -207,45 +207,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-struct klp_verify_args {
- const char *name;
- const unsigned long addr;
-};
-
-static int klp_verify_callback(void *data, const char *name,
- struct module *mod, unsigned long addr)
-{
- struct klp_verify_args *args = data;
-
- if (!mod &&
- !strcmp(args->name, name) &&
- args->addr == addr)
- return 1;
-
- return 0;
-}
-
-static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
-{
- struct klp_verify_args args = {
- .name = name,
- .addr = addr,
- };
- int ret;
-
- mutex_lock(&module_mutex);
- ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
- mutex_unlock(&module_mutex);
-
- if (!ret) {
- pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
- name, addr);
- return -EINVAL;
- }
-
- return 0;
-}
-
static int klp_find_verify_func_addr(struct klp_object *obj,
struct klp_func *func)
{
@@ -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;
@@ -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);
}
static int klp_write_object_relocations(struct module *pmod,
@@ -292,24 +253,19 @@ static int klp_write_object_relocations(struct module *pmod,
return -EINVAL;
for (reloc = obj->relocs; reloc->name; reloc++) {
- if (!klp_is_module(obj)) {
- ret = klp_verify_vmlinux_symbol(reloc->name,
- reloc->val);
- if (ret)
- return ret;
- } else {
- /* module, reloc->val needs to be discovered */
- if (reloc->external)
- ret = klp_find_external_symbol(pmod,
- reloc->name,
- &reloc->val);
- else
- ret = klp_find_object_symbol(obj->mod->name,
- reloc->name,
- &reloc->val, 0);
- if (ret)
- return ret;
- }
+ /* reloc->val needs to be discovered */
+ if (reloc->external)
+ ret = klp_find_external_symbol(pmod,
+ reloc->name,
+ &reloc->val,
+ reloc->sympos);
+ else
+ ret = klp_find_object_symbol(obj->mod->name,
+ reloc->name,
+ &reloc->val,
+ reloc->sympos);
+ if (ret)
+ return ret;
ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
reloc->val + reloc->addend);
if (ret) {
--
1.9.1
On 11/11/2015 10:29 AM, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default old_sympos will be 0, and patching will only succeed if
Minor typo. old_sympos, should just be sympos.
--chris
> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol will be used for patching if it is valid.
>
> Signed-off-by: Chris J Arges <[email protected]>
> ---
> include/linux/livepatch.h | 5 ++--
> kernel/livepatch/core.c | 74 ++++++++++-------------------------------------
> 2 files changed, 18 insertions(+), 61 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index df7b752..fb968a2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -68,8 +68,8 @@ struct klp_func {
> /**
> * struct klp_reloc - relocation structure for live patching
> * @loc: address where the relocation will be written
> - * @val: address of the referenced symbol (optional,
> - * vmlinux patches only)
> + * @val: address of the referenced symbol
> + * @sympos: position in kallsyms to disambiguate symbols (optional)
> * @type: ELF relocation type
> * @name: name of the referenced symbol (for lookup/verification)
> * @addend: offset from the referenced symbol
> @@ -78,6 +78,7 @@ struct klp_func {
> struct klp_reloc {
> unsigned long loc;
> unsigned long val;
> + unsigned long sympos;
> unsigned long type;
> const char *name;
> int addend;
> 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
> @@ -207,45 +207,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> return -EINVAL;
> }
>
> -struct klp_verify_args {
> - const char *name;
> - const unsigned long addr;
> -};
> -
> -static int klp_verify_callback(void *data, const char *name,
> - struct module *mod, unsigned long addr)
> -{
> - struct klp_verify_args *args = data;
> -
> - if (!mod &&
> - !strcmp(args->name, name) &&
> - args->addr == addr)
> - return 1;
> -
> - return 0;
> -}
> -
> -static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
> -{
> - struct klp_verify_args args = {
> - .name = name,
> - .addr = addr,
> - };
> - int ret;
> -
> - mutex_lock(&module_mutex);
> - ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> - mutex_unlock(&module_mutex);
> -
> - if (!ret) {
> - pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
> - name, addr);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int klp_find_verify_func_addr(struct klp_object *obj,
> struct klp_func *func)
> {
> @@ -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;
>
> @@ -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);
> }
>
> static int klp_write_object_relocations(struct module *pmod,
> @@ -292,24 +253,19 @@ static int klp_write_object_relocations(struct module *pmod,
> return -EINVAL;
>
> for (reloc = obj->relocs; reloc->name; reloc++) {
> - if (!klp_is_module(obj)) {
> - ret = klp_verify_vmlinux_symbol(reloc->name,
> - reloc->val);
> - if (ret)
> - return ret;
> - } else {
> - /* module, reloc->val needs to be discovered */
> - if (reloc->external)
> - ret = klp_find_external_symbol(pmod,
> - reloc->name,
> - &reloc->val);
> - else
> - ret = klp_find_object_symbol(obj->mod->name,
> - reloc->name,
> - &reloc->val, 0);
> - if (ret)
> - return ret;
> - }
> + /* reloc->val needs to be discovered */
> + if (reloc->external)
> + ret = klp_find_external_symbol(pmod,
> + reloc->name,
> + &reloc->val,
> + reloc->sympos);
> + else
> + ret = klp_find_object_symbol(obj->mod->name,
> + reloc->name,
> + &reloc->val,
> + reloc->sympos);
> + if (ret)
> + return ret;
> ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> reloc->val + reloc->addend);
> if (ret) {
>
On Wed, Nov 11, 2015 at 10:29:00AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default old_sympos will be 0, and patching will only succeed if
> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol will be used for patching if it is valid.
>
> Signed-off-by: Chris J Arges <[email protected]>
> ---
> include/linux/livepatch.h | 5 ++--
> kernel/livepatch/core.c | 74 ++++++++++-------------------------------------
> 2 files changed, 18 insertions(+), 61 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index df7b752..fb968a2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -68,8 +68,8 @@ struct klp_func {
> /**
> * struct klp_reloc - relocation structure for live patching
> * @loc: address where the relocation will be written
> - * @val: address of the referenced symbol (optional,
> - * vmlinux patches only)
> + * @val: address of the referenced symbol
> + * @sympos: position in kallsyms to disambiguate symbols (optional)
> * @type: ELF relocation type
> * @name: name of the referenced symbol (for lookup/verification)
> * @addend: offset from the referenced symbol
> @@ -78,6 +78,7 @@ struct klp_func {
> struct klp_reloc {
> unsigned long loc;
> unsigned long val;
> + unsigned long sympos;
> unsigned long type;
> const char *name;
> int addend;
I think 'val' can be removed from this struct, since it's now basically
just a private variable of klp_write_object_relocations().
> 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
> @@ -207,45 +207,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> return -EINVAL;
> }
>
> -struct klp_verify_args {
> - const char *name;
> - const unsigned long addr;
> -};
> -
> -static int klp_verify_callback(void *data, const char *name,
> - struct module *mod, unsigned long addr)
> -{
> - struct klp_verify_args *args = data;
> -
> - if (!mod &&
> - !strcmp(args->name, name) &&
> - args->addr == addr)
> - return 1;
> -
> - return 0;
> -}
> -
> -static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
> -{
> - struct klp_verify_args args = {
> - .name = name,
> - .addr = addr,
> - };
> - int ret;
> -
> - mutex_lock(&module_mutex);
> - ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> - mutex_unlock(&module_mutex);
> -
> - if (!ret) {
> - pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
> - name, addr);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int klp_find_verify_func_addr(struct klp_object *obj,
> struct klp_func *func)
> {
> @@ -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.
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);
> }
>
> static int klp_write_object_relocations(struct module *pmod,
> @@ -292,24 +253,19 @@ static int klp_write_object_relocations(struct module *pmod,
> return -EINVAL;
>
> for (reloc = obj->relocs; reloc->name; reloc++) {
> - if (!klp_is_module(obj)) {
> - ret = klp_verify_vmlinux_symbol(reloc->name,
> - reloc->val);
> - if (ret)
> - return ret;
> - } else {
> - /* module, reloc->val needs to be discovered */
> - if (reloc->external)
> - ret = klp_find_external_symbol(pmod,
> - reloc->name,
> - &reloc->val);
> - else
> - ret = klp_find_object_symbol(obj->mod->name,
> - reloc->name,
> - &reloc->val, 0);
> - if (ret)
> - return ret;
> - }
> + /* reloc->val needs to be discovered */
> + if (reloc->external)
> + ret = klp_find_external_symbol(pmod,
> + reloc->name,
> + &reloc->val,
> + reloc->sympos);
> + else
> + ret = klp_find_object_symbol(obj->mod->name,
> + reloc->name,
> + &reloc->val,
> + reloc->sympos);
> + if (ret)
> + return ret;
> ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> reloc->val + reloc->addend);
> if (ret) {
> --
> 1.9.1
--
Josh
On Wed, 11 Nov 2015, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default old_sympos will be 0, and patching will only succeed if
> the symbol is unique. Specifying a positive value will ensure that
> occurrence of the symbol will be used for patching if it is valid.
Again "...occurrence of the symbol in kallsyms for the patched object
will be used..."
> 2 files changed, 18 insertions(+), 61 deletions(-)
And this is indeed great.
Miroslav
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.
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
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
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
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
On Fri 2015-11-13 10:59:34, Josh Poimboeuf wrote:
> 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'?
Yeah, I messed the meaning of the various terms. Also I did not have a
good picture about the difference in handling global symbols in normal
userspace binaries and kernel.
> 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.
Thanks you and Mirek for explanation. I hope that I will send less
confusing mails from now on.
Best Regards,
Petr