In cases of duplicate symbols, old_sympos will be used to disambiguate
instead of old_addr. 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 in kallsyms for the patched object
will be used for patching if it is valid.
In addition, make old_addr an internal structure field not to be specified
by the user. Finally, remove klp_find_verify_func_addr as it can be
replaced by klp_find_object_symbol directly.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 20 ++++++++-------
kernel/livepatch/core.c | 65 ++++++++++++++++++++---------------------------
2 files changed, 38 insertions(+), 47 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..3d18dff 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
- * can be found (optional, vmlinux patches only)
+ * @old_sympos: a hint indicating which symbol position the old function
+ * can be found (optional)
+ * @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,18 @@ 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 livepatch objects. If this field is zero,
+ * it is expected the symbol is unique, otherwise patching fails. If
+ * this value is greater than zero then that occurrence of the symbol
+ * in kallsyms for the given object is used.
*/
- 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..479d75e 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -135,13 +135,8 @@ struct klp_find_arg {
const char *objname;
const char *name;
unsigned long addr;
- /*
- * If count == 0, the symbol was not found. If count == 1, a unique
- * match was found and addr is set. If count > 1, there is
- * unresolvable ambiguity among "count" number of symbols with the same
- * name in the same object.
- */
unsigned long count;
+ unsigned long pos;
};
static int klp_find_callback(void *data, const char *name,
@@ -159,36 +154,46 @@ static int klp_find_callback(void *data, const char *name,
return 0;
/*
- * args->addr might be overwritten if another match is found
- * but klp_find_object_symbol() handles this and only returns the
- * addr if count == 1.
+ * Increment and assign the address. Return 1 when the desired symbol
+ * position is found or the search for a unique symbol fails.
*/
args->addr = addr;
args->count++;
+ if (((args->pos > 0) && (args->count == args->pos)) ||
+ ((args->pos == 0) && (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. If sympos is 0, ensure symbol is unique;
+ * otherwise ensure 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)
+ else if (args.count > 1 && sympos == 0) {
pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
args.count, name, objname);
- else {
+ } else if (sympos != args.count && sympos > 0) {
+ 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;
}
@@ -236,27 +241,6 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
return 0;
}
-static int klp_find_verify_func_addr(struct klp_object *obj,
- struct klp_func *func)
-{
- 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_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);
-
- return ret;
-}
-
/*
* external symbols are located outside the parent object (where the parent
* object is either vmlinux or the kmod being patched).
@@ -277,7 +261,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 +291,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;
}
@@ -749,8 +733,13 @@ static int klp_init_object_loaded(struct klp_patch *patch,
return ret;
}
+ /*
+ * Verify the symbol, find old_addr, and write it to the structure.
+ */
klp_for_each_func(obj, func) {
- ret = klp_find_verify_func_addr(obj, func);
+ ret = klp_find_object_symbol(obj->name, func->old_name,
+ &func->old_addr,
+ func->old_sympos);
if (ret)
return ret;
}
--
1.9.1
On Thu, Nov 12, 2015 at 10:59:50AM -0600, Chris J Arges wrote:
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. 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 in kallsyms for the patched object
> will be used for patching if it is valid.
>
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
>
> Signed-off-by: Chris J Arges <[email protected]>
> ---
> include/linux/livepatch.h | 20 ++++++++-------
> kernel/livepatch/core.c | 65 ++++++++++++++++++++---------------------------
> 2 files changed, 38 insertions(+), 47 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..3d18dff 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
> - * can be found (optional, vmlinux patches only)
> + * @old_sympos: a hint indicating which symbol position the old function
> + * can be found (optional)
> + * @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,18 @@ 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 livepatch objects. If this field is zero,
> + * it is expected the symbol is unique, otherwise patching fails. If
> + * this value is greater than zero then that occurrence of the symbol
> + * in kallsyms for the given object is used.
> */
> - 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..479d75e 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -135,13 +135,8 @@ struct klp_find_arg {
> const char *objname;
> const char *name;
> unsigned long addr;
> - /*
> - * If count == 0, the symbol was not found. If count == 1, a unique
> - * match was found and addr is set. If count > 1, there is
> - * unresolvable ambiguity among "count" number of symbols with the same
> - * name in the same object.
> - */
> unsigned long count;
> + unsigned long pos;
> };
>
> static int klp_find_callback(void *data, const char *name,
> @@ -159,36 +154,46 @@ static int klp_find_callback(void *data, const char *name,
> return 0;
>
> /*
> - * args->addr might be overwritten if another match is found
> - * but klp_find_object_symbol() handles this and only returns the
> - * addr if count == 1.
> + * Increment and assign the address. Return 1 when the desired symbol
> + * position is found or the search for a unique symbol fails.
> */
> args->addr = addr;
> args->count++;
> + if (((args->pos > 0) && (args->count == args->pos)) ||
> + ((args->pos == 0) && (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)
One nit about this interface. I think it would be clearer if the sympos
argument came before the addr argument:
static int klp_find_object_symbol(const char *objname, const char *name,
unsigned long sympos, unsigned long addr)
Because:
1. it puts all input arguments before the output argument; and
2. it puts the sympos in its logical place immediately after the
function name.
--
Josh
On Thu, 12 Nov 2015, Chris J Arges wrote:
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. 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 in kallsyms for the patched object
> will be used for patching if it is valid.
>
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
The changelog has to cover the "why is this better than previous aproach".
It's neither in the cover letter (that by default doesn't make it into
changelog anyway) nor in either of the commit logs.
Thanks,
--
Jiri Kosina
SUSE Labs
On Thu, 12 Nov 2015, Chris J Arges wrote:
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..3d18dff 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
> - * can be found (optional, vmlinux patches only)
> + * @old_sympos: a hint indicating which symbol position the old function
> + * can be found (optional)
> + * @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,18 @@ struct klp_func {
> /* external */
> const char *old_name;
> void *new_func;
> +
A nit, but this new empty line is not necessary. Let's keep all the
external stuff in one pack.
> /*
> - * 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 livepatch objects. If this field is zero,
> + * it is expected the symbol is unique, otherwise patching fails. If
> + * this value is greater than zero then that occurrence of the symbol
> + * in kallsyms for the given object is used.
> */
> @@ -749,8 +733,13 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> return ret;
> }
>
> + /*
> + * Verify the symbol, find old_addr, and write it to the structure.
> + */
It is not verification anymore. I think a comment here is not necessary.
It is quite clear what klp_find_object_symbol does.
> klp_for_each_func(obj, func) {
> - ret = klp_find_verify_func_addr(obj, func);
> + ret = klp_find_object_symbol(obj->name, func->old_name,
> + &func->old_addr,
> + func->old_sympos);
> if (ret)
> return ret;
> }
Thanks,
Miroslav
On Thu 2015-11-12 10:59:50, Chris J Arges wrote:
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. 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 in kallsyms for the patched object
> will be used for patching if it is valid.
>
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
>
> Signed-off-by: Chris J Arges <[email protected]>
> ---
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..479d75e 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -159,36 +154,46 @@ static int klp_find_callback(void *data, const char *name,
> return 0;
>
> /*
> - * args->addr might be overwritten if another match is found
> - * but klp_find_object_symbol() handles this and only returns the
> - * addr if count == 1.
> + * Increment and assign the address. Return 1 when the desired symbol
> + * position is found or the search for a unique symbol fails.
> */
> args->addr = addr;
> args->count++;
I would avoid the obvious things and move the comment here:
/*
* Finish the search when the symbol is found on the desired
* position or the position is not defined for non-unique
* symbol.
*/
> + if (((args->pos > 0) && (args->count == args->pos)) ||
> + ((args->pos == 0) && (args->count > 1)))
> + return 1;
I wonder if this is better readable:
if ((args->pos && (args->count == args->pos)) ||
(!args->pos && (args->count > 1)))
return 1;
> return 0;
> }
>
[...]
> @@ -277,7 +261,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);
Please, add a comment here that external symbols always have to be unique.
> }
>
> static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +291,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);
Please, mention in the commit message that the support for sympos will
be added into relocations in a separate patch. Or add a FIXME comment
here. It will make easier the review and git history archaeology.
Thank you,
Petr