Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

[Chris J Arges <chris.j.arges@canonical.com>]

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