From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970139AbdIZTB4 (ORCPT ); Tue, 26 Sep 2017 15:01:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29727 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966175AbdIZTBy (ORCPT ); Tue, 26 Sep 2017 15:01:54 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2F8D113A5D Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=joe.lawrence@redhat.com Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks To: Petr Mladek Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Miroslav Benes , Chris J Arges References: <1504191233-2642-1-git-send-email-joe.lawrence@redhat.com> <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com> <20170926144912.GH21048@pathway.suse.cz> From: Joe Lawrence Organization: Red Hat Message-ID: <85d3cd2e-f51f-621d-70c7-df602fb5004b@redhat.com> Date: Tue, 26 Sep 2017 15:01:52 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170926144912.GH21048@pathway.suse.cz> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 26 Sep 2017 19:01:54 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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