Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory
From: Chris J Arges
Date: Mon Nov 09 2015 - 18:01:17 EST
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@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
>> @@ -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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/