On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote: > +++ b/Documentation/livepatch/callbacks.txt > @@ -0,0 +1,75 @@ > +(Un)patching Callbacks > +====================== > + > +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules > +to execute callback functions when a kernel object is (un)patched. I think it would be helpful to put a little blurb here about why callbacks are needed and when they might be used. Maybe steal some of the description from the first two bullet points here: https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble Also, I tested stop_machine() in the callbacks and it seemed to work fine. It might be worth mentioning in the docs that it's an option. > + > +These callbacks differ from existing kernel facilities: > + > + - Module init/exit code doesn't run when disabling and re-enabling a > + patch. > + > + - A module notifier can't stop the to-be-patched module from loading. > + > +Callbacks are part of the klp_object structure and their implementation > +is specific to the given object. Other livepatch objects may or may not > +be patched, irrespective of the target klp_object's current state. > + > +Callbacks can be registered for the following livepatch actions: > + > + * Pre-patch - before klp_object is patched > + > + * Post-patch - after klp_object has been patched and is active > + across all tasks > + > + * Pre-unpatch - before klp_object is unpatched, patched code is active > + > + * Post-unpatch - after klp_object has been patched, all code has been > + restored and no tasks are running patched code > + > +Callbacks are only executed if its host klp_object is loaded. For "Callbacks are" -> "A callback is" ? > +static inline int klp_pre_patch_callback(struct klp_object *obj) > +{ > + if (!obj->patched && obj->callbacks.pre_patch) > + return (*obj->callbacks.pre_patch)(obj); > + return 0; > +} > +static inline void klp_post_patch_callback(struct klp_object *obj) > +{ > + if (obj->patched && obj->callbacks.post_patch) > + (*obj->callbacks.post_patch)(obj); > +} > +static inline void klp_pre_unpatch_callback(struct klp_object *obj) > +{ > + if (obj->patched && obj->callbacks.pre_unpatch) > + (*obj->callbacks.pre_unpatch)(obj); > +} > +static inline void klp_post_unpatch_callback(struct klp_object *obj) > +{ > + if (!obj->patched && obj->callbacks.post_unpatch) > + (*obj->callbacks.post_unpatch)(obj); > +} > + Do these need the obj->patched checks? As far as I can tell they seem to be called in the right places and the checks are superfluous. > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -109,9 +109,6 @@ static void klp_complete_transition(void) > } > } > > - if (klp_target_state == KLP_UNPATCHED && !immediate_func) > - module_put(klp_transition_patch->mod); > - > /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */ > if (klp_target_state == KLP_PATCHED) > klp_synchronize_transition(); > @@ -130,6 +127,22 @@ static void klp_complete_transition(void) > } > > done: > + klp_for_each_object(klp_transition_patch, obj) { > + if (klp_target_state == KLP_PATCHED) > + klp_post_patch_callback(obj); > + else if (klp_target_state == KLP_PATCHED) s/KLP_PATCHED/KLP_UNPATCHED > + klp_post_unpatch_callback(obj); > + } > + > + /* > + * See complementary comment in __klp_enable_patch() for why we > + * keep the module reference for immediate patches. > + */ > + if (!klp_transition_patch->immediate) { > + if (klp_target_state == KLP_UNPATCHED && !immediate_func) > + module_put(klp_transition_patch->mod); > + } > + Maybe combine these into a single 'if' for clarity: if (klp_target_state == KLP_UNPATCHED && !immediate_func && !klp_transition_patch->immediate) module_put(klp_transition_patch->mod); > + * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch > + * callback return status. Try setting up a non-zero status > + * such as -19 (-ENODEV): > + * > + * # Load demo livepatch, vmlinux is patched > + * insmod samples/livepatch/livepatch-callbacks-demo.ko > + * > + * # Setup next pre-patch callback to return -ENODEV > + * echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret Git complained about trailing whitespace here ^ > + * > + * # Module loader refuses to load the target module > + * insmod samples/livepatch/livepatch-callbacks-mod.ko and here ^ > +/* Executed on object unpatching (ie, patch disablement) */ > +static void post_patch_callback(struct klp_object *obj) s/unpatching/patching/ -- Josh -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
