Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752835AbdLRNXs (ORCPT ); Mon, 18 Dec 2017 08:23:48 -0500 Received: from mx2.suse.de ([195.135.220.15]:48838 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759164AbdLRNXm (ORCPT ); Mon, 18 Dec 2017 08:23:42 -0500 Date: Mon, 18 Dec 2017 14:23:40 +0100 (CET) From: Miroslav Benes To: Jason Baron cc: jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, pmladek@suse.com, lpechacek@suse.cz, pavel@ucw.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4.1 2/2] livepatch: force transition to finish In-Reply-To: <70454f58-3d33-f305-e1df-5f9dc43222c7@akamai.com> Message-ID: References: <20171122102921.12289-1-mbenes@suse.cz> <70454f58-3d33-f305-e1df-5f9dc43222c7@akamai.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4245 Lines: 109 On Fri, 15 Dec 2017, Jason Baron wrote: > On 11/22/2017 05:29 AM, Miroslav Benes wrote: > > If a task sleeps in a set of patched functions uninterruptedly, it could > > block the whole transition indefinitely. Thus it may be useful to clear > > its TIF_PATCH_PENDING to allow the process to finish. > > > > Admin can do that now by writing to force sysfs attribute in livepatch > > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the > > transition can finish successfully. > > > > Important note! Administrator should not use this feature without a > > clearance from a patch distributor. It must be checked that by doing so > > the consistency model guarantees are not violated. Removal (rmmod) of > > patch modules is permanently disabled when the feature is used. It > > cannot be guaranteed there is no task sleeping in such module. > > > > Signed-off-by: Miroslav Benes > > Acked-by: Josh Poimboeuf > > Reviewed-by: Petr Mladek > > --- > > Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++ > > Documentation/livepatch/livepatch.txt | 18 ++++++++++-- > > kernel/livepatch/core.c | 30 ++++++++++++++++++++ > > kernel/livepatch/transition.c | 35 +++++++++++++++++++++++- > > kernel/livepatch/transition.h | 1 + > > 5 files changed, 95 insertions(+), 3 deletions(-) > > .... > > > + > > +/* > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an > > + * existing transition to finish. > > + * > > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or > > + * 'current'. This is not the case here and the consistency model could be > > + * broken. Administrator, who is the only one to execute the > > + * klp_force_transitions(), has to be aware of this. > > + */ > > +void klp_force_transition(void) > > +{ > > + struct task_struct *g, *task; > > + unsigned int cpu; > > + > > + pr_warn("forcing remaining tasks to the patched state\n"); > > + > > + read_lock(&tasklist_lock); > > + for_each_process_thread(g, task) > > + klp_update_patch_state(task); > > + read_unlock(&tasklist_lock); > > + > > + for_each_possible_cpu(cpu) > > + klp_update_patch_state(idle_task(cpu)); > > + > > + klp_forced = true; > > +} > > I had a question on this bit. If say cpu 0 executes > klp_force_transition(void), right up until klp_forced is set to true, > and then cpu 1 does klp_complete_transition() (since all threads have > the correct state), wouldn't it be possible then for > klp_complete_transition() to not see klp_forced set to true, and thus > the module could be potentially removed even though it was forced? Yes, you're right. That could happen. > If so, I think that the force path just needs to be set before the > threads are updated (as below). I don't think that the > klp_complete_transition() needs the corresponding rmb, b/c there is > sufficient ordering there already (although it would deserve a comment). Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be done with it once and for all. The problem is exactly what Petr predicted and I refused to have klp_mutex here just because it may have fixed theoretical issue. Petr, Josh, what do you think? Miroslav > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index be5bfa5..cca6a3a 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -671,6 +671,15 @@ void klp_force_transition(void) > > pr_warn("forcing remaining tasks to the patched state\n"); > > + klp_forced = true; > + > + /* > + * ensure that if klp_complete_transition() sees that all > + * the threads have been updated to desired task->patch_state > + * that we also see klp_forced = true; > + */ > + smp_wmb(); > + > read_lock(&tasklist_lock); > for_each_process_thread(g, task) > klp_update_patch_state(task); > @@ -678,6 +687,4 @@ void klp_force_transition(void) > > for_each_possible_cpu(cpu) > klp_update_patch_state(idle_task(cpu)); > - > - klp_forced = true; > } >