fixup review comments

Signed-off-by: Seth Jennings <sjenning@redhat.com>

kmod/core/core.c

@@ -341,10 +341,8 @@ static int kpatch_apply_patch(void *data)
 	list_for_each_entry(object, &kpmod->objects, list) {
 		if (!kpatch_object_linked(object))
 			continue;
-		list_for_each_entry(hook, &object->hooks_load, list) {
-			/* TODO: ignoring return code for now */
+		list_for_each_entry(hook, &object->hooks_load, list)
 			(*hook->hook)();
-		}
 	}
 
 
@@ -380,10 +378,8 @@ static int kpatch_remove_patch(void *data)
 	list_for_each_entry(object, &kpmod->objects, list) {
 		if (!kpatch_object_linked(object))
 			continue;
-		list_for_each_entry(hook, &object->hooks_unload, list) {
-			/* TODO: ignoring return code for now */
+		list_for_each_entry(hook, &object->hooks_unload, list)
 			(*hook->hook)();
-		}
 	}
 
 	return 0;
@@ -745,6 +741,7 @@ static int kpatch_module_notify(struct notifier_block *nb, unsigned long action,
 	struct kpatch_module *kpmod;
 	struct kpatch_object *object;
 	struct kpatch_func *func;
+	struct kpatch_hook *hook;
 	int ret = 0;
 	bool found = false;
 
@@ -775,6 +772,10 @@ done:
 
 	pr_notice("patching newly loaded module '%s'\n", object->name);
 
+	/* run any user-defined load hooks */
+	list_for_each_entry(hook, &object->hooks_load, list)
+		(*hook->hook)();
+
 	/* add to the global func hash */
 	list_for_each_entry(func, &object->funcs, list)
 		hash_add_rcu(kpatch_func_hash, &func->node, func->old_addr);

kmod/core/kpatch.h

@@ -59,7 +59,7 @@ struct kpatch_dynrela {
 
 struct kpatch_hook {
 	struct list_head list;
-	int (*hook)(void);
+	void (*hook)(void);
 };
 
 struct kpatch_object {

kmod/patch/kpatch-macros.h

@@ -0,0 +1,54 @@
+#ifndef __KPATCH_MACROS_H_
+#define __KPATCH_MACROS_H_
+
+#include <linux/compiler.h>
+
+typedef void (*kpatch_loadcall_t)(void);
+typedef void (*kpatch_unloadcall_t)(void);
+
+struct kpatch_load {
+	kpatch_loadcall_t fn;
+	char *objname; /* filled in by create-diff-object */
+};
+
+struct kpatch_unload {
+	kpatch_unloadcall_t fn;
+	char *objname; /* filled in by create-diff-object */
+};
+
+/*
+ * KPATCH_LOAD_HOOK macro
+ *
+ * The first line only ensures that the hook being registered has the required
+ * function signature.  If not, there is compile error on this line.
+ *
+ * The section line declares a struct kpatch_load to be allocated in a new
+ * .kpatch.hook.load section.  This kpatch_load_data symbol is later stripped
+ * by create-diff-object so that it can be declared in multiple objects that
+ * are later linked together, avoiding global symbol collision.  Since multiple
+ * hooks can be registered, the .kpatch.hook.load section is a table of struct
+ * kpatch_load elements that will be executed in series by the kpatch core
+ * module at load time, assuming the kernel object (module) is currently
+ * loaded; otherwise, the hook is called when module to be patched is loaded
+ * via the module load notifier.
+ */
+#define KPATCH_LOAD_HOOK(_fn) \
+	static inline kpatch_loadcall_t __loadtest(void) { return _fn; } \
+	struct kpatch_load kpatch_load_data __section(.kpatch.hooks.load) = { \
+		.fn = _fn, \
+		.objname = NULL \
+	};
+
+/*
+ * KPATCH_UNLOAD_HOOK
+ *
+ * Same as LOAD hook with s/load/unload/
+ */
+#define KPATCH_UNLOAD_HOOK(_fn) \
+	static inline kpatch_unloadcall_t __unloadtest(void) { return _fn; } \
+	struct kpatch_unload kpatch_unload_data __section(.kpatch.hooks.unload) = { \
+		.fn = _fn, \
+		.objname = NULL \
+	};
+
+#endif /* __KPATCH_HOOKS_H_ */

kmod/patch/kpatch-patch.h

@@ -42,7 +42,7 @@ struct kpatch_patch_dynrela {
 };
 
 struct kpatch_patch_hook {
-	int (*hook)(void);
+	void (*hook)(void);
 	char *objname;
 };
 

kmod/patch/kpatch.lds

@@ -8,6 +8,13 @@ SECTIONS
     __kpatch_hooks_load = . ;
     *(.kpatch.hooks.load)
     __kpatch_hooks_load_end = . ;
+    /*
+     * Pad the end of the section with zeros in case the section is empty.
+     * This prevents the kernel from discarding the section at module
+     * load time. __kpatch_hooks_load_end will still point to the end of
+     * the section before the padding.  If the .kpatch.hooks.load section
+     * is empty, __kpatch_hooks_load equals __kpatch_hooks_load_end.
+     */
     QUAD(0);
   }
   .kpatch.hooks.unload : {

kpatch-build/create-diff-object.c

@@ -1069,7 +1069,6 @@ int is_null_sym(struct symbol *sym)
 
 int is_file_sym(struct symbol *sym)
 {
-	log_debug("here!");
 	return sym->type == STT_FILE;
 }
 

kpatch-build/kpatch-build

@@ -75,6 +75,7 @@ cleanup() {
 		[[ -e $TEMPDIR/.config ]] && cp -f $TEMPDIR/.config $USERSRCDIR
 	fi
 	[[ "$DEBUG" -eq 0 ]] && rm -rf "$TEMPDIR"
+	unset KCFLAGS
 }
 
 clean_cache() {
@@ -341,12 +342,12 @@ echo "Testing patch file"
 cd "$SRCDIR" || die
 patch -N -p1 --dry-run < "$PATCHFILE" || die "source patch file failed to apply"
 cp "$PATCHFILE" "$APPLIEDPATCHFILE" || die
-cp "$SCRIPTDIR/kpatch-hooks.h" "$SRCDIR/include/linux"
+cp -LR "$DATADIR/patch" "$TEMPDIR" || die
+export KCFLAGS="-I$DATADIR/patch" # for kpatch-macros.h
 
 echo "Building original kernel"
 make mrproper >> "$LOGFILE" 2>&1 || die
 make "-j$CPUS" $TARGETS "O=$OBJDIR" >> "$LOGFILE" 2>&1 || die
-cp -LR "$DATADIR/patch" "$TEMPDIR" || die
 
 echo "Building patched kernel"
 patch -N -p1 < "$APPLIEDPATCHFILE" >> "$LOGFILE" 2>&1 || die
@@ -377,8 +378,9 @@ rm -rf "$OBJDIR2"
 mkdir -p "$OBJDIR2"
 cp "$OBJDIR/.config" "$OBJDIR2" || die
 mkdir "$TEMPDIR/patched"
+export KCFLAGS="$KCFLAGS -ffunction-sections -fdata-sections"
 for i in $(cat $TEMPDIR/changed_objs); do
-	KCFLAGS="-ffunction-sections -fdata-sections" make "$i" "O=$OBJDIR2" >> "$LOGFILE" 2>&1 || die
+	make "$i" "O=$OBJDIR2" >> "$LOGFILE" 2>&1 || die
 	mkdir -p "$TEMPDIR/patched/$(dirname $i)"
 	cp -f "$OBJDIR2/$i" "$TEMPDIR/patched/$i" || die
 done
@@ -387,7 +389,7 @@ rm -f "$APPLIEDPATCHFILE"
 mkdir "$TEMPDIR/orig"
 for i in $(cat $TEMPDIR/changed_objs); do
 	rm -f "$i"
-	KCFLAGS="-ffunction-sections -fdata-sections" make "$i" "O=$OBJDIR2" >> "$LOGFILE" 2>&1 || die
+	make "$i" "O=$OBJDIR2" >> "$LOGFILE" 2>&1 || die
 	mkdir -p "$TEMPDIR/orig/$(dirname $i)"
 	cp -f "$OBJDIR2/$i" "$TEMPDIR/orig/$i" || die
 done

kpatch-build/kpatch-hooks.h

@@ -1,33 +0,0 @@
-#ifndef __KPATCH_HOOKS_H_
-#define __KPATCH_HOOKS_H_
-
-#include <linux/compiler.h>
-
-typedef int (*kpatch_loadcall_t)(void);
-typedef int (*kpatch_unloadcall_t)(void);
-
-struct kpatch_load {
-	kpatch_loadcall_t fn;
-	char *objname; /* filled in by create-diff-object */
-};
-
-struct kpatch_unload {
-	kpatch_unloadcall_t fn;
-	char *objname; /* filled in by create-diff-object */
-};
-
-#define kpatch_load_hook(_fn) \
-	static inline kpatch_loadcall_t __loadtest(void) { return _fn; } \
-	struct kpatch_load kpatch_load_data __section(.kpatch.hooks.load) = { \
-		.fn = _fn, \
-		.objname = NULL \
-	};
-
-#define kpatch_unload_hook(_fn) \
-	static inline kpatch_unloadcall_t __unloadtest(void) { return _fn; } \
-	struct kpatch_unload kpatch_unload_data __section(.kpatch.hooks.unload) = { \
-		.fn = _fn, \
-		.objname = NULL \
-	};
-
-#endif /* __KPATCH_HOOKS_H_ */