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 will be used for patching if it is
valid. Finally, old_addr is now an internal structure element and not to
be specified by the user.
Signed-off-by: Chris J Arges <[email protected]>
---
include/linux/livepatch.h | 20 ++++++++++--------
kernel/livepatch/core.c | 53 +++++++++++++++++++++++------------------------
2 files changed, 37 insertions(+), 36 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..df7b752 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 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..26f9778 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,
@@ -159,36 +160,45 @@ 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 address, return only if checking pos and
+ * it matches count.
*/
- args->addr = addr;
args->count++;
+ args->addr = addr;
+ if ((args->pos > 0) && (args->count == args->pos))
+ 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;
}
@@ -239,22 +249,11 @@ 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 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;
+ /*
+ * Verify the symbol, find old_addr, and write it to the structure.
+ */
+ return klp_find_object_symbol(obj->name, func->old_name,
+ &func->old_addr, func->old_sympos);
}
/*
@@ -277,7 +276,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 +306,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;
}
--
1.9.1
On Wed, Nov 11, 2015 at 10:28:59AM -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 will be used for patching if it is
> valid. Finally, old_addr is now an internal structure element and not to
> be specified by the user.
>
> Signed-off-by: Chris J Arges <[email protected]>
> ---
> include/linux/livepatch.h | 20 ++++++++++--------
> kernel/livepatch/core.c | 53 +++++++++++++++++++++++------------------------
> 2 files changed, 37 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..df7b752 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 is used.
I would clarify this:
...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..26f9778 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;
There's a comment above this that says:
/*
* 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.
*/
That comment is no longer accurate and can probably be removed since IMO
the purpose of 'count' is obvious.
> };
>
> static int klp_find_callback(void *data, const char *name,
> @@ -159,36 +160,45 @@ 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 address, return only if checking pos and
> + * it matches count.
> */
> - args->addr = addr;
> args->count++;
> + args->addr = addr;
> + if ((args->pos > 0) && (args->count == args->pos))
> + 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;
> }
> @@ -239,22 +249,11 @@ 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 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;
> + /*
> + * Verify the symbol, find old_addr, and write it to the structure.
> + */
> + return klp_find_object_symbol(obj->name, func->old_name,
> + &func->old_addr, func->old_sympos);
klp_find_verify_func_addr() is no longer correctly named and can
probably be removed since klp_init_object_loaded() can call
klp_find_object_symbol() directly.
> }
>
> /*
> @@ -277,7 +276,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 +306,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;
> }
> --
> 1.9.1
>
--
Josh
Next to Josh's remarks I have some more (mainly nitpicks, so it is often
up to you).
On Wed, 11 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 will be used for patching if it is
"...occurrence of the symbol in kallsyms for the patched object will
be used..."
Just to have it even in the changelog for clarity.
> valid. Finally, old_addr is now an internal structure element and not to
> be specified by the user.
> @@ -159,36 +160,45 @@ 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 address, return only if checking pos and
> + * it matches count.
> */
> - args->addr = addr;
> args->count++;
> + args->addr = addr;
I guess that this row swap is remnant of a rebase. Anyway it is
superfluous.
> + if ((args->pos > 0) && (args->count == args->pos))
> + return 1;
We could add an optimization here. If args->pos == 0 and args->count > 1
we can return 1, because the symbol is not unique. The case is then
correctly handled in klp_find_object_symbol. There is no need to walk
through the rest of kallsyms.
Thanks,
Miroslav