safe kpatch unload

Currently the patch module calls kpatch_unregister in the patch module
exit path.  If the activeness safety check fails in kpatch_unregister,
it's too late for the patch module to stop exiting, so all it can do is
panic.

Prevent this scenario by requiring the user to disable the patch module
via sysfs before allowing the module to be unloaded.  The sysfs write
will fail if the activeness safety check fails.  An rmmod will fail if
the patch is still enabled.

Also add support for this new unloading model in "kpatch unload".

README.md

@@ -259,7 +259,8 @@ kpatch needs gcc >= 4.6 and Linux >= 3.7 for use of the -mfentry flag.
 
 **Q. Is it possible to remove a patch?**
 
-Yes.  Just unload the patch module and the original function will be restored.
+Yes.  Just run `kpatch unload` which will disable and unload the patch module
+and restore the function to its original state.
 
 **Q. Can you apply multiple patches?**
 

kmod/core/core.c

@@ -53,6 +53,10 @@ DEFINE_SEMAPHORE(kpatch_mutex);
 
 static int kpatch_num_registered;
 
+static struct kobject *kpatch_root_kobj;
+struct kobject *kpatch_patches_kobj;
+EXPORT_SYMBOL_GPL(kpatch_patches_kobj);
+
 struct kpatch_backtrace_args {
 	struct kpatch_module *kpmod;
 	int ret;
@@ -354,8 +358,15 @@ int kpatch_register(struct kpatch_module *kpmod)
 	if (!kpmod->mod || !funcs || !num_funcs)
 		return -EINVAL;
 
+	kpmod->enabled = false;
+
 	down(&kpatch_mutex);
 
+	if (!try_module_get(kpmod->mod)) {
+		ret = -ENODEV;
+		goto err_up;
+	}
+
 	for (i = 0; i < num_funcs; i++) {
 		struct kpatch_func *func = &funcs[i];
 
@@ -418,6 +429,8 @@ int kpatch_register(struct kpatch_module *kpmod)
 	pr_notice("loaded patch module \"%s\"\n", kpmod->mod->name);
 
 	atomic_set(&kpatch_operation, KPATCH_OP_NONE);
+	kpmod->enabled = true;
+
 	up(&kpatch_mutex);
 	return 0;
 
@@ -433,6 +446,8 @@ err_unregister:
 	kpatch_num_registered--;
 err_rollback:
 	kpatch_remove_funcs_from_filter(funcs, num_funcs);
+	module_put(kpmod->mod);
+err_up:
 	up(&kpatch_mutex);
 	return ret;
 }
@@ -444,6 +459,8 @@ int kpatch_unregister(struct kpatch_module *kpmod)
 	int num_funcs = kpmod->num_funcs;
 	int i, ret;
 
+	WARN_ON(!kpmod->enabled);
+
 	down(&kpatch_mutex);
 
 	/* Start unpatching operation */
@@ -483,6 +500,9 @@ int kpatch_unregister(struct kpatch_module *kpmod)
 
 	pr_notice("unloaded patch module \"%s\"\n", kpmod->mod->name);
 
+	kpmod->enabled = false;
+	module_put(kpmod->mod);
+
 out:
 	atomic_set(&kpatch_operation, KPATCH_OP_NONE);
 	up(&kpatch_mutex);
@@ -490,4 +510,26 @@ out:
 }
 EXPORT_SYMBOL(kpatch_unregister);
 
+static int kpatch_init(void)
+{
+	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)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void kpatch_exit(void)
+{
+	kobject_put(kpatch_patches_kobj);
+	kobject_put(kpatch_root_kobj);
+}
+
+module_init(kpatch_init);
+module_exit(kpatch_exit);
 MODULE_LICENSE("GPL");

kmod/core/kpatch.h

@@ -40,8 +40,12 @@ struct kpatch_module {
 	struct module *mod;
 	struct kpatch_func *funcs;
 	int num_funcs;
+
+	bool enabled;
 };
 
+extern struct kobject *kpatch_patches_kobj;
+
 extern int kpatch_register(struct kpatch_module *kpmod);
 extern int kpatch_unregister(struct kpatch_module *kpmod);
 

kmod/patch/kpatch-patch-hook.c

@@ -28,6 +28,43 @@
 extern char __kpatch_patches, __kpatch_patches_end;
 
 static struct kpatch_module kpmod;
