Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932867AbcDENof (ORCPT <rfc822;w@1wt.eu>);
	Tue, 5 Apr 2016 09:44:35 -0400
Received: from mx1.redhat.com ([209.132.183.28]:46110 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1758992AbcDENoc (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 5 Apr 2016 09:44:32 -0400
Date: Tue, 5 Apr 2016 08:44:30 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Jiri Kosina <jikos@kernel.org>, Jessica Yu <jeyu@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>, linux-kernel@vger.kernel.org,
        live-patching@vger.kernel.org, Vojtech Pavlik <vojtech@suse.com>
Subject: Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model
Message-ID: <20160405134430.q4pqm2oalivjka4s@treble.redhat.com>
References: <cover.1458933243.git.jpoimboe@redhat.com>
 <20160401133944.GQ5522@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20160401133944.GQ5522@pathway.suse.cz>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3163
Lines: 67

On Fri, Apr 01, 2016 at 03:39:44PM +0200, Petr Mladek wrote:
> On Fri 2016-03-25 14:34:47, Josh Poimboeuf wrote:
> > These patches are still a work in progress, but Jiri asked that I share
> > them before I go on vacation next week.  Based on origin/master because
> > it has CONFIG_STACK_VALIDATION.
> 
> I have to follow Mirek and say that it is a great work.
> 
> > There's also a func->immediate flag which allows users to specify that
> > certain functions in the patch can be applied without per-task
> > consistency.  This might be useful if you want to patch a common
> > function like schedule(), and the function change doesn't need
> > consistency but the rest of the patch does.
> 
> We probably should not set func->transition flag when func->immediate
> is set or when the related func->object is set. It currently happens
> only when patch->immediate is set.

Agreed, I'll skip setting func->transition if func->immediate is set.

> > Still have a lot of TODOs, some of them are listed here.  If you see
> > something objectionable, it might be a good idea to make sure it's not
> > already on the TODO list :-)
> > 
> > TODO:
> > - come up with a better name than universe?  KLP_STATE_PREV/NEXT?
> >   KLP_UNPATCHED/PATCHED?  there were some objections to the name in v1.
> 
> The name "universe" has an advantage if we later allow to
> enable/disable more patches in parallel. The integer might hold
> an identifier of the last applied patch. I have been playing with
> this for kGraft one year ago and it was really challenging.
> We should avoid it if possible. It is not really needed
> if we are able to complete any transition in a reasonable time.

Agreed, we should really avoid having more than two universes at a time,
and I don't foresee a reason to do that in the future.  I'll probably
get rid of the universe name in favor of something more descriptive.

> If we support only one transition at a time, a simple boolean
> or even bit should be enough. The most descriptive name would
> be klp_transition_patch_applied but it is quite long.

Yeah, I'll change it to a bool.

> Note that similar information is provided by TIF_KLP_NEED_UPDATE
> flag. We use only this bit in kGraft. It saves some space in
> task_struct but it brings other challenges. We need to prevent
> migration using a global "kgr_immutable" flag until ftrace handlers
> for all patched functions are in place. We need to set the flag
> back when the ftrace handler is called and the global "kgr_immutable"
> flag is set.
> 
> > - update documentation for sysfs, proc, livepatch
> 
> Also we should publish somewhere the information about TIF_KLP_NEED_UPDATE
> flag, e.g. /proc/<pid>/klp_need_update. It is handy to see what
> process blocks the transition. We have something similar in
> kGraft, see in_progress_show() at
> https://git.kernel.org/cgit/linux/kernel/git/jirislaby/kgraft.git/commit/?h=kgraft-4.4&id=1c82fbd7b1fe240f4ed178a6506a93033f6a4bed

Patch 13/14 exposes the per-task patched state in /proc, so I think that
already does what you're asking for?  It doesn't expose
TIF_KLP_NEED_UPDATE, but that's more of an internal detail I think.

-- 
Josh