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

[Miroslav Benes]

On Thu, 21 Dec 2017, Petr Mladek wrote:

> 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.

I agree with Josh here.

If there is a problem with a patch module, the recommended action is to 
simply cancel its transition (by writing 0 to enabled). If there are 
serious reasons to apply the patch, there is force as the last resort. In 
that case the user should probably plan for reboot into an updated kernel 
and not to plan to apply more live patches. 
 
> 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.

Having said the above, I'm not against to update the warning and 
documentation. I would not introduce another force flag to deal with it.

Thanks,
Miroslav