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
--
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
