On 09/26/2017 10:49 AM, Petr Mladek wrote:
> 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.
Are you talking about the error path? As its coded here,
klp_cancel_transition() will call klp_complete_transition() with
klp_target_state = KLP_UNPATCHED and then klp_complete_transition()'s
done: code will call klp_post_unpatch_callback() on all the necessary
kobj's. Is there something asynchronous about that?
> Well, what about calling:
>
> klp_pre_patch_callback() inside klp_patch_object() and
> klp_post_unpatch_callback() inside klp_unpatch_object()
v1 started out that way, but we migrated to placing these around the
callers of klp_(un)patch_object() to try and better line up the
locations of the pre- hooks with the post- hook locations.
I can take a second look at reversing this decision, but that may take a
little time while I page all the testing corner cases back into my brain :)
> 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
Indeed, it would be nice to ditch that callbacks_enabled wart.
> 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.
I think the only other outstanding issue before rolling a v6 is the one
that Miroslav raised about the error path in klp_module_coming():
https://marc.info/?l=linux-kernel&m=150590635602784&w=2
https://marc.info/?l=linux-kernel&m=150592065007463&w=2
Thanks,
-- Joe
