From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966089AbdEWR2C (ORCPT ); Tue, 23 May 2017 13:28:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59922 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965708AbdEWR16 (ORCPT ); Tue, 23 May 2017 13:27:58 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 383143B54C Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 383143B54C Date: Tue, 23 May 2017 12:27:57 -0500 From: Josh Poimboeuf To: Miroslav Benes Cc: jeyu@redhat.com, jikos@kernel.org, pmladek@suse.com, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] livepatch: force transition process to finish Message-ID: <20170523172757.mo4piel6i5svs2ux@treble> References: <20170518120043.7205-1-mbenes@suse.cz> <20170518120043.7205-4-mbenes@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170518120043.7205-4-mbenes@suse.cz> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 23 May 2017 17:27:58 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 18, 2017 at 02:00:43PM +0200, Miroslav Benes wrote: > If a task sleeps in a set of patched functions uninterruptibly, it could > block the whole transition process 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 2 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! Use wisely. Admin must be sure that it is safe to > execute such action. This means that it must be checked that by doing so > the consistency model guarantees are not violated. > > Signed-off-by: Miroslav Benes These patches look good to me. Just some minor comments. > --- > include/linux/livepatch.h | 1 + > kernel/livepatch/core.c | 3 +++ > kernel/livepatch/transition.c | 16 ++++++++++++++++ > kernel/livepatch/transition.h | 1 + > 4 files changed, 21 insertions(+) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 43cfeebeb42b..b567208a1c6e 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -31,6 +31,7 @@ > > /* values for sysfs force attribute */ > #define KLP_FORCE_FAKE 1 > +#define KLP_FORCE_UNMARK 2 > > /* task patch states */ > #define KLP_UNDEFINED -1 > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bb3b78fa7d2b..9bc1103348c9 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr, > case KLP_FORCE_FAKE: > klp_send_fake_signal(); > break; > + case KLP_FORCE_UNMARK: > + klp_unmark_tasks(); > + break; I think the naming could be a little clearer, and more consistent. What do you think about: KLP_FORCE_SIGNALS -> klp_force_signals() KLP_FORCE_TRANSITIONS -> klp_force_transitions() > default: > return -EINVAL; > } > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index bb61aaa196d3..d057a34510e6 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -591,3 +591,19 @@ void klp_send_fake_signal(void) > } > read_unlock(&tasklist_lock); > } > + > +/* > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an > + * existing transition to finish. > + */ > +void klp_unmark_tasks(void) > +{ > + struct task_struct *g, *task; > + > + pr_warn("all tasks marked as migrated on admin's request\n"); The user might not know what migrated means. How about "forcing remaining tasks to the patched state" or something similar? > + > + read_lock(&tasklist_lock); > + for_each_process_thread(g, task) > + klp_update_patch_state(task); > + read_unlock(&tasklist_lock); So klp_update_patch_state() has the following comment: * NOTE: If task is not 'current', the caller must ensure the task is inactive. * Otherwise klp_ftrace_handler() might read the wrong 'patch_state' value. This code doesn't ensure the task is inactive. But I think that's ok as long as we document the fact that this could break the consistency model, right? On a related note, I think the new sysfs entry should also be documented in Documentation/livepatch/livepatch.txt somewhere. -- Josh