Merge pull request #242 from jpoimboe/future-loaded-modules

support for patching future loaded modules

kmod/core/core.c

@@ -58,7 +58,9 @@ static DEFINE_HASHTABLE(kpatch_func_hash, KPATCH_HASH_BITS);
 
 static DEFINE_SEMAPHORE(kpatch_mutex);
 
-static int kpatch_num_registered;
+LIST_HEAD(kpmod_list);
+
+static int kpatch_num_patched;
 
 static struct kobject *kpatch_root_kobj;
 struct kobject *kpatch_patches_kobj;
@@ -127,16 +129,6 @@ struct kpatch_func {
 	struct module *mod;
 };
 
-/*
- * This structure is allocated on a per registered module basis
- * and is stored in the struct kpatch_module "internal" field.
- * Any data associated with each registered module used internally
- * by this core module can be added here.
- */
-struct kpatch_internal {
-	struct kpatch_func *funcs;
-};
-
 static inline void kpatch_state_idle(void)
 {
 	int state = atomic_read(&kpatch_state);
@@ -208,7 +200,12 @@ static void kpatch_backtrace_address_verify(void *data, unsigned long address,
 		char *func_name;
 		struct kpatch_func *active_func;
 
-		func = &kpmod->internal->funcs[i];
+		func = &kpmod->funcs[i];
+
+		/* check if patch depends on a future module load */
+		if (!func->old_addr)
+			continue;
+
 		active_func = kpatch_get_func(func->old_addr);
 		if (!active_func) {
 			/* patching an unpatched func */
@@ -285,7 +282,7 @@ out:
 static int kpatch_apply_patch(void *data)
 {
 	struct kpatch_module *kpmod = data;
-	struct kpatch_func *funcs = kpmod->internal->funcs;
+	struct kpatch_func *funcs = kpmod->funcs;
 	int num_funcs = kpmod->patches_nr;
 	int i, ret;
 
@@ -297,8 +294,9 @@ static int kpatch_apply_patch(void *data)
 
 	/* tentatively add the new funcs to the global func hash */
 	for (i = 0; i < num_funcs; i++)
-		hash_add_rcu(kpatch_func_hash, &funcs[i].node,
+		if (funcs[i].old_addr)
-			     funcs[i].old_addr);
+			hash_add_rcu(kpatch_func_hash, &funcs[i].node,
+				     funcs[i].old_addr);
 
 	/* memory barrier between func hash add and state change */
 	smp_wmb();
@@ -313,7 +311,8 @@ static int kpatch_apply_patch(void *data)
 
 		/* Failed, we have to rollback patching process */
 		for (i = 0; i < num_funcs; i++)
-			hash_del_rcu(&funcs[i].node);
+			if (funcs[i].old_addr)
+				hash_del_rcu(&funcs[i].node);
 
 		return -EBUSY;
 	}
@@ -325,7 +324,7 @@ static int kpatch_apply_patch(void *data)
 static int kpatch_remove_patch(void *data)
 {
 	struct kpatch_module *kpmod = data;
-	struct kpatch_func *funcs = kpmod->internal->funcs;
+	struct kpatch_func *funcs = kpmod->funcs;
 	int num_funcs = kpmod->patches_nr;
 	int ret, i;
 
@@ -342,7 +341,8 @@ static int kpatch_remove_patch(void *data)
 
 	/* Succeeded, remove all updating funcs from hash table */
 	for (i = 0; i < num_funcs; i++)
-		hash_del_rcu(&funcs[i].node);
+		if (funcs[i].old_addr)
+			hash_del_rcu(&funcs[i].node);
 
 	return 0;
 }
@@ -406,6 +406,60 @@ static struct ftrace_ops kpatch_ftrace_ops __read_mostly = {
 	.flags = FTRACE_OPS_FL_SAVE_REGS,
 };
 
+static int kpatch_ftrace_add_func(unsigned long ip)
+{
+	int ret;
+
+	/* check if any other patch modules have also patched this func */
+	if (kpatch_get_func(ip))
+		return 0;
+
+	ret = ftrace_set_filter_ip(&kpatch_ftrace_ops, ip, 0, 0);
+	if (ret) {
+		pr_err("can't set ftrace filter at address 0x%lx\n", ip);
+		return ret;
+	}
+
+	if (!kpatch_num_patched) {
+		ret = register_ftrace_function(&kpatch_ftrace_ops);
+		if (ret) {
+			pr_err("can't register ftrace handler\n");
+			ftrace_set_filter_ip(&kpatch_ftrace_ops, ip, 1, 0);
+			return ret;
+		}
+	}
+	kpatch_num_patched++;
+
+	return 0;
+}
+
+static int kpatch_ftrace_remove_func(unsigned long ip)
+{
+	int ret;
+
+	/* check if any other patch modules have also patched this func */
+	if (kpatch_get_func(ip))
+		return 0;
+
+	ret = ftrace_set_filter_ip(&kpatch_ftrace_ops, ip, 1, 0);
+	if (ret) {
+		pr_err("can't remove ftrace filter at address 0x%lx\n", ip);
+		return ret;
+	}
+
+	if (kpatch_num_patched == 1) {
+		ret = unregister_ftrace_function(&kpatch_ftrace_ops);
+		if (ret) {
+			pr_err("can't unregister ftrace handler\n");
+			ftrace_set_filter_ip(&kpatch_ftrace_ops, ip, 0, 0);
+			return ret;
+		}
+	}
+	kpatch_num_patched--;
+
+	return 0;
+}
+
 static void kpatch_put_modules(struct kpatch_func *funcs, int num_funcs)
 {
 	int i;
@@ -418,31 +472,6 @@ static void kpatch_put_modules(struct kpatch_func *funcs, int num_funcs)
 	}
 }
 
-/* Remove kpatch_funcs from ftrace filter */
-static void kpatch_remove_funcs_from_filter(struct kpatch_func *funcs,
-					    int num_funcs)
-{
-	int i, ret = 0;
-
-	for (i = 0; i < num_funcs; i++) {
-		struct kpatch_func *func = &funcs[i];
-
-		/*
-		 * If any other modules have also patched this function, don't
-		 * remove its ftrace handler.
-		 */
-		if (kpatch_get_func(func->old_addr))
-			continue;
-
-		/* Remove the ftrace handler for this function. */
-		ret = ftrace_set_filter_ip(&kpatch_ftrace_ops,
-		                  func->old_addr, 1, 0);
-
-		WARN(ret, "can't remove ftrace filter at address 0x%lx (rc=%d)",
-		     func->old_addr, ret);
-	}
-}
-
 static int kpatch_kallsyms_callback(void *data, const char *name,
 					 struct module *mod,
 					 unsigned long addr)
@@ -501,7 +530,7 @@ static int kpatch_write_relocations(struct kpatch_module *kpmod)
 	unsigned long core = (unsigned long)kpmod->mod->module_core;
 	unsigned long core_ro_size = kpmod->mod->core_ro_size;
 	unsigned long core_size = kpmod->mod->core_size;
-	unsigned long src_addr;
+	unsigned long src;
 	struct module *mod;
 
 	for (i = 0; i < kpmod->dynrelas_nr; i++) {
@@ -509,47 +538,60 @@ static int kpatch_write_relocations(struct kpatch_module *kpmod)
 
 		if (!strcmp(dynrela->objname, "vmlinux")) {
 			/* vmlinux */
-			ret = kpatch_verify_symbol_match(dynrela->name, dynrela->src);
+
+			/* check if reloc has already been written */
+			if (kpmod->enabled)
+				continue;
+
+			ret = kpatch_verify_symbol_match(dynrela->name,
+							 dynrela->src);
 			if (ret)
 				return ret;
-			src_addr = dynrela->src;
 		} else {
-			/* module, dynrela->src is not right */
+			/* module, dynrela->src needs to be discovered */
+
+			/* check if reloc has already been written */
+			if (dynrela->src)
+				continue;
+
+			/* check if dynrela depends on a future loaded module */
+			mutex_lock(&module_mutex);
 			mod = find_module(dynrela->objname);
-			if (!mod) {
+			mutex_unlock(&module_mutex);
-				pr_err("unable to find module '%s'\n",
+			if (!mod)
-				       dynrela->objname);
+				continue;
-				return -EINVAL;
+
-			}
+			src = kpatch_find_module_symbol(mod, dynrela->name);
-			src_addr = kpatch_find_module_symbol(mod, dynrela->name);
+			if (!src) {
-			if (!src_addr) {
 				pr_err("unable to find symbol '%s' in module '%s'\n",
 				       dynrela->name, mod->name);
 				return -EINVAL;
 			}
+
+			dynrela->src = src;
 		}
 
 		switch (dynrela->type) {
 			case R_X86_64_PC32:
 				loc = dynrela->dest;
-				val = (u32)(src_addr + dynrela->addend -
+				val = (u32)(dynrela->src + dynrela->addend -
 				            dynrela->dest);
 				size = 4;
 				break;
 			case R_X86_64_32S:
 				loc = dynrela->dest;
-				val = (s32)src_addr + dynrela->addend;
+				val = (s32)dynrela->src + dynrela->addend;
 				size = 4;
 				break;
 			case R_X86_64_64:
 				loc = dynrela->dest;
-				val = src_addr;
+				val = dynrela->src;
 				size = 8;
 				break;
 			default:
 				printk("unsupported rela type %ld for source %s (0x%lx <- 0x%lx) at index %d\n",
 				       dynrela->type, dynrela->name,
-				       dynrela->dest, src_addr, i);
+				       dynrela->dest, dynrela->src, i);
 				return -EINVAL;
 		}
 
@@ -581,29 +623,96 @@ static int kpatch_write_relocations(struct kpatch_module *kpmod)
 	return 0;
 }
 
-static int kpatch_calculate_old_addr(struct kpatch_func *func)
+static void kpatch_calculate_old_addr(struct kpatch_func *func)
 {
 	struct module *module;
 
 	if (!strcmp(func->patch->objname, "vmlinux")) {
 		func->old_addr = func->patch->old_offset;
-		return 0;
+		return;
 	}
 
+	/*
+	 * If the module hasn't been loaded yet, we'll patch it later in
+	 * kpatch_module_notify().
+	 */
+	mutex_lock(&module_mutex);
 	module = find_module(func->patch->objname);
 	if (!module) {
-		pr_err("patch contains code for a module that is not loaded\n");
+		mutex_unlock(&module_mutex);
-		return -EINVAL;
+		return;
 	}
 
-	if (!try_module_get(module)) {
+	/* should never fail because we have the mutex */
-		pr_err("failed to reference module\n");
+	WARN_ON(!try_module_get(module));
-		return -EINVAL;
+	mutex_unlock(&module_mutex);
-	}
 
 	func->mod = module;
 	func->old_addr = (unsigned long)module->module_core +
 	                 func->patch->old_offset;
+}
+
+static int kpatch_module_notify(struct notifier_block *nb, unsigned long action,
+				void *data)
+{
+	struct module *mod = data;
+	struct kpatch_module *kpmod;
+	int printed = 0, ret = 0, i;
+
+	if (action != MODULE_STATE_COMING)
+		return ret;
+
+	down(&kpatch_mutex);
+
+	list_for_each_entry(kpmod, &kpmod_list, list) {
+		if (!kpmod->pending)
+			continue;
+
+		/* temporarily clear pending, can be set again below */
+		kpmod->pending = 0;
+
+		for (i = 0; i < kpmod->patches_nr; i++) {
+			struct kpatch_func *func = &kpmod->funcs[i];
+
+			/* check if it has already been patched */
+			if (func->old_addr)
+				continue;
+
+			/* calculate actual old location (refs if module) */
+			kpatch_calculate_old_addr(func);
+
+			if (!func->old_addr) {
+				kpmod->pending = 1;
+				continue;
+			}
+
+			if (!printed) {
+				pr_notice("patching newly loaded module '%s'\n",
+					  mod->name);
+				printed = 1;
+			}
+
+			ret = kpatch_ftrace_add_func(func->old_addr);
+			if (ret)
+				goto out;
+
+			/* add to the global func hash */
+			hash_add_rcu(kpatch_func_hash, &func->node,
+				     func->old_addr);
+		}
+
+		/* write any newly valid relocations */
+		ret = kpatch_write_relocations(kpmod);
+		if (ret)
+			goto out;
+	}
+
+out:
+	up(&kpatch_mutex);
+
+	/* no way to stop the module load on error */
+	WARN(ret, "error (%d) patching newly loaded module '%s'\n", ret,
+	     mod->name);
 	return 0;
 }
 
@@ -616,30 +725,22 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 	if (!kpmod->mod || !kpmod->patches || !num_funcs)
 		return -EINVAL;
 
-	kpmod->enabled = false;
-
-	kpmod->internal = kmalloc(sizeof(*kpmod->internal), GFP_KERNEL);
-	if (!kpmod->internal)
-		return -ENOMEM;
-
 	funcs = kzalloc(sizeof(*funcs) * num_funcs, GFP_KERNEL);
-	if (!funcs) {
+	if (!funcs)
-		kfree(kpmod->internal);
 		return -ENOMEM;
-	}
-	kpmod->internal->funcs = funcs;
 
 	down(&kpatch_mutex);
 
+	kpmod->enabled = false;
+	kpmod->pending = 0;
+	kpmod->funcs = funcs;
+	list_add_tail(&kpmod->list, &kpmod_list);
+
 	if (!try_module_get(kpmod->mod)) {
 		ret = -ENODEV;
 		goto err_up;
 	}
 
-	ret = kpatch_write_relocations(kpmod);
-	if (ret)
-		goto err_put;
-
 	for (i = 0; i < num_funcs; i++) {
 		func = &funcs[i];
 
@@ -648,42 +749,31 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 		func->patch = &kpmod->patches[i];
 
 		/* calculate actual old location (refs if module) */
-		ret = kpatch_calculate_old_addr(func);
+		kpatch_calculate_old_addr(func);
-		if (ret)
+
-			goto err_rollback;
+		/* check if patch depends on a future loaded module */
+		if (!func->old_addr) {
+			kpmod->pending = 1;
+			continue;
+		}
 
 		ret = kpatch_verify_symbol_match(func->patch->name,
-		                               func->old_addr);
+						 func->old_addr);
-		if (ret)
+		if (ret) {
-			goto err_rollback;
+			num_funcs = i;
-
+			goto err_rollback;
-		/*
+		}
-		 * If any other modules have also patched this function, it
+
-		 * already has an ftrace handler.
+		ret = kpatch_ftrace_add_func(func->old_addr);
-		 */
-		if (kpatch_get_func(func->old_addr))
-			continue;
-
-		/* Add an ftrace handler for this function. */
-		ret = ftrace_set_filter_ip(&kpatch_ftrace_ops,
-		                  func->old_addr, 0, 0);
 		if (ret) {
-			pr_err("can't set ftrace filter at address 0x%lx\n",
-			       func->old_addr);
 			num_funcs = i;
 			goto err_rollback;
 		}
 	}
 
-	/* Register the ftrace handler if it hasn't been done already. */
+	ret = kpatch_write_relocations(kpmod);
-	if (!kpatch_num_registered) {
+	if (ret)
-		ret = register_ftrace_function(&kpatch_ftrace_ops);
+		goto err_rollback;
-		if (ret) {
-			pr_err("can't register ftrace handler\n");
-			goto err_rollback;
-		}
-	}
-	kpatch_num_registered++;
 
 	if (replace)
 		hash_for_each_rcu(kpatch_func_hash, i, func, node)
@@ -705,21 +795,29 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 	 * the ftrace filter, and disable the owning patch module so that it
 	 * can be removed.
 	 */
-	if (!ret && replace)
+	if (!ret && replace) {
+		struct kpatch_module *kpmod2, *safe;
+
 		hash_for_each_rcu(kpatch_func_hash, i, func, node) {
 			if (func->op != KPATCH_OP_UNPATCH)
 				continue;
 			hash_del_rcu(&func->node);
-			kpatch_remove_funcs_from_filter(func, 1);
+			WARN_ON(kpatch_ftrace_remove_func(func->old_addr));
-			if (func->kpmod->enabled) {
-				kpatch_num_registered--;
-				func->kpmod->enabled = false;
-				pr_notice("unloaded patch module \"%s\"\n",
-					  func->kpmod->mod->name);
-				module_put(func->kpmod->mod);
-			}
 		}
 
+		list_for_each_entry_safe(kpmod2, safe, &kpmod_list, list) {
+			if (kpmod == kpmod2)
+				continue;
+
+			kpmod2->enabled = false;
+			pr_notice("unloaded patch module '%s'\n",
+				  kpmod2->mod->name);
+			module_put(kpmod2->mod);
+			list_del(&kpmod2->list);
+		}
+	}
+
+
 	/* memory barrier between func hash and state write */
 	smp_wmb();
 
@@ -736,7 +834,7 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 	synchronize_rcu();
 
 	if (ret)
-		goto err_unregister;
+		goto err_ops;
 
 	for (i = 0; i < num_funcs; i++)
 		funcs[i].op = KPATCH_OP_NONE;
@@ -745,41 +843,33 @@ int kpatch_register(struct kpatch_module *kpmod, bool replace)
 	pr_notice_once("tainting kernel with TAINT_USER\n");
 	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 
-	pr_notice("loaded patch module \"%s\"\n", kpmod->mod->name);
+	pr_notice("loaded patch module '%s'\n", kpmod->mod->name);
 
 	kpmod->enabled = true;
 
 	up(&kpatch_mutex);
 	return 0;
 
-err_unregister:
+err_ops:
 	if (replace)
 		hash_for_each_rcu(kpatch_func_hash, i, func, node)
 			func->op = KPATCH_OP_NONE;
-	if (kpatch_num_registered == 1) {
-		int ret2 = unregister_ftrace_function(&kpatch_ftrace_ops);
-		if (ret2) {
-			pr_err("ftrace unregister failed (%d)\n", ret2);
-			goto err_rollback;
-		}
-	}
-	kpatch_num_registered--;
 err_rollback:
-	kpatch_put_modules(funcs, num_funcs);
+	for (i = 0; i < num_funcs; i++)
-	kpatch_remove_funcs_from_filter(funcs, num_funcs);
+		kpatch_ftrace_remove_func(funcs[i].old_addr);
-err_put:
+	kpatch_put_modules(funcs, kpmod->patches_nr);
 	module_put(kpmod->mod);
 err_up:
+	list_del(&kpmod->list);
 	up(&kpatch_mutex);
-	kfree(kpmod->internal->funcs);
+	kfree(kpmod->funcs);
-	kfree(kpmod->internal);
 	return ret;
 }
 EXPORT_SYMBOL(kpatch_register);
 
 int kpatch_unregister(struct kpatch_module *kpmod)
 {
-	struct kpatch_func *funcs = kpmod->internal->funcs;
+	struct kpatch_func *funcs = kpmod->funcs;
 	int num_funcs = kpmod->patches_nr;
 	int i, ret;
 
@@ -816,25 +906,26 @@ int kpatch_unregister(struct kpatch_module *kpmod)
 		goto out;
 	}
 
-	kpatch_num_registered--;
+	for (i = 0; i < num_funcs; i++) {
-	if (!kpatch_num_registered) {
+		if (!funcs[i].old_addr)
-		ret = unregister_ftrace_function(&kpatch_ftrace_ops);
+			continue;
+		ret = kpatch_ftrace_remove_func(funcs[i].old_addr);
 		if (ret) {
-			WARN(1, "can't unregister ftrace handler");
+			WARN(1, "can't unregister ftrace for address 0x%lx\n",
-			kpatch_num_registered++;
+			     funcs[i].old_addr);
+			goto out;
 		}
 	}
 
 	kpatch_put_modules(funcs, num_funcs);
-	kpatch_remove_funcs_from_filter(funcs, num_funcs);
 
-	kfree(kpmod->internal->funcs);
+	kfree(kpmod->funcs);
-	kfree(kpmod->internal);
 
-	pr_notice("unloaded patch module \"%s\"\n", kpmod->mod->name);
+	pr_notice("unloaded patch module '%s'\n", kpmod->mod->name);
 
 	kpmod->enabled = false;
 	module_put(kpmod->mod);
+	list_del(&kpmod->list);
 
 out:
 	up(&kpatch_mutex);
@@ -842,23 +933,44 @@ out:
 }
 EXPORT_SYMBOL(kpatch_unregister);
 
+
+static struct notifier_block kpatch_module_nb = {
+	.notifier_call = kpatch_module_notify,
+	.priority = INT_MIN, /* called last */
+};
+
 static int kpatch_init(void)
 {
+	int ret;
+
 	kpatch_root_kobj = kobject_create_and_add("kpatch", kernel_kobj);
 	if (!kpatch_root_kobj)
 		return -ENOMEM;
 
 	kpatch_patches_kobj = kobject_create_and_add("patches",
 						     kpatch_root_kobj);
-	if (!kpatch_patches_kobj)
+	if (!kpatch_patches_kobj) {
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_root_kobj;
+	}
+
+	ret = register_module_notifier(&kpatch_module_nb);
+	if (ret)
+		goto err_patches_kobj;
 
 	return 0;
+
+err_patches_kobj:
+	kobject_put(kpatch_patches_kobj);
+err_root_kobj:
+	kobject_put(kpatch_root_kobj);
+	return ret;
 }
 
 static void kpatch_exit(void)
 {
-	WARN_ON(kpatch_num_registered != 0);
+	WARN_ON(kpatch_num_patched != 0);
+	WARN_ON(unregister_module_notifier(&kpatch_module_nb));
 	kobject_put(kpatch_patches_kobj);
 	kobject_put(kpatch_root_kobj);
 }

kmod/core/kpatch.h

@@ -46,16 +46,19 @@ struct kpatch_dynrela {
 #include <linux/types.h>
 #include <linux/module.h>
 
-struct kpatch_internal;
-
 struct kpatch_module {
+	/* public */
 	struct module *mod;
 	struct kpatch_patch *patches;
 	struct kpatch_dynrela *dynrelas;
 	int patches_nr;
 	int dynrelas_nr;
 	bool enabled;
-	struct kpatch_internal *internal; /* used internally by core module */
+
+	/* private */
+	struct list_head list;
+	struct kpatch_func *funcs;
+	bool pending;
 };
 
 extern struct kobject *kpatch_patches_kobj;

kpatch-build/create-diff-object.c

@@ -1660,7 +1660,7 @@ void kpatch_create_dynamic_rela_sections(struct kpatch_elf *kelf,
 	sec->sh.sh_type = SHT_PROGBITS;
 	sec->sh.sh_entsize = sizeof(*dynrelas);
 	sec->sh.sh_addralign = 8;
-	sec->sh.sh_flags = SHF_ALLOC;
+	sec->sh.sh_flags = SHF_ALLOC | SHF_WRITE;
 
 	/* create .rela.kpatch.dynrelas*/
 
@@ -1734,7 +1734,11 @@ void kpatch_create_dynamic_rela_sections(struct kpatch_elf *kelf,
 			          rela->sym->name, result.value, result.size);
 
 			/* dest filed in by rela entry below */
-			dynrelas[index].src = result.value;
+			if (!strcmp(objname, "vmlinux"))
+				dynrelas[index].src = result.value;
+			else
+				/* for modules, src is discovered at runtime */
+				dynrelas[index].src = 0;
 			dynrelas[index].addend = rela->addend;
 			dynrelas[index].type = rela->type;