Merge pull request #986 from euspectre/old-replace-fix

Patch replacement fixes for the old KPatch core

kmod/core/core.c

@@ -83,6 +83,11 @@ struct kpatch_kallsyms_args {
 	unsigned long pos;
 };
 
+struct kpatch_apply_patch_args {
+	struct kpatch_module *kpmod;
+	bool replace;
+};
+
 /* this is a double loop, use goto instead of break */
 #define do_for_each_linked_func(kpmod, func) {				\
 	struct kpatch_object *_object;					\
@@ -207,7 +212,8 @@ static inline int kpatch_compare_addresses(unsigned long stack_addr,
 }
 
 static int kpatch_backtrace_address_verify(struct kpatch_module *kpmod,
-					   unsigned long address)
+					   unsigned long address,
+					   bool replace)
 {
 	struct kpatch_func *func;
 	int i;
@@ -242,8 +248,11 @@ static int kpatch_backtrace_address_verify(struct kpatch_module *kpmod,
 	} while_for_each_linked_func();
 
 	/* in the replace case, need to check the func hash as well */
-	hash_for_each_rcu(kpatch_func_hash, i, func, node) {
+	if (replace) {
-		if (func->op == KPATCH_OP_UNPATCH && !func->force) {
+		hash_for_each_rcu(kpatch_func_hash, i, func, node) {
+			if (func->op != KPATCH_OP_UNPATCH || func->force)
+				continue;
+
 			ret = kpatch_compare_addresses(address,
 						       func->new_addr,
 						       func->new_size,
@@ -262,7 +271,8 @@ static int kpatch_backtrace_address_verify(struct kpatch_module *kpmod,
  *
  * This function is called from stop_machine() context.
  */
-static int kpatch_verify_activeness_safety(struct kpatch_module *kpmod)
+static int kpatch_verify_activeness_safety(struct kpatch_module *kpmod,
+					   bool replace)
 {
 	struct task_struct *g, *t;
 	int i;
@@ -284,7 +294,8 @@ static int kpatch_verify_activeness_safety(struct kpatch_module *kpmod)
 			if (trace.entries[i] == ULONG_MAX)
 				break;
 			ret = kpatch_backtrace_address_verify(kpmod,
-							      trace.entries[i]);
+							      trace.entries[i],
+							      replace);
 			if (ret)
 				goto out;
 		}
@@ -350,12 +361,17 @@ static inline void post_unpatch_callback(struct kpatch_object *object)
 /* Called from stop_machine */
 static int kpatch_apply_patch(void *data)
 {
-	struct kpatch_module *kpmod = data;
+	struct kpatch_apply_patch_args *args = data;
+	struct kpatch_module *kpmod;
 	struct kpatch_func *func;
+	struct hlist_node *tmp;
 	struct kpatch_object *object;
 	int ret;
+	int i;
 
-	ret = kpatch_verify_activeness_safety(kpmod);
+	kpmod = args->kpmod;
+
+	ret = kpatch_verify_activeness_safety(kpmod, args->replace);
 	if (ret) {
 		kpatch_state_finish(KPATCH_STATE_FAILURE);
 		return ret;
@@ -396,6 +412,19 @@ static int kpatch_apply_patch(void *data)
 		goto err;
 	}
 
+	/*
+	 * The new patch has been applied successfully. Remove the functions
+	 * provided by the replaced patches (if any) from hash, to make sure
+	 * they will not be executed anymore.
+	 */
+	if (args->replace) {
+		hash_for_each_safe(kpatch_func_hash, i, tmp, func, node) {
+			if (func->op != KPATCH_OP_UNPATCH)
+				continue;
+			hash_del_rcu(&func->node);
+		}
+	}
+
 	/* run any user-defined post-patch callbacks */
 	list_for_each_entry(object, &kpmod->objects, list)
 		post_patch_callback(object);
@@ -417,7 +446,7 @@ static int kpatch_remove_patch(void *data)
 	struct kpatch_object *object;
 	int ret;
 
-	ret = kpatch_verify_activeness_safety(kpmod);
+	ret = kpatch_verify_activeness_safety(kpmod, false);
 	if (ret) {
 		kpatch_state_finish(KPATCH_STATE_FAILURE);
 		return ret;
@@ -975,12 +1004,41 @@ out:
 	return 0;
 }
 
+/*
+ * Remove the obsolete functions from the ftrace filter.
+ * Return 1 if one or more of such functions have 'force' flag set,
+ * 0 otherwise.
+ */
+static int kpatch_ftrace_remove_unpatched_funcs(void)
+{
+	struct kpatch_module *kpmod;
+	struct kpatch_func *func;
+	int force = 0;
+
+	list_for_each_entry(kpmod, &kpmod_list, list) {
+		do_for_each_linked_func(kpmod, func) {
+			if (func->op != KPATCH_OP_UNPATCH)
+				continue;
+			if (func->force)
+				force = 1;
+			WARN_ON(kpatch_ftrace_remove_func(func->old_addr));
+		} while_for_each_linked_func();
+	}
+
+	return force;
+}
+
 int kpatch_register(struct kpatch_module *kpmod, bool replace)
 {
-	int ret, i, force = 0;
+	int ret, i;
 	struct kpatch_object *object, *object_err = NULL;
 	struct kpatch_func *func;
 
+	struct kpatch_apply_patch_args args = {
+		.kpmod = kpmod,
+		.replace = replace,
+	};
+
 	if (!kpmod->mod || list_empty(&kpmod->objects))
 		return -EINVAL;
 
@@ -1032,24 +1090,18 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 	 * Idle the CPUs, verify activeness safety, and atomically make the new
 	 * functions visible to the ftrace handler.
 	 */
-	ret = stop_machine(kpatch_apply_patch, kpmod, NULL);
+	ret = stop_machine(kpatch_apply_patch, &args, NULL);
 
 	/*
-	 * For the replace case, remove any obsolete funcs from the hash and
+	 * For the replace case, remove any obsolete funcs from the ftrace
-	 * the ftrace filter, and disable the owning patch module so that it
+	 * filter, and disable the owning patch module so that it can be
-	 * can be removed.
+	 * removed.
 	 */
 	if (!ret && replace) {
 		struct kpatch_module *kpmod2, *safe;
+		int force;
 
-		hash_for_each_rcu(kpatch_func_hash, i, func, node) {
+		force = kpatch_ftrace_remove_unpatched_funcs();
-			if (func->op != KPATCH_OP_UNPATCH)
-				continue;
-			if (func->force)
-				force = 1;
-			hash_del_rcu(&func->node);
-			WARN_ON(kpatch_ftrace_remove_func(func->old_addr));
-		}
 
 		list_for_each_entry_safe(kpmod2, safe, &kpmod_list, list) {
 			if (kpmod == kpmod2)