+static struct kobject *patch_kobj;
+
+static ssize_t patch_enabled_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", kpmod.enabled);
+}
+
+static ssize_t patch_enabled_store(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf,
+				   size_t count)
+{
+	int ret;
+	unsigned long val;
+
+	/* only disabling is supported */
+	if (!kpmod.enabled)
+		return -EINVAL;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	val = !!val;
+
+	/* only disabling is supported */
+	if (val)
+		return -EINVAL;
+
+	ret = kpatch_unregister(&kpmod);
+	if (ret)
+		return ret;
+
+	return count;
+}
+static struct kobj_attribute patch_enabled_attr =
+	__ATTR(enabled, 0644, patch_enabled_show, patch_enabled_store);
 
 static int __init patch_init(void)
 {
@@ -51,12 +88,27 @@ static int __init patch_init(void)
 		kpmod.funcs[i].new_size = patches[i].new_size;
 	}
 
+	patch_kobj = kobject_create_and_add(THIS_MODULE->name,
+					    kpatch_patches_kobj);
+	if (!patch_kobj) {
+		ret = -ENOMEM;
+		goto err_free;
+	}
+
+	ret = sysfs_create_file(patch_kobj, &patch_enabled_attr.attr);
+	if (ret)
+		goto err_put;
+
 	ret = kpatch_register(&kpmod);
 	if (ret)
-		goto err_free;
+		goto err_sysfs;
 
 	return 0;
 
+err_sysfs:
+	sysfs_remove_file(patch_kobj, &patch_enabled_attr.attr);
+err_put:
+	kobject_put(patch_kobj);
 err_free:
 	kfree(kpmod.funcs);
 	return ret;
@@ -64,22 +116,9 @@ err_free:
 
 static void __exit patch_exit(void)
 {
-	int ret;
-
-	ret = kpatch_unregister(&kpmod);
-	if (ret) {
-		/*
-		 * TODO: If this happens, we're screwed.  We need a way to
-		 * prevent the module from unloading if the activeness safety
-		 * check fails.
-		 *
-		 * Or alternatively we could keep trying the activeness safety
-		 * check in a loop, until it works or we timeout.  Then we
-		 * could panic.
-		 */
-		panic("kpatch_unregister failed: %d", ret);
-	}
-
+	WARN_ON(kpmod.enabled);
+	sysfs_remove_file(patch_kobj, &patch_enabled_attr.attr);
+	kobject_put(patch_kobj);
 	kfree(kpmod.funcs);
 }
 

kpatch-build/link-vmlinux-syms.c

@@ -237,7 +237,8 @@ int main(int argc, char **argv)
 		    GELF_ST_BIND(cur->sym.st_info) != STB_GLOBAL ||
 		    cur->sym.st_shndx != STN_UNDEF ||
 		    !strcmp(cur->name, "kpatch_register") ||
-		    !strcmp(cur->name, "kpatch_unregister"))
+		    !strcmp(cur->name, "kpatch_unregister") ||
+		    !strcmp(cur->name, "kpatch_patches_kobj"))
 			continue;
 
 		printf("found global symbol %s\n", cur->name);

kpatch/kpatch

@@ -100,6 +100,19 @@ load_module () {
 }
 
 unload_module () {
+	# strip leading path, .ko suffix and convert '-' to '_' so we can find
+	# the module name in sysfs
+	PATCH="$(basename $1)"
+	PATCH="${PATCH%.ko}"
+	PATCH="${PATCH//-/_}"
+
+	ENABLED=/sys/kernel/kpatch/patches/"$PATCH"/enabled
+
+	if [[ -e "$ENABLED" ]] && [[ $(cat "$ENABLED") -eq 1 ]]; then
+		echo "disabling patch module: $1"
+		echo 0 > $ENABLED || die "can't disable $PATCH"
+	fi
+
 	echo "unloading patch module: $1"
 	/usr/sbin/rmmod "$(basename $1)"
 }
@@ -135,9 +148,7 @@ case "$1" in
 
 "unload")
 	[[ "$#" -ne 2 ]] && usage
-	PATCH="$2"
-	find_module "$PATCH" || die "can't find $PATCH"
-	unload_module "$MODULE" || die "failed to unload patch $PATCH"
+	unload_module "$2" || die "failed to unload patch $2"
 	;;
 
 "install")