[PATCH 6/6] livepatch v5: clean up usage of the old and new addresses

[Petr Mladek]

The situation with variables storing the address of the old and new code
is not ideal. One is called "func" and the other is called "addr".
The one is pointer and the other is unsigned long. It makes sense
from the point of how the values are defined. But it might make problems
to understand the code when the values are used.

This patch introduces two new variables "old_ip" and "new_ip".
They have the same type and the name is symmetric. They are supposed
to carry the address of the mcount call-site that ftrace framework
operates with. The value is the same as the function address
on x86 but it might be different on another architectures.

The change is inspired by kGraft that even adds ftrace_function_to_fentry(),
see https://lkml.org/lkml/2014/4/30/688. The function allows to find the right
mcount call-site location. This patch uses ftrace_location() that just allows
to verify that the address is correct. It means that we detect potential problem
already when the patch in being initialized.

Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
---
 include/linux/livepatch.h | 24 ++++++++++++-----------
 kernel/livepatch/core.c   | 49 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index b61fe7402f1f..32f82d798c54 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -36,8 +36,16 @@ 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_addr:	address of the function to be patched; if predefined then
+ *		the value is used as a hint conveying at what address
+ *		the old function can be found; otherwise, the value is
+ *		located by name with kallsyms; the predefined value makes
+ *		sense only for vmlinux function and when randomization is
+ *		disabled
+ * @old_ip:	address of the mcount call-site for the old function;
+ *		it is the address that ftrace works with
+ * @new_ip:	address of the mcount call-sire for the new function;
+ *		it is the address that ftrace works with
  * @kobj:	kobject for sysfs resources
  * @fops:	ftrace operations structure
  * @state:	tracks function-level patch application state
@@ -46,15 +54,9 @@ 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.
-	 */
 	unsigned long old_addr;
+	unsigned long old_ip;
+	unsigned long new_ip;
 
 	/* internal */
 	struct kobject kobj;
@@ -88,7 +90,7 @@ struct klp_reloc {
  * @funcs:	function entries for functions to be patched in the object
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
- * 		(NULL for vmlinux)
+ *		(NULL for vmlinux)
  * @state:	tracks object-level patch application state
  */
 struct klp_object {
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index fe312b9ada78..c54230bf17fe 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -179,8 +179,8 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
 	return -EINVAL;
 }
 
-static int klp_find_verify_func_addr(struct klp_object *obj,
-				     struct klp_func *func)
+static int klp_find_verify_func_old_addr(struct klp_object *obj,
+					 struct klp_func *func)
 {
 	int ret;
 
@@ -205,6 +205,36 @@ static int klp_find_verify_func_addr(struct klp_object *obj,
 	return ret;
 }
 
+static int klp_find_verify_func_addresses(struct klp_object *obj,
+				     struct klp_func *func)
+{
+	int ret;
+
+	ret = klp_find_verify_func_old_addr(obj, func);
+	if (ret)
+		return ret;
+
+	func->old_ip = ftrace_location(func->old_addr);
+	if (!func->old_ip) {
+		pr_err("function '%s' at the address '%lx' can not be patched\n",
+		       func->old_name, func->old_addr);
+		return -EINVAL;
+	}
+
+	/*
+	 * In fact, the new function need not be traceable. This check is
+	 * used to make sure that the address is a correct mcount call-site.
+	 */
+	func->new_ip = ftrace_location((unsigned long)func->new_func);
+	if (!func->old_ip) {
+		pr_err("function at the address '%p' can not be used for patching\n",
+		       func->new_func);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * external symbols are located outside the parent object (where the parent
  * object is either vmlinux or the kmod being patched).
@@ -277,7 +307,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 {
 	struct klp_func *func = ops->private;
 
-	regs->ip = (unsigned long)func->new_func;
+	regs->ip = func->new_ip;
 }
 
 static int klp_disable_func(struct klp_func *func)
@@ -287,7 +317,7 @@ static int klp_disable_func(struct klp_func *func)
 	if (WARN_ON(func->state != KLP_ENABLED))
 		return -EINVAL;
 
-	if (WARN_ON(!func->old_addr))
+	if (WARN_ON(!func->old_ip))
 		return -EINVAL;
 
 	ret = unregister_ftrace_function(func->fops);
@@ -297,7 +327,7 @@ static int klp_disable_func(struct klp_func *func)
 		return ret;
 	}
 
-	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+	ret = ftrace_set_filter_ip(func->fops, func->old_ip, 1, 0);
 	if (ret)
 		pr_warn("function unregister succeeded but failed to clear the filter\n");
 
@@ -310,13 +340,13 @@ static int klp_enable_func(struct klp_func *func)
 {
 	int ret;
 
-	if (WARN_ON(!func->old_addr))
+	if (WARN_ON(!func->old_ip))
 		return -EINVAL;
 
 	if (WARN_ON(func->state != KLP_DISABLED))
 		return -EINVAL;
 
-	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
+	ret = ftrace_set_filter_ip(func->fops, func->old_ip, 0, 0);
 	if (ret) {
 		pr_err("failed to set ftrace filter for function '%s' (%d)\n",
 		       func->old_name, ret);
@@ -327,7 +357,7 @@ static int klp_enable_func(struct klp_func *func)
 	if (ret) {
 		pr_err("failed to register ftrace handler for function '%s' (%d)\n",
 		       func->old_name, ret);
-		ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+		ftrace_set_filter_ip(func->fops, func->old_ip, 1, 0);
 	} else {
 		func->state = KLP_ENABLED;
 	}
@@ -594,6 +624,7 @@ static struct kobj_type klp_ktype_func = {
 static void klp_free_func_loaded(struct klp_func *func)
 {
 	func->old_addr = 0;
+	func->old_ip = 0;
 }
 
 /*
@@ -646,7 +677,7 @@ static void klp_free_patch(struct klp_patch *patch)
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_func_loaded(struct klp_object *obj, struct klp_func *func)
 {
-	return klp_find_verify_func_addr(obj, func);
+	return klp_find_verify_func_addresses(obj, func);
 }
 
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
-- 
1.8.5.2

--
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/