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

On Tue 2015-11-03 11:52:08, Miroslav Benes wrote:

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 might happen when the function name is unique. Then we might but
we do not need to pre-define the address in the patch.

Also I would omit the suffix at all when it is the first occurrence.
It will cause that unique symbols will not be numbered.

Best Regards,
Petr