Date: Wed, 20 Dec 2017 11:09:37 -0600
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Miroslav Benes <mbenes@suse.cz>, jeyu@kernel.org, jikos@kernel.org,
        linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
        jbaron@akamai.com
Subject: Re: [PATCH 1/2] livepatch: Remove immediate feature
Message-ID: <20171220170937.2wow5fkxoza2cf6v@treble>
References: <20171208172523.12150-1-mbenes@suse.cz>
 <20171208172523.12150-2-mbenes@suse.cz>
 <20171220143512.7uawixpxkxre6n7i@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20171220143512.7uawixpxkxre6n7i@pathway.suse.cz>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3066
Lines: 71

On Wed, Dec 20, 2017 at 03:35:12PM +0100, Petr Mladek wrote:
> On Fri 2017-12-08 18:25:22, Miroslav Benes wrote:
> > immediate flag has been used to disable per-task consistency and patch
> > all tasks immediately. It could be useful if the patch doesn't change any
> > function or data semantics.
> > 
> > However, it causes problems on its own. The consistency problem is
> > currently broken with respect to immediate patches.
> > 
> > func            a
> > patches         1i
> >                 2i
> >                 3
> > 
> > When the patch 3 is applied, only 2i function is checked (by stack
> > checking facility). There might be a task sleeping in 1i though. Such
> > task is migrated to 3, because we do not check 1i in
> > klp_check_stack_func() at all.
> > 
> > Coming atomic replace feature would be easier to implement and more
> > reliable without immediate.
> > 
> > Moreover, the fake signal and force feature give us almost the same
> > benefits and the user can decide to use them in problematic situations
> > (while immediate needs to be set before the patch is applied). It is
> > also more isolated in terms of code.
> > 
> > Thus, remove immediate feature completely and save us from the problems.
> 
> Sigh, the force feature actually have the same problem. We would use
> it when a process never has a reliable stack or when it is endlessly
> sleeping in a function that might have been patched immediately.
> 
> The documentation about the force feature says that the user should
> consult the patch provider before using the flag. The provider
> would check that it is really safe in the given situation
> and eventually allow to use the force.
> 
> But what about any future livepatches? If the force flag was
> safe for a given livepatch/process, it does not mean that
> it would be safe for the next one. The process might still
> be sleeping on the original function or on one lower in
> the stack.
> 
> I see two possibilities. We could either refuse loading new
> livepatches after using the force flag. Or we would need
> to check all variants of the function "a" that might still
> be in use.
> 
> I think that we might want to check the stack correctly.
> Note that we need to take care also about livepatches that
> were disabled. They are usually removed from func->stack_node.
> We might need to maintain separate stack where we would
> put all variants of the function that might be in
> use when using the immediate or force flag.
> 
> I am not sure if we still want to remove the immediate
> flag then.

"Using the force" is a nuclear option.  User beware.  If you use it (or
recommend that others use it), be prepared for the consequences.  That
means anticipating how forcing this patch might affect future patches,
and planning accordingly.

>From a livepatch code standpoint, let's avoid adding complexity (or
limitations) where none are needed.  I think all we need to do is
permanently disable patch module unloading when somebody forces a patch,
which we already do.  Otherwise the user is on their own.

-- 
Josh