by Josh Poimboeuf

[permalink] [raw]

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

On Wed, Aug 30, 2017 at 09:27:16AM -0400, Joe Lawrence wrote:
> > So instead of 'obj->pre_patch_callback_done', how about
> > 'obj->callbacks_enabled'?
> >
> > It could be set in the following cases:
> >
> > a) if the object has a pre_patch callback, set obj->callbacks_enabled
> > after the pre_patch callback succeeds;
> >
> > b) else, if the patch does *not* have a pre_patch callback, set
> > obj->callbacks_enabled after klp_patch_object() succeeds.
> >
> > And the variable would need to be cleared after the post_unpatch
> > callback was run.
> >
> > It's a bit complicated, but that seems to be the most logicial behavior
> > as far as I can tell.
> >
> > Thoughts?
>
> What if we flip it around as "callbacks_disabled"? By default, kzalloc
> would init as false. It would only be set to true if the pre-patch
> callback is provided and if it returns failure. Would that reduce the
> number of conditions when we need to set this var?

Yeah, 'callbacks_disabled' sounds better.

> Also, as you noted, I think it would need to reset/cleared after the
> post-patch callback. (For the livepatch-already-loaded cases.)

Since it can only be set when the pre-patch fails, I think it would only
need to be cleared after post-unpatch?

Or another alternative would be to unconditionally clear and/or set it
in klp_pre_patch_callback() so that its previous value doesn't matter.

--
Josh

2017-08-30 14:48:52

by Josh Poimboeuf

[permalink] [raw]

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