Re: [PATCH 1/2] livepatch: Remove immediate feature

From: Petr Mladek <pmladek@suse.com>
To: Josh Poimboeuf <jpoimboe@redhat.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
Date: Thu, 21 Dec 2017 14:30:04 +0100	[thread overview]
Message-ID: <20171221133004.lilrqfiu3btb74lj@pathway.suse.cz> (raw)
In-Reply-To: <20171220170937.2wow5fkxoza2cf6v@treble>

On Wed 2017-12-20 11:09:37, Josh Poimboeuf wrote:
> 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.
> > > 
> > 
> > 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.
> > 
> > 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.
> 
> "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.

This looks like a rather weak protection against nuclear diseases ;-)

If we want to keep it simple and safe, we should either print
a big fact warning about this danger when the option is used.
Or we should allow to load new patches only with yet another
force flag.

Anyway, I agree that we should keep it simple. The fact is
that the immediate flag removal makes the code better
readable.

Best Regards,
Petr