Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1260841 > unrolled thread
| Started by | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| First post | 2015-11-02 19:00 +0100 |
| Last post | 2015-11-10 10:10 +0100 |
| Articles | 20 on this page of 43 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 19:00 +0100
Re: livepatch: old_name.number scheme in livepatch sysfs directory Jessica Yu <jeyu@redhat.com> - 2015-11-02 20:20 +0100
Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:00 +0100
Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 21:20 +0100
Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:40 +0100
[PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 00:10 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 11:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 12:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Petr Mladek <pmladek@suse.com> - 2015-11-03 13:50 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Jiri Kosina <jikos@kernel.org> - 2015-11-03 21:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 21:10 +0100
[PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:30 +0100
Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 18:50 +0100
Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
[PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
Re: [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:10 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
[PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:00 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-12 15:40 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 20:20 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-13 15:00 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-13 18:00 +0100
Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 17:20 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 18:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 21:50 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-04 11:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-04 17:10 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-04 17:20 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-05 16:20 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 17:00 +0100
Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-05 17:10 +0100
[PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-09 17:20 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-09 22:00 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-10 00:10 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 06:00 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 09:50 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 14:50 +0100
Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 10:10 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-11 19:00 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qtHQC-5Vc-7@gated-at.bofh.it> |
| In reply to | #1267304 |
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 <chris.j.arges@canonical.com>
> ---
> 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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2015-11-12 15:40 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qu1cC-1EX-23@gated-at.bofh.it> |
| In reply to | #1267350 |
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 <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 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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-12 20:20 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qu5zz-4wv-15@gated-at.bofh.it> |
| In reply to | #1267988 |
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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2015-11-13 15:00 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qun3w-77o-79@gated-at.bofh.it> |
| In reply to | #1268219 |
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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-13 18:00 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qupRE-up-11@gated-at.bofh.it> |
| In reply to | #1268956 |
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/
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-12 11:30 +0100 |
| Subject | Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc |
| Message-ID | <qtXiG-7Fc-7@gated-at.bofh.it> |
| In reply to | #1267304 |
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 -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-03 16:00 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qqLe2-41D-11@gated-at.bofh.it> |
| In reply to | #1261385 |
On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
> On Mon, 2 Nov 2015, Chris J Arges wrote:
>
> [...]
>
> > +static int klp_get_func_pos_callback(void *data, const char *name,
> > + struct module *mod, unsigned long addr)
> > +{
> > + struct klp_find_arg *args = data;
> > +
> > + if ((mod && !args->objname) || (!mod && args->objname))
> > + return 0;
> > +
> > + if (strcmp(args->name, name))
> > + return 0;
> > +
> > + if (args->objname && strcmp(args->objname, mod->name))
> > + return 0;
> > +
> > + /* on address match, return 1 to break kallsyms_on_each_symbol loop */
> > + if (args->addr == addr)
> > + return 1;
> > +
> > + /* if we don't match addr, count instance of named symbol */
> > + args->count++;
> > +
> > + return 0;
> > +}
> > +
> > +static int klp_get_func_pos(struct klp_object *obj, struct klp_func *func)
> > +{
> > + struct klp_find_arg args = {
> > + .objname = obj->name,
> > + .name = func->old_name,
> > + .addr = func->old_addr,
> > + .count = 0,
> > + };
> > +
> > + mutex_lock(&module_mutex);
> > + kallsyms_on_each_symbol(klp_get_func_pos_callback, &args);
> > + mutex_unlock(&module_mutex);
> > +
> > + return args.count;
> > +}
> > +
> > static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > {
> > INIT_LIST_HEAD(&func->stack_node);
> > func->state = KLP_DISABLED;
> >
> > return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> > - &obj->kobj, "%s", func->old_name);
> > + &obj->kobj, "%s,%d", func->old_name,
> > + klp_get_func_pos(obj, func));
> > }
>
> There is a problem which I missed before. klp_init_func() is called before
> klp_find_verify_func_addr() in klp_init_object(). This means that
> func->old_addr is either not verified yet or worse it is still 0. This
> means that klp_get_func_pos_callback() never returns 1 and is thus called
> on each symbol. So if you for example patched cmdline_proc_show the
> resulting directory in sysfs would be called cmdline_proc_show,1 because
> addr is never matched. Had old_addr been specified the name would have
> been probably correct, but not for sure.
>
> This should be fixed as well.
Even worse, klp_init_func() can be called even if the object hasn't been
loaded. In that case there's no way to know what the value of n is, and
therefore no way to reliably create the sysfs entry.
Should we create "func,n" in klp_init_object_loaded() instead of
klp_init_func()?
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-03 17:20 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qqMtt-57V-25@gated-at.bofh.it> |
| In reply to | #1261555 |
On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
> > On Mon, 2 Nov 2015, Chris J Arges wrote:
> >
> > [...]
> >
> > > +static int klp_get_func_pos_callback(void *data, const char *name,
> > > + struct module *mod, unsigned long addr)
> > > +{
> > > + struct klp_find_arg *args = data;
> > > +
> > > + if ((mod && !args->objname) || (!mod && args->objname))
> > > + return 0;
> > > +
> > > + if (strcmp(args->name, name))
> > > + return 0;
> > > +
> > > + if (args->objname && strcmp(args->objname, mod->name))
> > > + return 0;
> > > +
> > > + /* on address match, return 1 to break kallsyms_on_each_symbol loop */
> > > + if (args->addr == addr)
> > > + return 1;
> > > +
> > > + /* if we don't match addr, count instance of named symbol */
> > > + args->count++;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int klp_get_func_pos(struct klp_object *obj, struct klp_func *func)
> > > +{
> > > + struct klp_find_arg args = {
> > > + .objname = obj->name,
> > > + .name = func->old_name,
> > > + .addr = func->old_addr,
> > > + .count = 0,
> > > + };
> > > +
> > > + mutex_lock(&module_mutex);
> > > + kallsyms_on_each_symbol(klp_get_func_pos_callback, &args);
> > > + mutex_unlock(&module_mutex);
> > > +
> > > + return args.count;
> > > +}
> > > +
> > > static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > > {
> > > INIT_LIST_HEAD(&func->stack_node);
> > > func->state = KLP_DISABLED;
> > >
> > > return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> > > - &obj->kobj, "%s", func->old_name);
> > > + &obj->kobj, "%s,%d", func->old_name,
> > > + klp_get_func_pos(obj, func));
> > > }
> >
> > There is a problem which I missed before. klp_init_func() is called before
> > klp_find_verify_func_addr() in klp_init_object(). This means that
> > func->old_addr is either not verified yet or worse it is still 0. This
> > means that klp_get_func_pos_callback() never returns 1 and is thus called
> > on each symbol. So if you for example patched cmdline_proc_show the
> > resulting directory in sysfs would be called cmdline_proc_show,1 because
> > addr is never matched. Had old_addr been specified the name would have
> > been probably correct, but not for sure.
> >
> > This should be fixed as well.
>
> Even worse, klp_init_func() can be called even if the object hasn't been
> loaded. In that case there's no way to know what the value of n is, and
> therefore no way to reliably create the sysfs entry.
Ah, right.
> Should we create "func,n" in klp_init_object_loaded() instead of
> klp_init_func()?
So that the function entries in sysfs would be created only when the
object is loaded? Well, why not, but in that case it could easily confuse
the user. Object entry would be empty for not loaded object. I would not
dare to propose to remove such object entries. It would make things worse.
So maybe we could introduce an attribute in sysfs object entry which would
say if the object is loaded or not. Or something like that. Hm, we can
easily mess this up :)
Miroslav
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-03 18:00 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qqN6a-5mL-5@gated-at.bofh.it> |
| In reply to | #1261686 |
On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
>
> > On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
> > > On Mon, 2 Nov 2015, Chris J Arges wrote:
> > >
> > > [...]
> > >
> > > > +static int klp_get_func_pos_callback(void *data, const char *name,
> > > > + struct module *mod, unsigned long addr)
> > > > +{
> > > > + struct klp_find_arg *args = data;
> > > > +
> > > > + if ((mod && !args->objname) || (!mod && args->objname))
> > > > + return 0;
> > > > +
> > > > + if (strcmp(args->name, name))
> > > > + return 0;
> > > > +
> > > > + if (args->objname && strcmp(args->objname, mod->name))
> > > > + return 0;
> > > > +
> > > > + /* on address match, return 1 to break kallsyms_on_each_symbol loop */
> > > > + if (args->addr == addr)
> > > > + return 1;
> > > > +
> > > > + /* if we don't match addr, count instance of named symbol */
> > > > + args->count++;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int klp_get_func_pos(struct klp_object *obj, struct klp_func *func)
> > > > +{
> > > > + struct klp_find_arg args = {
> > > > + .objname = obj->name,
> > > > + .name = func->old_name,
> > > > + .addr = func->old_addr,
> > > > + .count = 0,
> > > > + };
> > > > +
> > > > + mutex_lock(&module_mutex);
> > > > + kallsyms_on_each_symbol(klp_get_func_pos_callback, &args);
> > > > + mutex_unlock(&module_mutex);
> > > > +
> > > > + return args.count;
> > > > +}
> > > > +
> > > > static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > > > {
> > > > INIT_LIST_HEAD(&func->stack_node);
> > > > func->state = KLP_DISABLED;
> > > >
> > > > return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> > > > - &obj->kobj, "%s", func->old_name);
> > > > + &obj->kobj, "%s,%d", func->old_name,
> > > > + klp_get_func_pos(obj, func));
> > > > }
> > >
> > > There is a problem which I missed before. klp_init_func() is called before
> > > klp_find_verify_func_addr() in klp_init_object(). This means that
> > > func->old_addr is either not verified yet or worse it is still 0. This
> > > means that klp_get_func_pos_callback() never returns 1 and is thus called
> > > on each symbol. So if you for example patched cmdline_proc_show the
> > > resulting directory in sysfs would be called cmdline_proc_show,1 because
> > > addr is never matched. Had old_addr been specified the name would have
> > > been probably correct, but not for sure.
> > >
> > > This should be fixed as well.
> >
> > Even worse, klp_init_func() can be called even if the object hasn't been
> > loaded. In that case there's no way to know what the value of n is, and
> > therefore no way to reliably create the sysfs entry.
>
> Ah, right.
>
> > Should we create "func,n" in klp_init_object_loaded() instead of
> > klp_init_func()?
>
> So that the function entries in sysfs would be created only when the
> object is loaded? Well, why not, but in that case it could easily confuse
> the user.
Maybe, but I think it would be fine if we document it. It should only
be relied on by tools, anyway.
> Object entry would be empty for not loaded object. I would not
> dare to propose to remove such object entries. It would make things worse.
Why would removing an empty object entry make things worse?
> So maybe we could introduce an attribute in sysfs object entry which would
> say if the object is loaded or not. Or something like that.
Hm, I'm not sure I see how this would help.
> Hm, we can easily mess this up :)
Agreed 100% :-)
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| Date | 2015-11-03 21:50 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qqQGJ-7Hz-1@gated-at.bofh.it> |
| In reply to | #1261720 |
On 11/03/2015 10:50 AM, Josh Poimboeuf wrote:
> On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
>> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
>>
>>> On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
>>>> On Mon, 2 Nov 2015, Chris J Arges wrote:
>>>>
>>>> [...]
>>>>
>>>>> +static int klp_get_func_pos_callback(void *data, const char
>>>>> *name, + struct module *mod, unsigned long addr) +{
>>>>> + struct klp_find_arg *args = data; + + if ((mod &&
>>>>> !args->objname) || (!mod && args->objname)) + return 0; + +
>>>>> if (strcmp(args->name, name)) + return 0; + + if
>>>>> (args->objname && strcmp(args->objname, mod->name)) + return
>>>>> 0; + + /* on address match, return 1 to break
>>>>> kallsyms_on_each_symbol loop */ + if (args->addr == addr) +
>>>>> return 1; + + /* if we don't match addr, count instance of
>>>>> named symbol */ + args->count++; + + return 0; +} + +static
>>>>> int klp_get_func_pos(struct klp_object *obj, struct klp_func
>>>>> *func) +{ + struct klp_find_arg args = { + .objname =
>>>>> obj->name, + .name = func->old_name, + .addr =
>>>>> func->old_addr, + .count = 0, + }; + +
>>>>> mutex_lock(&module_mutex); +
>>>>> kallsyms_on_each_symbol(klp_get_func_pos_callback, &args); +
>>>>> mutex_unlock(&module_mutex); + + return args.count; +} +
>>>>> static int klp_init_func(struct klp_object *obj, struct
>>>>> klp_func *func) { INIT_LIST_HEAD(&func->stack_node);
>>>>> func->state = KLP_DISABLED;
>>>>>
>>>>> return kobject_init_and_add(&func->kobj, &klp_ktype_func, -
>>>>> &obj->kobj, "%s", func->old_name); + &obj->kobj,
>>>>> "%s,%d", func->old_name, + klp_get_func_pos(obj,
>>>>> func)); }
>>>>
>>>> There is a problem which I missed before. klp_init_func() is
>>>> called before klp_find_verify_func_addr() in klp_init_object().
>>>> This means that func->old_addr is either not verified yet or
>>>> worse it is still 0. This means that
>>>> klp_get_func_pos_callback() never returns 1 and is thus called
>>>> on each symbol. So if you for example patched
>>>> cmdline_proc_show the resulting directory in sysfs would be
>>>> called cmdline_proc_show,1 because addr is never matched. Had
>>>> old_addr been specified the name would have been probably
>>>> correct, but not for sure.
>>>>
>>>> This should be fixed as well.
>>>
>>> Even worse, klp_init_func() can be called even if the object
>>> hasn't been loaded. In that case there's no way to know what the
>>> value of n is, and therefore no way to reliably create the sysfs
>>> entry.
>>
>> Ah, right.
>>
>>> Should we create "func,n" in klp_init_object_loaded() instead of
>>> klp_init_func()?
>>
>> So that the function entries in sysfs would be created only when
>> the object is loaded? Well, why not, but in that case it could
>> easily confuse the user.
>
> Maybe, but I think it would be fine if we document it. It should
> only be relied on by tools, anyway.
>
>> Object entry would be empty for not loaded object. I would not dare
>> to propose to remove such object entries. It would make things
>> worse.
>
> Why would removing an empty object entry make things worse?
>
>> So maybe we could introduce an attribute in sysfs object entry
>> which would say if the object is loaded or not. Or something like
>> that.
>
> Hm, I'm not sure I see how this would help.
>
>> Hm, we can easily mess this up :)
>
> Agreed 100% :-)
>
Working on v3 with these suggestions.
- Documentation fixes
- create func,n in klp_init_object_loaded
I'll test unique and non-unique functions being patched. In addition
I'll test this when the object is vmlinux or a module (to test the
object being loaded later).
--chris
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-04 11:00 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qr31h-7c0-15@gated-at.bofh.it> |
| In reply to | #1261720 |
On Tue, 3 Nov 2015, Josh Poimboeuf wrote: > On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote: > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote: > > > > > On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote: > > > > > > > > There is a problem which I missed before. klp_init_func() is called before > > > > klp_find_verify_func_addr() in klp_init_object(). This means that > > > > func->old_addr is either not verified yet or worse it is still 0. This > > > > means that klp_get_func_pos_callback() never returns 1 and is thus called > > > > on each symbol. So if you for example patched cmdline_proc_show the > > > > resulting directory in sysfs would be called cmdline_proc_show,1 because > > > > addr is never matched. Had old_addr been specified the name would have > > > > been probably correct, but not for sure. > > > > > > > > This should be fixed as well. > > > > > > Even worse, klp_init_func() can be called even if the object hasn't been > > > loaded. In that case there's no way to know what the value of n is, and > > > therefore no way to reliably create the sysfs entry. > > > > Ah, right. > > > > > Should we create "func,n" in klp_init_object_loaded() instead of > > > klp_init_func()? > > > > So that the function entries in sysfs would be created only when the > > object is loaded? Well, why not, but in that case it could easily confuse > > the user. > > Maybe, but I think it would be fine if we document it. It should only > be relied on by tools, anyway. Agreed. > > Object entry would be empty for not loaded object. I would not > > dare to propose to remove such object entries. It would make things worse. > > Why would removing an empty object entry make things worse? I think it all comes down to a question whether the sysfs entries say what a patch is capable to patch or what this patch is currently patching in the system. I am inclined to the former so the removal would make me nervous. But I am not against the second approach. We are still in testing mode as far as sysfs is concerned so we can try even harsh changes and see how it's gonna go. > > So maybe we could introduce an attribute in sysfs object entry which would > > say if the object is loaded or not. Or something like that. > > Hm, I'm not sure I see how this would help. Hopefully I cleared this up with the above. Miroslav -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-04 17:10 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qr8Nk-2Ge-11@gated-at.bofh.it> |
| In reply to | #1262166 |
On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote: > On Tue, 3 Nov 2015, Josh Poimboeuf wrote: > > > Object entry would be empty for not loaded object. I would not > > > dare to propose to remove such object entries. It would make things worse. > > > > Why would removing an empty object entry make things worse? > > I think it all comes down to a question whether the sysfs entries say what > a patch is capable to patch or what this patch is currently patching in > the system. I am inclined to the former so the removal would make me > nervous. But I am not against the second approach. We are still in testing > mode as far as sysfs is concerned so we can try even harsh changes and see > how it's gonna go. I see your point. This approach only describes what is patched now, but it doesn't describe what *will* be patched. Ideally we could find a way to describe both. Speaking of harsh changes, here's an idea. What if we require the patch author to supply the value of 'n' instead of supplying the symbol address? We could get rid of 'old_addr' as an input in klp_func and and replace it with 'old_sympos' which has the value of 'n'. Or alternatively we could require old_name to be of the format "func,n". That would uniquely identify each patched function, even _before_ the object is loaded. It would also fix another big problem we have today, where there's no way to disambiguate duplicate symbols in modules, for both function addresses and for relocs. It would simplify the code in other places as well: no special handling for kASLR, no need for klp_verify_vmlinux_symbol() vs klp_find_object_symbol(). A drawback is that it requires the patch author to do a little more due diligence when filling out klp_func. But we already require them to be careful. Thoughts? -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| Date | 2015-11-04 17:20 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qr8WZ-2KK-5@gated-at.bofh.it> |
| In reply to | #1262403 |
On 11/04/2015 10:03 AM, Josh Poimboeuf wrote: > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote: >> On Tue, 3 Nov 2015, Josh Poimboeuf wrote: >>>> Object entry would be empty for not loaded object. I would not >>>> dare to propose to remove such object entries. It would make things worse. >>> >>> Why would removing an empty object entry make things worse? >> >> I think it all comes down to a question whether the sysfs entries say what >> a patch is capable to patch or what this patch is currently patching in >> the system. I am inclined to the former so the removal would make me >> nervous. But I am not against the second approach. We are still in testing >> mode as far as sysfs is concerned so we can try even harsh changes and see >> how it's gonna go. > > I see your point. This approach only describes what is patched now, but > it doesn't describe what *will* be patched. Ideally we could find a way > to describe both. > > Speaking of harsh changes, here's an idea. > > What if we require the patch author to supply the value of 'n' instead > of supplying the symbol address? We could get rid of 'old_addr' as an > input in klp_func and and replace it with 'old_sympos' which has the > value of 'n'. Or alternatively we could require old_name to be of the > format "func,n". I like the idea of old_sympos better than modifying the string. In addition if no old_sympos is specified then it should default to 0, since this will probably be the more common case. > > That would uniquely identify each patched function, even _before_ the > object is loaded. > > It would also fix another big problem we have today, where there's no > way to disambiguate duplicate symbols in modules, for both function > addresses and for relocs. > > It would simplify the code in other places as well: no special handling > for kASLR, no need for klp_verify_vmlinux_symbol() vs > klp_find_object_symbol(). > > A drawback is that it requires the patch author to do a little more due > diligence when filling out klp_func. But we already require them to be > careful. > > Thoughts? > I'll hold off on my v3 for now. Very interesting discussion : ). --chris -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2015-11-05 16:20 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qruuu-8oo-19@gated-at.bofh.it> |
| In reply to | #1262403 |
On Wed, 4 Nov 2015, Josh Poimboeuf wrote:
> On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > > Object entry would be empty for not loaded object. I would not
> > > > dare to propose to remove such object entries. It would make things worse.
> > >
> > > Why would removing an empty object entry make things worse?
> >
> > I think it all comes down to a question whether the sysfs entries say what
> > a patch is capable to patch or what this patch is currently patching in
> > the system. I am inclined to the former so the removal would make me
> > nervous. But I am not against the second approach. We are still in testing
> > mode as far as sysfs is concerned so we can try even harsh changes and see
> > how it's gonna go.
>
> I see your point. This approach only describes what is patched now, but
> it doesn't describe what *will* be patched. Ideally we could find a way
> to describe both.
>
> Speaking of harsh changes, here's an idea.
Which is the very same you proposed last year when I tried to persuade you
to get rid off old_addr and stuff. I called it crazy I remember :D. So
here we are again...
> What if we require the patch author to supply the value of 'n' instead
> of supplying the symbol address? We could get rid of 'old_addr' as an
> input in klp_func and and replace it with 'old_sympos' which has the
> value of 'n'. Or alternatively we could require old_name to be of the
> format "func,n".
>
> That would uniquely identify each patched function, even _before_ the
> object is loaded.
I find it reasonable and we should try it. I think that old_sympos should
have this semantics
0 - default, preserve more or less current behaviour. If the symbol is
unique there is no problem. If it is not the patching would fail.
1, 2, ... - occurrence of the symbol in kallsyms.
The advantage is that if the user does not care and is certain that the
symbol is unique he doesn't have to do anything. If the symbol is not
unique he still has means how to solve it.
Does it make sense?
> It would also fix another big problem we have today, where there's no
> way to disambiguate duplicate symbols in modules, for both function
> addresses and for relocs.
True.
> It would simplify the code in other places as well: no special handling
> for kASLR, no need for klp_verify_vmlinux_symbol() vs
> klp_find_object_symbol().
Which would be great.
> A drawback is that it requires the patch author to do a little more due
> diligence when filling out klp_func. But we already require them to be
> careful.
Yes, I don't think this should be a problem.
> Thoughts?
Yup, we should try it. I suppose that the order of the symbols in kallsyms
table is stable for once-built kernel. It is the order of the symbols in
the object files, isn't it? And since each livepatch module is built
against the specific kernel there should be no issues with this.
Regards,
Miroslav
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-05 17:00 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qrv7d-9F-33@gated-at.bofh.it> |
| In reply to | #1263319 |
On Thu, Nov 05, 2015 at 04:18:12PM +0100, Miroslav Benes wrote: > On Wed, 4 Nov 2015, Josh Poimboeuf wrote: > > > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote: > > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote: > > > > > Object entry would be empty for not loaded object. I would not > > > > > dare to propose to remove such object entries. It would make things worse. > > > > > > > > Why would removing an empty object entry make things worse? > > > > > > I think it all comes down to a question whether the sysfs entries say what > > > a patch is capable to patch or what this patch is currently patching in > > > the system. I am inclined to the former so the removal would make me > > > nervous. But I am not against the second approach. We are still in testing > > > mode as far as sysfs is concerned so we can try even harsh changes and see > > > how it's gonna go. > > > > I see your point. This approach only describes what is patched now, but > > it doesn't describe what *will* be patched. Ideally we could find a way > > to describe both. > > > > Speaking of harsh changes, here's an idea. > > Which is the very same you proposed last year when I tried to persuade you > to get rid off old_addr and stuff. I called it crazy I remember :D. So > here we are again... Ah, I knew I had entertained the idea before, but I forgot we discussed it. It is indeed a little crazy. But this is live patching after all ;-) > > What if we require the patch author to supply the value of 'n' instead > > of supplying the symbol address? We could get rid of 'old_addr' as an > > input in klp_func and and replace it with 'old_sympos' which has the > > value of 'n'. Or alternatively we could require old_name to be of the > > format "func,n". > > > > That would uniquely identify each patched function, even _before_ the > > object is loaded. > > I find it reasonable and we should try it. I think that old_sympos should > have this semantics > > 0 - default, preserve more or less current behaviour. If the symbol is > unique there is no problem. If it is not the patching would fail. > 1, 2, ... - occurrence of the symbol in kallsyms. > > The advantage is that if the user does not care and is certain that the > symbol is unique he doesn't have to do anything. If the symbol is not > unique he still has means how to solve it. > > Does it make sense? Sounds good! > > It would also fix another big problem we have today, where there's no > > way to disambiguate duplicate symbols in modules, for both function > > addresses and for relocs. > > True. > > > It would simplify the code in other places as well: no special handling > > for kASLR, no need for klp_verify_vmlinux_symbol() vs > > klp_find_object_symbol(). > > Which would be great. > > > A drawback is that it requires the patch author to do a little more due > > diligence when filling out klp_func. But we already require them to be > > careful. > > Yes, I don't think this should be a problem. > > > Thoughts? > > Yup, we should try it. I suppose that the order of the symbols in kallsyms > table is stable for once-built kernel. It is the order of the symbols in > the object files, isn't it? And since each livepatch module is built > against the specific kernel there should be no issues with this. The order of the symbols in an object's symbol table does appear to be the same as the order in kallsyms (per-object). So yeah, let's try it. -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| Date | 2015-11-05 17:10 +0100 |
| Subject | Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory |
| Message-ID | <qrvgS-tj-23@gated-at.bofh.it> |
| In reply to | #1263350 |
On Thu, Nov 05, 2015 at 09:56:56AM -0600, Josh Poimboeuf wrote: > On Thu, Nov 05, 2015 at 04:18:12PM +0100, Miroslav Benes wrote: > > On Wed, 4 Nov 2015, Josh Poimboeuf wrote: > > > > > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote: > > > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote: > > > > > > Object entry would be empty for not loaded object. I would not > > > > > > dare to propose to remove such object entries. It would make things worse. > > > > > > > > > > Why would removing an empty object entry make things worse? > > > > > > > > I think it all comes down to a question whether the sysfs entries say what > > > > a patch is capable to patch or what this patch is currently patching in > > > > the system. I am inclined to the former so the removal would make me > > > > nervous. But I am not against the second approach. We are still in testing > > > > mode as far as sysfs is concerned so we can try even harsh changes and see > > > > how it's gonna go. > > > > > > I see your point. This approach only describes what is patched now, but > > > it doesn't describe what *will* be patched. Ideally we could find a way > > > to describe both. > > > > > > Speaking of harsh changes, here's an idea. > > > > Which is the very same you proposed last year when I tried to persuade you > > to get rid off old_addr and stuff. I called it crazy I remember :D. So > > here we are again... > > Ah, I knew I had entertained the idea before, but I forgot we discussed > it. It is indeed a little crazy. But this is live patching after all > ;-) > > > > What if we require the patch author to supply the value of 'n' instead > > > of supplying the symbol address? We could get rid of 'old_addr' as an > > > input in klp_func and and replace it with 'old_sympos' which has the > > > value of 'n'. Or alternatively we could require old_name to be of the > > > format "func,n". > > > > > > That would uniquely identify each patched function, even _before_ the > > > object is loaded. > > > > I find it reasonable and we should try it. I think that old_sympos should > > have this semantics > > > > 0 - default, preserve more or less current behaviour. If the symbol is > > unique there is no problem. If it is not the patching would fail. > > 1, 2, ... - occurrence of the symbol in kallsyms. > > > > The advantage is that if the user does not care and is certain that the > > symbol is unique he doesn't have to do anything. If the symbol is not > > unique he still has means how to solve it. > > > > Does it make sense? > > Sounds good! > > > > It would also fix another big problem we have today, where there's no > > > way to disambiguate duplicate symbols in modules, for both function > > > addresses and for relocs. > > > > True. > > > > > It would simplify the code in other places as well: no special handling > > > for kASLR, no need for klp_verify_vmlinux_symbol() vs > > > klp_find_object_symbol(). > > > > Which would be great. > > > > > A drawback is that it requires the patch author to do a little more due > > > diligence when filling out klp_func. But we already require them to be > > > careful. > > > > Yes, I don't think this should be a problem. > > > > > Thoughts? > > > > Yup, we should try it. I suppose that the order of the symbols in kallsyms > > table is stable for once-built kernel. It is the order of the symbols in > > the object files, isn't it? And since each livepatch module is built > > against the specific kernel there should be no issues with this. > > The order of the symbols in an object's symbol table does appear to be > the same as the order in kallsyms (per-object). So yeah, let's try it. > > -- > Josh > Great! Using this feedback to create the next patch. I'll post something in the next few days. --chris -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| Date | 2015-11-09 17:20 +0100 |
| Subject | [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory |
| Message-ID | <qsXkK-i7-13@gated-at.bofh.it> |
| In reply to | #1263350 |
In cases of duplicate symbols in vmlinux, old_sympos will be used to
disambiguate instead of old_addr. Normally old_sympos will be 0, and
default to only returning the first found instance of that symbol. If an
incorrect symbol position is specified then livepatching will fail.
Finally, old_addr is now an internal structure element and not to be
specified by the user.
The following directory structure will allow for cases when the same
function name exists in a single object.
/sys/kernel/livepatch/<patch>/<object>/<function.number>
The number corresponds to the nth occurrence of the symbol name in
kallsyms for the patched object.
An example of patching multiple symbols can be found here:
https://github.com/dynup/kpatch/issues/493
Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
Documentation/ABI/testing/sysfs-kernel-livepatch | 6 ++-
include/linux/livepatch.h | 20 ++++---
kernel/livepatch/core.c | 66 ++++++++++++++++--------
3 files changed, 61 insertions(+), 31 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 5bf42a8..21b6bc1 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,7 +33,7 @@ Description:
The object directory contains subdirectories for each function
that is patched within the object.
-What: /sys/kernel/livepatch/<patch>/<object>/<function>
+What: /sys/kernel/livepatch/<patch>/<object>/<function,number>
Date: Nov 2014
KernelVersion: 3.19.0
Contact: live-patching@vger.kernel.org
@@ -41,4 +41,8 @@ Description:
The function directory contains attributes regarding the
properties and state of the patched function.
+ The directory name contains the patched function name and a
+ number corresponding to the nth occurrence of the symbol name
+ in kallsyms for the patched object.
+
There are currently no such attributes.
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..986e06d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,8 +37,9 @@ enum klp_state {
* struct klp_func - function structure for live patching
* @old_name: name of the function to be patched
* @new_func: pointer to the patched function code
- * @old_addr: a hint conveying at what address the old function
+ * @old_sympos: a hint indicating which symbol position the old function
* can be found (optional, vmlinux patches only)
+ * @old_addr: the address of the function being patched
* @kobj: kobject for sysfs resources
* @state: tracks function-level patch application state
* @stack_node: list node for klp_ops func_stack list
@@ -47,17 +48,20 @@ struct klp_func {
/* external */
const char *old_name;
void *new_func;
+
/*
- * The old_addr field is optional and can be used to resolve
- * duplicate symbol names in the vmlinux object. If this
- * information is not present, the symbol is located by name
- * with kallsyms. If the name is not unique and old_addr is
- * not provided, the patch application fails as there is no
- * way to resolve the ambiguity.
+ * The old_sympos field is optional and can be used to resolve duplicate
+ * symbol names in the vmlinux object. If this information is not
+ * present, the first symbol located with kallsyms is used. This value
+ * corresponds to the nth occurrence of the symbol name in kallsyms for
+ * the patched object. If the name is not unique and old_sympos is not
+ * provided, the patch application fails as there is no way to resolve
+ * the ambiguity.
*/
- unsigned long old_addr;
+ unsigned long old_sympos;
/* internal */
+ unsigned long old_addr;
struct kobject kobj;
enum klp_state state;
struct list_head stack_node;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..1dd0d44 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -142,6 +142,7 @@ struct klp_find_arg {
* name in the same object.
*/
unsigned long count;
+ unsigned long pos;
};
static int klp_find_callback(void *data, const char *name,
@@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
args->addr = addr;
args->count++;
+ /*
+ * ensure count matches the symbol position
+ */
+ if (args->pos == (args->count-1))
+ return 1;
+
return 0;
}
static int klp_find_object_symbol(const char *objname, const char *name,
- unsigned long *addr)
+ unsigned long *addr, unsigned long sympos)
{
struct klp_find_arg args = {
.objname = objname,
.name = name,
.addr = 0,
- .count = 0
+ .count = 0,
+ .pos = sympos,
};
mutex_lock(&module_mutex);
kallsyms_on_each_symbol(klp_find_callback, &args);
mutex_unlock(&module_mutex);
- if (args.count == 0)
+ /*
+ * Ensure an address was found, then check that the symbol position
+ * count matches sympos.
+ */
+ if (args.addr == 0)
pr_err("symbol '%s' not found in symbol table\n", name);
- else if (args.count > 1)
- pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
- args.count, name, objname);
+ else if (sympos != (args.count - 1))
+ pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
+ sympos, name, objname ? objname : "vmlinux");
else {
*addr = args.addr;
return 0;
@@ -239,20 +251,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
static int klp_find_verify_func_addr(struct klp_object *obj,
struct klp_func *func)
{
+ int sympos = 0;
int ret;
-#if defined(CONFIG_RANDOMIZE_BASE)
- /* If KASLR has been enabled, adjust old_addr accordingly */
- if (kaslr_enabled() && func->old_addr)
- func->old_addr += kaslr_offset();
-#endif
+ if (func->old_sympos && !klp_is_module(obj))
+ sympos = func->old_sympos;
- if (!func->old_addr || klp_is_module(obj))
- ret = klp_find_object_symbol(obj->name, func->old_name,
- &func->old_addr);
- else
- ret = klp_verify_vmlinux_symbol(func->old_name,
- func->old_addr);
+ /*
+ * Verify the symbol, find old_addr, and write it to the structure.
+ * By default sympos will be 0 and thus will only look for the first
+ * occurrence. If another value is specified then that will be used.
+ */
+ ret = klp_find_object_symbol(obj->name, func->old_name,
+ &func->old_addr, sympos);
return ret;
}
@@ -277,7 +288,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);
+ return klp_find_object_symbol(pmod->name, name, addr, 0);
}
static int klp_write_object_relocations(struct module *pmod,
@@ -307,7 +318,7 @@ static int klp_write_object_relocations(struct module *pmod,
else
ret = klp_find_object_symbol(obj->mod->name,
reloc->name,
- &reloc->val);
+ &reloc->val, 0);
if (ret)
return ret;
}
@@ -587,7 +598,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
* /sys/kernel/livepatch/<patch>
* /sys/kernel/livepatch/<patch>/enabled
* /sys/kernel/livepatch/<patch>/<object>
- * /sys/kernel/livepatch/<patch>/<object>/<func>
+ * /sys/kernel/livepatch/<patch>/<object>/<func,number>
*/
static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -732,8 +743,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
INIT_LIST_HEAD(&func->stack_node);
func->state = KLP_DISABLED;
- return kobject_init_and_add(&func->kobj, &klp_ktype_func,
- &obj->kobj, "%s", func->old_name);
+ return 0;
}
/* parts of the initialization that is done only when the object is loaded */
@@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
return ret;
}
+ /*
+ * for each function initialize and add, old_sympos will be already
+ * verified at this point
+ */
+ klp_for_each_func(obj, func) {
+ ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
+ &obj->kobj, "%s,%lu", func->old_name,
+ func->old_sympos ? func->old_sympos : 0);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
--
1.9.1
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-09 22:00 +0100 |
| Subject | Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory |
| Message-ID | <qt1HJ-2YW-19@gated-at.bofh.it> |
| In reply to | #1265788 |
I'd recommend splitting this up into two separate patches:
1. introduce old_sympos
2. change the sysfs interface
On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> default to only returning the first found instance of that symbol. If an
> incorrect symbol position is specified then livepatching will fail.
In the case of old_sympos == 0, instead of just returning the first
symbol it finds, I think it should ensure that the symbol is unique. As
Miroslav suggested:
0 - default, preserve more or less current behaviour. If the symbol is
unique there is no problem. If it is not the patching would fail.
1, 2, ... - occurrence of the symbol in kallsyms.
The advantage is that if the user does not care and is certain that the
symbol is unique he doesn't have to do anything. If the symbol is not
unique he still has means how to solve it.
> Finally, old_addr is now an internal structure element and not to be
> specified by the user.
>
> The following directory structure will allow for cases when the same
> function name exists in a single object.
> /sys/kernel/livepatch/<patch>/<object>/<function.number>
Period should be changed to a comma.
> The number corresponds to the nth occurrence of the symbol name in
> kallsyms for the patched object.
>
> An example of patching multiple symbols can be found here:
> https://github.com/dynup/kpatch/issues/493
>
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
> Documentation/ABI/testing/sysfs-kernel-livepatch | 6 ++-
> include/linux/livepatch.h | 20 ++++---
> kernel/livepatch/core.c | 66 ++++++++++++++++--------
> 3 files changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index 5bf42a8..21b6bc1 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -33,7 +33,7 @@ Description:
> The object directory contains subdirectories for each function
> that is patched within the object.
>
> -What: /sys/kernel/livepatch/<patch>/<object>/<function>
> +What: /sys/kernel/livepatch/<patch>/<object>/<function,number>
> Date: Nov 2014
> KernelVersion: 3.19.0
> Contact: live-patching@vger.kernel.org
> @@ -41,4 +41,8 @@ Description:
> The function directory contains attributes regarding the
> properties and state of the patched function.
>
> + The directory name contains the patched function name and a
> + number corresponding to the nth occurrence of the symbol name
> + in kallsyms for the patched object.
> +
> There are currently no such attributes.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..986e06d 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -37,8 +37,9 @@ enum klp_state {
> * struct klp_func - function structure for live patching
> * @old_name: name of the function to be patched
> * @new_func: pointer to the patched function code
> - * @old_addr: a hint conveying at what address the old function
> + * @old_sympos: a hint indicating which symbol position the old function
> * can be found (optional, vmlinux patches only)
Why is old_sympos only checked for vmlinux patches only? It's a
per-object count, so it should work for modules as well.
> + * @old_addr: the address of the function being patched
> * @kobj: kobject for sysfs resources
> * @state: tracks function-level patch application state
> * @stack_node: list node for klp_ops func_stack list
> @@ -47,17 +48,20 @@ struct klp_func {
> /* external */
> const char *old_name;
> void *new_func;
> +
> /*
> - * The old_addr field is optional and can be used to resolve
> - * duplicate symbol names in the vmlinux object. If this
> - * information is not present, the symbol is located by name
> - * with kallsyms. If the name is not unique and old_addr is
> - * not provided, the patch application fails as there is no
> - * way to resolve the ambiguity.
> + * The old_sympos field is optional and can be used to resolve duplicate
> + * symbol names in the vmlinux object. If this information is not
> + * present, the first symbol located with kallsyms is used. This value
> + * corresponds to the nth occurrence of the symbol name in kallsyms for
> + * the patched object. If the name is not unique and old_sympos is not
> + * provided, the patch application fails as there is no way to resolve
> + * the ambiguity.
> */
> - unsigned long old_addr;
> + unsigned long old_sympos;
>
> /* internal */
> + unsigned long old_addr;
> struct kobject kobj;
> enum klp_state state;
> struct list_head stack_node;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..1dd0d44 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,7 @@ struct klp_find_arg {
> * name in the same object.
> */
> unsigned long count;
> + unsigned long pos;
> };
>
> static int klp_find_callback(void *data, const char *name,
> @@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
> args->addr = addr;
> args->count++;
>
> + /*
> + * ensure count matches the symbol position
> + */
> + if (args->pos == (args->count-1))
> + return 1;
> +
The code can be simplified a bit if args->addr only gets set when
args->pos is a match. Then klp_find_object_symbol() only needs to check
args.addr to see if a match was found.
> return 0;
> }
>
> static int klp_find_object_symbol(const char *objname, const char *name,
> - unsigned long *addr)
> + unsigned long *addr, unsigned long sympos)
> {
> struct klp_find_arg args = {
> .objname = objname,
> .name = name,
> .addr = 0,
> - .count = 0
> + .count = 0,
> + .pos = sympos,
> };
>
> mutex_lock(&module_mutex);
> kallsyms_on_each_symbol(klp_find_callback, &args);
> mutex_unlock(&module_mutex);
>
> - if (args.count == 0)
> + /*
> + * Ensure an address was found, then check that the symbol position
> + * count matches sympos.
> + */
> + if (args.addr == 0)
> pr_err("symbol '%s' not found in symbol table\n", name);
> - else if (args.count > 1)
> - pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
> - args.count, name, objname);
> + else if (sympos != (args.count - 1))
> + pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
> + sympos, name, objname ? objname : "vmlinux");
> else {
> *addr = args.addr;
> return 0;
> @@ -239,20 +251,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
> static int klp_find_verify_func_addr(struct klp_object *obj,
> struct klp_func *func)
> {
> + int sympos = 0;
> int ret;
>
> -#if defined(CONFIG_RANDOMIZE_BASE)
> - /* If KASLR has been enabled, adjust old_addr accordingly */
> - if (kaslr_enabled() && func->old_addr)
> - func->old_addr += kaslr_offset();
> -#endif
> + if (func->old_sympos && !klp_is_module(obj))
> + sympos = func->old_sympos;
>
> - if (!func->old_addr || klp_is_module(obj))
> - ret = klp_find_object_symbol(obj->name, func->old_name,
> - &func->old_addr);
> - else
> - ret = klp_verify_vmlinux_symbol(func->old_name,
> - func->old_addr);
> + /*
> + * Verify the symbol, find old_addr, and write it to the structure.
> + * By default sympos will be 0 and thus will only look for the first
> + * occurrence. If another value is specified then that will be used.
> + */
> + ret = klp_find_object_symbol(obj->name, func->old_name,
> + &func->old_addr, sympos);
>
> return ret;
> }
> @@ -277,7 +288,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);
> + return klp_find_object_symbol(pmod->name, name, addr, 0);
> }
>
> static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +318,7 @@ static int klp_write_object_relocations(struct module *pmod,
> else
> ret = klp_find_object_symbol(obj->mod->name,
> reloc->name,
> - &reloc->val);
> + &reloc->val, 0);
I think it would be a good idea to also add old_sympos to klp_reloc so
the relocation code is consistent with the klp_func symbol addressing.
> if (ret)
> return ret;
> }
> @@ -587,7 +598,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
> * /sys/kernel/livepatch/<patch>
> * /sys/kernel/livepatch/<patch>/enabled
> * /sys/kernel/livepatch/<patch>/<object>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>
> + * /sys/kernel/livepatch/<patch>/<object>/<func,number>
> */
>
> static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -732,8 +743,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> INIT_LIST_HEAD(&func->stack_node);
> func->state = KLP_DISABLED;
>
> - return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> - &obj->kobj, "%s", func->old_name);
> + return 0;
> }
The sysfs entry can still be created here, since the function name and
sympos are both already known.
>
> /* parts of the initialization that is done only when the object is loaded */
> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> return ret;
> }
>
> + /*
> + * for each function initialize and add, old_sympos will be already
> + * verified at this point
> + */
> + klp_for_each_func(obj, func) {
> + ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> + &obj->kobj, "%s,%lu", func->old_name,
> + func->old_sympos ? func->old_sympos : 0);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
>
> --
> 1.9.1
>
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Chris J Arges <chris.j.arges@canonical.com> |
|---|---|
| Date | 2015-11-10 00:10 +0100 |
| Subject | Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory |
| Message-ID | <qt3Jw-5gt-17@gated-at.bofh.it> |
| In reply to | #1266002 |
On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> I'd recommend splitting this up into two separate patches:
>
> 1. introduce old_sympos
> 2. change the sysfs interface
>
> On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
>> In cases of duplicate symbols in vmlinux, old_sympos will be used to
>> disambiguate instead of old_addr. Normally old_sympos will be 0, and
>> default to only returning the first found instance of that symbol. If an
>> incorrect symbol position is specified then livepatching will fail.
>
> In the case of old_sympos == 0, instead of just returning the first
> symbol it finds, I think it should ensure that the symbol is unique. As
> Miroslav suggested:
>
> 0 - default, preserve more or less current behaviour. If the symbol is
> unique there is no problem. If it is not the patching would fail.
> 1, 2, ... - occurrence of the symbol in kallsyms.
>
> The advantage is that if the user does not care and is certain that the
> symbol is unique he doesn't have to do anything. If the symbol is not
> unique he still has means how to solve it.
>
So one part that will be confusing here is as follows.
If '0' is specified for old_sympos, should the symbol be 'func_name,0'
or 'func_name,1' provided we have a unique symbol? We could also default
to 'what the user provides', but this seems odd.
Another option would be to use no postfix when 0 is given, and only
introduce the ',n' postfix if old_sympos is > 0.
--chris
>> Finally, old_addr is now an internal structure element and not to be
>> specified by the user.
>>
>> The following directory structure will allow for cases when the same
>> function name exists in a single object.
>> /sys/kernel/livepatch/<patch>/<object>/<function.number>
>
> Period should be changed to a comma.
>
>> The number corresponds to the nth occurrence of the symbol name in
>> kallsyms for the patched object.
>>
>> An example of patching multiple symbols can be found here:
>> https://github.com/dynup/kpatch/issues/493
>>
>> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
>> ---
>> Documentation/ABI/testing/sysfs-kernel-livepatch | 6 ++-
>> include/linux/livepatch.h | 20 ++++---
>> kernel/livepatch/core.c | 66 ++++++++++++++++--------
>> 3 files changed, 61 insertions(+), 31 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
>> index 5bf42a8..21b6bc1 100644
>> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
>> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
>> @@ -33,7 +33,7 @@ Description:
>> The object directory contains subdirectories for each function
>> that is patched within the object.
>>
>> -What: /sys/kernel/livepatch/<patch>/<object>/<function>
>> +What: /sys/kernel/livepatch/<patch>/<object>/<function,number>
>> Date: Nov 2014
>> KernelVersion: 3.19.0
>> Contact: live-patching@vger.kernel.org
>> @@ -41,4 +41,8 @@ Description:
>> The function directory contains attributes regarding the
>> properties and state of the patched function.
>>
>> + The directory name contains the patched function name and a
>> + number corresponding to the nth occurrence of the symbol name
>> + in kallsyms for the patched object.
>> +
>> There are currently no such attributes.
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..986e06d 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -37,8 +37,9 @@ enum klp_state {
>> * struct klp_func - function structure for live patching
>> * @old_name: name of the function to be patched
>> * @new_func: pointer to the patched function code
>> - * @old_addr: a hint conveying at what address the old function
>> + * @old_sympos: a hint indicating which symbol position the old function
>> * can be found (optional, vmlinux patches only)
>
> Why is old_sympos only checked for vmlinux patches only? It's a
> per-object count, so it should work for modules as well.
>
>> + * @old_addr: the address of the function being patched
>> * @kobj: kobject for sysfs resources
>> * @state: tracks function-level patch application state
>> * @stack_node: list node for klp_ops func_stack list
>> @@ -47,17 +48,20 @@ struct klp_func {
>> /* external */
>> const char *old_name;
>> void *new_func;
>> +
>> /*
>> - * The old_addr field is optional and can be used to resolve
>> - * duplicate symbol names in the vmlinux object. If this
>> - * information is not present, the symbol is located by name
>> - * with kallsyms. If the name is not unique and old_addr is
>> - * not provided, the patch application fails as there is no
>> - * way to resolve the ambiguity.
>> + * The old_sympos field is optional and can be used to resolve duplicate
>> + * symbol names in the vmlinux object. If this information is not
>> + * present, the first symbol located with kallsyms is used. This value
>> + * corresponds to the nth occurrence of the symbol name in kallsyms for
>> + * the patched object. If the name is not unique and old_sympos is not
>> + * provided, the patch application fails as there is no way to resolve
>> + * the ambiguity.
>> */
>> - unsigned long old_addr;
>> + unsigned long old_sympos;
>>
>> /* internal */
>> + unsigned long old_addr;
>> struct kobject kobj;
>> enum klp_state state;
>> struct list_head stack_node;
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 6e53441..1dd0d44 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -142,6 +142,7 @@ struct klp_find_arg {
>> * name in the same object.
>> */
>> unsigned long count;
>> + unsigned long pos;
>> };
>>
>> static int klp_find_callback(void *data, const char *name,
>> @@ -166,28 +167,39 @@ static int klp_find_callback(void *data, const char *name,
>> args->addr = addr;
>> args->count++;
>>
>> + /*
>> + * ensure count matches the symbol position
>> + */
>> + if (args->pos == (args->count-1))
>> + return 1;
>> +
>
> The code can be simplified a bit if args->addr only gets set when
> args->pos is a match. Then klp_find_object_symbol() only needs to check
> args.addr to see if a match was found.
>
>> return 0;
>> }
>>
>> static int klp_find_object_symbol(const char *objname, const char *name,
>> - unsigned long *addr)
>> + unsigned long *addr, unsigned long sympos)
>> {
>> struct klp_find_arg args = {
>> .objname = objname,
>> .name = name,
>> .addr = 0,
>> - .count = 0
>> + .count = 0,
>> + .pos = sympos,
>> };
>>
>> mutex_lock(&module_mutex);
>> kallsyms_on_each_symbol(klp_find_callback, &args);
>> mutex_unlock(&module_mutex);
>>
>> - if (args.count == 0)
>> + /*
>> + * Ensure an address was found, then check that the symbol position
>> + * count matches sympos.
>> + */
>> + if (args.addr == 0)
>> pr_err("symbol '%s' not found in symbol table\n", name);
>> - else if (args.count > 1)
>> - pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
>> - args.count, name, objname);
>> + else if (sympos != (args.count - 1))
>> + pr_err("symbol position %lu for symbol '%s' in object '%s' not found\n",
>> + sympos, name, objname ? objname : "vmlinux");
>> else {
>> *addr = args.addr;
>> return 0;
>> @@ -239,20 +251,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
>> static int klp_find_verify_func_addr(struct klp_object *obj,
>> struct klp_func *func)
>> {
>> + int sympos = 0;
>> int ret;
>>
>> -#if defined(CONFIG_RANDOMIZE_BASE)
>> - /* If KASLR has been enabled, adjust old_addr accordingly */
>> - if (kaslr_enabled() && func->old_addr)
>> - func->old_addr += kaslr_offset();
>> -#endif
>> + if (func->old_sympos && !klp_is_module(obj))
>> + sympos = func->old_sympos;
>>
>> - if (!func->old_addr || klp_is_module(obj))
>> - ret = klp_find_object_symbol(obj->name, func->old_name,
>> - &func->old_addr);
>> - else
>> - ret = klp_verify_vmlinux_symbol(func->old_name,
>> - func->old_addr);
>> + /*
>> + * Verify the symbol, find old_addr, and write it to the structure.
>> + * By default sympos will be 0 and thus will only look for the first
>> + * occurrence. If another value is specified then that will be used.
>> + */
>> + ret = klp_find_object_symbol(obj->name, func->old_name,
>> + &func->old_addr, sympos);
>>
>> return ret;
>> }
>> @@ -277,7 +288,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);
>> + return klp_find_object_symbol(pmod->name, name, addr, 0);
>> }
>>
>> static int klp_write_object_relocations(struct module *pmod,
>> @@ -307,7 +318,7 @@ static int klp_write_object_relocations(struct module *pmod,
>> else
>> ret = klp_find_object_symbol(obj->mod->name,
>> reloc->name,
>> - &reloc->val);
>> + &reloc->val, 0);
>
> I think it would be a good idea to also add old_sympos to klp_reloc so
> the relocation code is consistent with the klp_func symbol addressing.
>
So you are thinking as an optional external field as well? I'll have to
look at this a bit more but makes sense to me.
--chris
>> if (ret)
>> return ret;
>> }
>> @@ -587,7 +598,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>> * /sys/kernel/livepatch/<patch>
>> * /sys/kernel/livepatch/<patch>/enabled
>> * /sys/kernel/livepatch/<patch>/<object>
>> - * /sys/kernel/livepatch/<patch>/<object>/<func>
>> + * /sys/kernel/livepatch/<patch>/<object>/<func,number>
>> */
>>
>> static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>> @@ -732,8 +743,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>> INIT_LIST_HEAD(&func->stack_node);
>> func->state = KLP_DISABLED;
>>
>> - return kobject_init_and_add(&func->kobj, &klp_ktype_func,
>> - &obj->kobj, "%s", func->old_name);
>> + return 0;
>> }
>
> The sysfs entry can still be created here, since the function name and
> sympos are both already known.
>
>>
>> /* parts of the initialization that is done only when the object is loaded */
>> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>> return ret;
>> }
>>
>> + /*
>> + * for each function initialize and add, old_sympos will be already
>> + * verified at this point
>> + */
>> + klp_for_each_func(obj, func) {
>> + ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
>> + &obj->kobj, "%s,%lu", func->old_name,
>> + func->old_sympos ? func->old_sympos : 0);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> return 0;
>> }
>>
>> --
>> 1.9.1
>>
>
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-11-10 06:00 +0100 |
| Subject | Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory |
| Message-ID | <qt9cd-nP-7@gated-at.bofh.it> |
| In reply to | #1266071 |
On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote: > On 11/09/2015 02:56 PM, Josh Poimboeuf wrote: > > I'd recommend splitting this up into two separate patches: > > > > 1. introduce old_sympos > > 2. change the sysfs interface > > > > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote: > >> In cases of duplicate symbols in vmlinux, old_sympos will be used to > >> disambiguate instead of old_addr. Normally old_sympos will be 0, and > >> default to only returning the first found instance of that symbol. If an > >> incorrect symbol position is specified then livepatching will fail. > > > > In the case of old_sympos == 0, instead of just returning the first > > symbol it finds, I think it should ensure that the symbol is unique. As > > Miroslav suggested: > > > > 0 - default, preserve more or less current behaviour. If the symbol is > > unique there is no problem. If it is not the patching would fail. > > 1, 2, ... - occurrence of the symbol in kallsyms. > > > > The advantage is that if the user does not care and is certain that the > > symbol is unique he doesn't have to do anything. If the symbol is not > > unique he still has means how to solve it. > > > > So one part that will be confusing here is as follows. > > If '0' is specified for old_sympos, should the symbol be 'func_name,0' > or 'func_name,1' provided we have a unique symbol? We could also default > to 'what the user provides', but this seems odd. I don't feel strongly either way, but I think using the same number the user provides is fine, since it makes the sysfs interface consistent with the old_sympos usage. > Another option would be to use no postfix when 0 is given, and only > introduce the ',n' postfix if old_sympos is > 0. IMO always having a suffix is good, as it makes parsing less surprising and less error-prone. > >> static int klp_write_object_relocations(struct module *pmod, > >> @@ -307,7 +318,7 @@ static int klp_write_object_relocations(struct module *pmod, > >> else > >> ret = klp_find_object_symbol(obj->mod->name, > >> reloc->name, > >> - &reloc->val); > >> + &reloc->val, 0); > > > > I think it would be a good idea to also add old_sympos to klp_reloc so > > the relocation code is consistent with the klp_func symbol addressing. > > > > So you are thinking as an optional external field as well? I'll have to > look at this a bit more but makes sense to me. Yeah, the semantics would be the same as klp_func.old_sympos. We could add a new klp_reloc.sympos and make klp_reloc.val a private field. -- 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/
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web