Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model

[Josh Poimboeuf <jpoimboe@redhat.com>]

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