From patchwork Thu Feb 16 20:31:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Josh Poimboeuf X-Patchwork-Id: 728902 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vPSXC1bL3z9s7D for ; Fri, 17 Feb 2017 07:32:59 +1100 (AEDT) Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3vPSXC0pXTzDqCr for ; Fri, 17 Feb 2017 07:32:59 +1100 (AEDT) X-Original-To: linuxppc-dev@lists.ozlabs.org Delivered-To: linuxppc-dev@lists.ozlabs.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3vPSW65v8rzDq60 for ; Fri, 17 Feb 2017 07:32:02 +1100 (AEDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 279423D966; Thu, 16 Feb 2017 20:32:00 +0000 (UTC) Received: from treble (ovpn-121-112.rdu2.redhat.com [10.10.121.112] (may be forged)) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id v1GKVvms030306; Thu, 16 Feb 2017 15:31:58 -0500 Date: Thu, 16 Feb 2017 14:31:56 -0600 From: Josh Poimboeuf To: Miroslav Benes Subject: Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model Message-ID: <20170216203156.igvctdmyt6b6zcgg@treble> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 16 Feb 2017 20:32:01 +0000 (UTC) X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Petr Mladek , x86@kernel.org, Jessica Yu , Vojtech Pavlik , linux-s390@vger.kernel.org, Peter Zijlstra , Jiri Kosina , Heiko Carstens , linux-kernel@vger.kernel.org, Kamalesh Babulal , Andy Lutomirski , live-patching@vger.kernel.org, Jiri Slaby , linuxppc-dev@lists.ozlabs.org, Ingo Molnar , Chris J Arges Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Feb 16, 2017 at 03:33:26PM +0100, Miroslav Benes wrote: > > > @@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch) > > > > pr_notice("enabling patch '%s'\n", patch->mod->name); > > > > + klp_init_transition(patch, KLP_PATCHED); > > + > > + /* > > + * Enforce the order of the func->transition writes in > > + * klp_init_transition() and the ops->func_stack writes in > > + * klp_patch_object(), so that klp_ftrace_handler() will see the > > + * func->transition updates before the handler is registered and the > > + * new funcs become visible to the handler. > > + */ > > + smp_wmb(); > > + > > klp_for_each_object(patch, obj) { > > if (!klp_is_object_loaded(obj)) > > continue; > > > > ret = klp_patch_object(obj); > > - if (ret) > > - goto unregister; > > + if (ret) { > > + pr_warn("failed to enable patch '%s'\n", > > + patch->mod->name); > > + > > + klp_cancel_transition(); > > + return ret; > > + } > > [...] > > > +static void klp_complete_transition(void) > > +{ > > + struct klp_object *obj; > > + struct klp_func *func; > > + struct task_struct *g, *task; > > + unsigned int cpu; > > + > > + if (klp_target_state == KLP_UNPATCHED) { > > + /* > > + * All tasks have transitioned to KLP_UNPATCHED so we can now > > + * remove the new functions from the func_stack. > > + */ > > + klp_unpatch_objects(klp_transition_patch); > > + > > + /* > > + * Make sure klp_ftrace_handler() can no longer see functions > > + * from this patch on the ops->func_stack. Otherwise, after > > + * func->transition gets cleared, the handler may choose a > > + * removed function. > > + */ > > + synchronize_rcu(); > > + } > > + > > + if (klp_transition_patch->immediate) > > + goto done; > > + > > + klp_for_each_object(klp_transition_patch, obj) > > + klp_for_each_func(obj, func) > > + func->transition = false; > > + > > + /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */ > > + if (klp_target_state == KLP_PATCHED) > > + synchronize_rcu(); > > + > > + read_lock(&tasklist_lock); > > + for_each_process_thread(g, task) { > > + WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING)); > > + task->patch_state = KLP_UNDEFINED; > > + } > > + read_unlock(&tasklist_lock); > > + > > + for_each_possible_cpu(cpu) { > > + task = idle_task(cpu); > > + WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING)); > > + task->patch_state = KLP_UNDEFINED; > > + } > > + > > +done: > > + klp_target_state = KLP_UNDEFINED; > > + klp_transition_patch = NULL; > > +} > > + > > +/* > > + * This is called in the error path, to cancel a transition before it has > > + * started, i.e. klp_init_transition() has been called but > > + * klp_start_transition() hasn't. If the transition *has* been started, > > + * klp_reverse_transition() should be used instead. > > + */ > > +void klp_cancel_transition(void) > > +{ > > + klp_target_state = !klp_target_state; > > + klp_complete_transition(); > > +} > > If we fail to enable patch in __klp_enable_patch(), we call > klp_cancel_transition() and get to klp_complete_transition(). If the patch > has immediate set to true, the module would not be allowed to go (the > changes are in the last patch unfortunately, but my remark is closely > related to klp_cancel_transition() and __klp_enable_patch()). This could > annoy a user if it was due to a trivial reason. So we could call > module_put() in this case. It should be safe as no task could be in a new > function thanks to klp_ftrace_handler() logic. > > Pity I have not spotted this earlier. > > Putting module_put(patch->mod) right after klp_cancel_transition() call in > __klp_enable_patch() would be the simplest fix (as a part of 15/15 patch). > Maybe with a comment that it is safe to do it there. > > What do you think? Good catch. I agree that 15/15 should have something like that. Also, the module_put() will be needed for non-immediate patches which have a func->immediate set. What do you think about the following? I tried to put the logic in klp_complete_transition(), so the module_put()'s would be in one place. But it was too messy, so I put it in klp_cancel_transition() instead. diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index e96346e..bd1c1fd 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -121,8 +121,28 @@ static void klp_complete_transition(void) */ void klp_cancel_transition(void) { + bool immediate_func = false; + klp_target_state = !klp_target_state; klp_complete_transition(); + + if (klp_target_state == KLP_PATCHED) + return; + + /* + * In the enable error path, even immediate patches can be safely + * removed because the transition hasn't been started yet. + * + * klp_complete_transition() doesn't have a module_put() for immediate + * patches, so do it here. + */ + klp_for_each_object(klp_transition_patch, obj) + klp_for_each_func(obj, func) + if (func->immediate) + immediate_func = true; + + if (klp_transition_patch->immediate || immediate_func) + module_put(klp_transition_patch->mod); } /*