In cases of duplicate symbols, sympos will be used to disambiguate instead
of val. By default 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 in kallsyms for the patched object will be used
for patching if it is valid.
Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
as they are no longer used.
>From the klp_reloc structure remove val, as it can be refactored as a
local variable in klp_write_object_relocations.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 5 ++--
kernel/livepatch/core.c | 75 +++++++++++------------------------------------
2 files changed, 19 insertions(+), 61 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index b60e8ab..a882865 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -67,8 +67,7 @@ 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)
+ * @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
@@ -76,7 +75,7 @@ struct klp_func {
*/
struct klp_reloc {
unsigned long loc;
- unsigned long val;
+ unsigned long sympos;
unsigned long type;
const char *name;
int addend;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5a53383..9d98c7b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -203,45 +203,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;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -272,6 +233,7 @@ static int klp_write_object_relocations(struct module *pmod,
struct klp_object *obj)
{
int ret;
+ unsigned long val;
struct klp_reloc *reloc;
if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -281,29 +243,26 @@ 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,
- 0, &reloc->val);
- if (ret)
- return ret;
+ if (reloc->sympos && (reloc->sympos != 1)) {
+ pr_err("symbol '%s' is external and has sympos %lu\n",
+ reloc->name, reloc->sympos);
+ return -EINVAL;
}
+ /* discover the address of the referenced symbol */
+ if (reloc->external) {
+ ret = klp_find_external_symbol(pmod, reloc->name, &val);
+ } else
+ ret = klp_find_object_symbol(obj->mod->name,
+ reloc->name,
+ reloc->sympos,
+ &val);
+ if (ret)
+ return ret;
ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
- reloc->val + reloc->addend);
+ val + reloc->addend);
if (ret) {
pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
- reloc->name, reloc->val, ret);
+ reloc->name, val, ret);
return ret;
}
}
--
1.9.1
On Mon, 16 Nov 2015, Chris J Arges wrote:
> @@ -281,29 +243,26 @@ 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,
> - 0, &reloc->val);
> - if (ret)
> - return ret;
> + if (reloc->sympos && (reloc->sympos != 1)) {
> + pr_err("symbol '%s' is external and has sympos %lu\n",
> + reloc->name, reloc->sympos);
> + return -EINVAL;
> }
Oh, this check should not be here, right? I think it needs to be under 'if
(reloc->external) {'.
And
if (reloc->sympos && (reloc->sympos != 1)) {
can be written as
if (reloc->sympos > 1) {
Anyway, I am not sure if this is the right thing to do. I know Petr
suggested it, but maybe it would be sufficient just to error out when
reloc->sympos is specified for external symbols. Despite there is a valid
value (== 1). Like it had been done before. Josh came up with the issue.
So what is your opinion, Josh? It is a nitpick, but nevertheless.
Miroslav
> + /* discover the address of the referenced symbol */
> + if (reloc->external) {
> + ret = klp_find_external_symbol(pmod, reloc->name, &val);
> + } else
> + ret = klp_find_object_symbol(obj->mod->name,
> + reloc->name,
> + reloc->sympos,
> + &val);
> + if (ret)
> + return ret;
> ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> - reloc->val + reloc->addend);
> + val + reloc->addend);
> if (ret) {
> pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> - reloc->name, reloc->val, ret);
> + reloc->name, val, ret);
> return ret;
> }
> }
>
On Wed, Nov 18, 2015 at 10:56:00AM +0100, Miroslav Benes wrote:
> On Mon, 16 Nov 2015, Chris J Arges wrote:
>
> > @@ -281,29 +243,26 @@ 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,
> > - 0, &reloc->val);
> > - if (ret)
> > - return ret;
> > + if (reloc->sympos && (reloc->sympos != 1)) {
> > + pr_err("symbol '%s' is external and has sympos %lu\n",
> > + reloc->name, reloc->sympos);
> > + return -EINVAL;
> > }
>
> Oh, this check should not be here, right? I think it needs to be under 'if
> (reloc->external) {'.
>
> And
>
> if (reloc->sympos && (reloc->sympos != 1)) {
>
> can be written as
>
> if (reloc->sympos > 1) {
>
> Anyway, I am not sure if this is the right thing to do. I know Petr
> suggested it, but maybe it would be sufficient just to error out when
> reloc->sympos is specified for external symbols. Despite there is a valid
> value (== 1). Like it had been done before. Josh came up with the issue.
> So what is your opinion, Josh? It is a nitpick, but nevertheless.
I tend to agree. IMO, any non-zero value of sympos doesn't make sense
for external symbols.
--
Josh
On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
> In cases of duplicate symbols, sympos will be used to disambiguate instead
> of val. By default 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 in kallsyms for the patched object will be used
> for patching if it is valid.
>
> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> as they are no longer used.
>
> >From the klp_reloc structure remove val, as it can be refactored as a
^
Please, remove this extra '>'. I guess that you cut&pasted the
comment from a mail reply ;-)
Best Regards,
Petr
On 11/18/2015 10:37 AM, Petr Mladek wrote:
> On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
>> In cases of duplicate symbols, sympos will be used to disambiguate instead
>> of val. By default 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 in kallsyms for the patched object will be used
>> for patching if it is valid.
>>
>> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
>> as they are no longer used.
>>
>> >From the klp_reloc structure remove val, as it can be refactored as a
> ^
> Please, remove this extra '>'. I guess that you cut&pasted the
> comment from a mail reply ;-)
>
> Best Regards,
> Petr
>
I think that '>' was added in a subsequent reply. My patch doesn't have
that, and I'm guessing checkpatch would have warned me about that as well.
: )
--chris
On Wed 2015-11-18 08:01:37, Josh Poimboeuf wrote:
> On Wed, Nov 18, 2015 at 10:56:00AM +0100, Miroslav Benes wrote:
> > On Mon, 16 Nov 2015, Chris J Arges wrote:
> >
> > > @@ -281,29 +243,26 @@ 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,
> > > - 0, &reloc->val);
> > > - if (ret)
> > > - return ret;
> > > + if (reloc->sympos && (reloc->sympos != 1)) {
> > > + pr_err("symbol '%s' is external and has sympos %lu\n",
> > > + reloc->name, reloc->sympos);
> > > + return -EINVAL;
> > > }
> >
> > Oh, this check should not be here, right? I think it needs to be under 'if
> > (reloc->external) {'.
> >
> > And
> >
> > if (reloc->sympos && (reloc->sympos != 1)) {
> >
> > can be written as
> >
> > if (reloc->sympos > 1) {
> >
> > Anyway, I am not sure if this is the right thing to do. I know Petr
> > suggested it, but maybe it would be sufficient just to error out when
> > reloc->sympos is specified for external symbols. Despite there is a valid
> > value (== 1). Like it had been done before. Josh came up with the issue.
> > So what is your opinion, Josh? It is a nitpick, but nevertheless.
>
> I tend to agree. IMO, any non-zero value of sympos doesn't make sense
> for external symbols.
OK, I do not have a strong opinion here. I am fain with refusing any
non-zero value.
Just please improve the error message. If I find in dmesg
"symbol 'bla' is external and has sympos 1" I would worry what
the problem is.
Please, somehow express that sympos is not supported for external
relocated symbols. You are not limited by the 80 characters here!
Best Regards,
Petr
On Wed 2015-11-18 10:39:24, Chris J Arges wrote:
> On 11/18/2015 10:37 AM, Petr Mladek wrote:
> > On Mon 2015-11-16 11:03:06, Chris J Arges wrote:
> >> In cases of duplicate symbols, sympos will be used to disambiguate instead
> >> of val. By default 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 in kallsyms for the patched object will be used
> >> for patching if it is valid.
> >>
> >> Remove klp_verify_callback, klp_verify_args and klp_verify_vmlinux_symbol
> >> as they are no longer used.
> >>
> >> >From the klp_reloc structure remove val, as it can be refactored as a
> > ^
> > Please, remove this extra '>'. I guess that you cut&pasted the
> > comment from a mail reply ;-)
> >
> > Best Regards,
> > Petr
> >
>
> I think that '>' was added in a subsequent reply. My patch doesn't have
> that, and I'm guessing checkpatch would have warned me about that as well.
> : )
Heh, it seems that some mail delivery tool or mail client tries to be
clever. I see the '>' in you original mail. But it is fine in the
archive, see
http://article.gmane.org/gmane.linux.kernel/2086364
http://marc.info/?l=linux-kernel&m=144769343212421
I am sorry for the noise.
Best Regards,
Petr
On Wed, 18 Nov 2015, Petr Mladek wrote:
> > I think that '>' was added in a subsequent reply. My patch doesn't have
> > that, and I'm guessing checkpatch would have warned me about that as well.
> > : )
>
> Heh, it seems that some mail delivery tool or mail client tries to be
> clever. I see the '>' in you original mail. But it is fine in the
> archive, see
> http://article.gmane.org/gmane.linux.kernel/2086364
> http://marc.info/?l=linux-kernel&m=144769343212421
It has to be stored this way if you are using mbox format as a storage, as
lines startng with "From" that directly follow an empty line (which is
exactly what happened here) have special meaning in mbox format (it is
used as a message separator).
--
Jiri Kosina
SUSE Labs