On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..aca62c4b8616 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
> -static bool klp_is_object_loaded(struct klp_object *obj)
> -{
> - return !obj->name || obj->mod;
> -}
> -
> /* sets obj->mod if object is not vmlinux and module is found */
> static void klp_find_object_module(struct klp_object *obj)
> {
> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module
> *pmod,
>
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> + struct klp_object *obj;
> +
> if (klp_transition_patch)
> return -EBUSY;
>
> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>
> klp_init_transition(patch, KLP_UNPATCHED);
>
> + klp_for_each_object(patch, obj)
> + if (patch->enabled && obj->patched)
> + klp_pre_unpatch_callback(obj);
> +
> /*
> * Enforce the order of the func->transition writes in
> * klp_init_transition() and the TIF_PATCH_PENDING writes in
> @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (!klp_is_object_loaded(obj))
> continue;
>
> - ret = klp_patch_object(obj);
> + ret = klp_pre_patch_callback(obj);
> if (ret) {
> - pr_warn("failed to enable patch '%s'\n",
> - patch->mod->name);
> + pr_warn("pre-patch callback failed for object '%s'\n",
> + klp_is_module(obj) ? obj->name : "vmlinux");
> + goto err;
> + }
>
> - klp_cancel_transition();
> - return ret;
> + ret = klp_patch_object(obj);
> + if (ret) {
> + pr_warn("failed to patch object '%s'\n",
> + klp_is_module(obj) ? obj->name : "vmlinux");
We should call klp_post_unpatch_callback(obj) here to make it
synchronous.
Well, what about calling:
klp_pre_patch_callback() inside klp_patch_object() and
klp_post_unpatch_callback() inside klp_unpatch_object()
By other words, we would do the two operations. It would have
two advantages:
+ error handling for free
+ no need for the strange callbacks_enabled flag
It would require the more strict consistency model if there
is a dependency between the callbacks and patches from various
modules. But we would probably need the consistency model
in this case anyway.
> + goto err;
> }
> }
>
Otherwise I think that we are getting close.
Best Regards,
Petr
PS: I hope that the above problem and solution has not been mentioned
yet. I am sorry if it was. I am a bit lost in many mails after
vacation, sickness, and conference.
