Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

[Josh Poimboeuf]

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