Messages in this thread |  | | Date | Wed, 30 Aug 2017 09:48:44 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks |
| |
On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote: > @@ -871,6 +882,13 @@ int klp_module_coming(struct module *mod) > pr_notice("applying patch '%s' to loading module '%s'\n", > patch->mod->name, obj->mod->name); > > + ret = klp_pre_patch_callback(obj); > + if (ret) { > + pr_warn("pre-patch callback failed for object '%s'\n", > + obj->name); > + goto err; > + } > + > ret = klp_patch_object(obj); > if (ret) { > pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
If klp_pre_patch_callback() succeeds but klp_patch_object() fails, the post-unpatch callback needs to be called in the error path.
> +/** > + * klp_pre_patch_callback - executed before klp_object is patched > + * @obj: invoke callback for this klp_object > + * > + * Return: status from callback > + * > + * Callers should ensure obj->patched is *not* set.
Can this comment be removed since it no longer checks obj->patched?
> +static inline int klp_pre_patch_callback(struct klp_object *obj) > +{ > + obj->pre_patch_callback_status = > + (obj->callbacks.pre_patch) ? > + (*obj->callbacks.pre_patch)(obj) : 0; > + > + return obj->pre_patch_callback_status; > +} > + > +/** > + * klp_post_patch_callback() - executed after klp_object is patched > + * @obj: invoke callback for this klp_object > + * > + * Callers should ensure obj->patched is set.
Ditto here and below.
> +static inline void klp_post_patch_callback(struct klp_object *obj) > +{ > + if (obj->callbacks.post_patch) > + (*obj->callbacks.post_patch)(obj); > +} > + > +/** > + * klp_pre_unpatch_callback() - executed before klp_object is unpatched > + * and is active across all tasks > + * @obj: invoke callback for this klp_object > + * > + * This callback will not be run if the pre-patch callback status was > + * non-zero.
I think this comment should be a little broader. The callback won't be called if the object fails to patch for *any* reason.
Also, I think all the comments about when the callbacks are run or not run would be better placed in a paragraph above the 'struct klp_callbacks' definition in include/linux/livepatch.h.
And actually, IMO, all the functions in core.h are straightforward enough that they don't need *any* function header comments. And the same for klp_is_object_loaded(). I would just remove all the core.h comments, and just add a short explanation in livepatch.h about when the callbacks are called and not called.
> + * Callers should ensure obj->patched is set. > + */ > +static inline void klp_pre_unpatch_callback(struct klp_object *obj) > +{ > + if (!obj->pre_patch_callback_status && > + obj->callbacks.pre_unpatch) > + (*obj->callbacks.pre_unpatch)(obj); > +}
I think pre_patch_callback_status (or callbacks_disabled) doesn't need to be checked here, right? Since if we got this far, the patching was successful and callbacks will be enabled by definition?
-- Josh
|  |