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

On 11/03/2015 10:50 AM, Josh Poimboeuf wrote:

On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:

quoted

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

quoted

On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:

quoted

On Mon, 2 Nov 2015, Chris J Arges wrote:

[...]

quoted

+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.

quoted

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.

quoted

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?

quoted

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.

quoted

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