Re: [PATCH v2 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Wed, 30 Aug 2017, Pavel Machek wrote:

> On Thu 2017-08-10 12:48:15, Miroslav Benes wrote:
> > If a task sleeps in a set of patched functions uninterruptibly, it could
> > block the whole transition process indefinitely.  Thus it may be useful
> > to clear its TIF_PATCH_PENDING to allow the process to finish.
> > 
> > Admin can do that now by writing to force sysfs attribute in livepatch
> > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > transition can finish successfully.
> > 
> > Important note! Use wisely. Admin must be sure that it is safe to
> > execute such action. This means that it must be checked that by doing so
> > the consistency model guarantees are not violated.
> 
> Yes, that's what admins are good for. Magically determining what state
> their machine is in, and deciding if all the processes are in the sane
> state and what the consequences are. They have all the tools they need
> to do that, like JTAG connection to the CPU and about 10 years of
> time... do they?

It is not that bad in this case fortunately. The decision depends on

1. which processes are not yet migrated and why (which functions they're 
sleeping on)

AND

2. what functions are patched and what is the nature of a fix.

Admin knows 1. and he needs input from a livepatch author about 2. There 
is no magic there, but cooperation between those two is definitely 
required.
 
> This should taint the kernel at the very least.

That's what we discussed in v1. In cases where the patch does not need the 
consistency model at all, the taint would be too much. But I have no 
problem to transform pr_warn to WARN_ON_ONCE.

> It should also require capabilities beyond "normal root", because it
> allows malicious admin to do "bad things (tm)" to the kernel.

I think that malicious admin can do pretty bad (and more serious) things 
even without this. That is not an excuse, but it is general problem not 
specific to this patch.

> > diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
> > index 343b0bfa1b9f..7626d1b947c2 100644
> > --- a/Documentation/livepatch/livepatch.txt
> > +++ b/Documentation/livepatch/livepatch.txt
> > @@ -183,7 +183,11 @@ attribute. Reading from the file returns all available operations. Writing one
> >  of the strings to the file executes the operation. "signal" is available for
> >  signalling all remaining blocking tasks. This is an alternative for
> >  SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
> > -less harmful to the system.
> > +less harmful to the system. "force" clears TIF_PATCH_PENDING flag of all tasks
> > +and thus forces the tasks to the patched state. Important note! Use "force"
> > +wisely. Administrator must be sure that it is safe to execute such action. This
> > +means that it must be checked that by doing so the consistency model guarantees
> > +are not violated.
> 
> You may want to elaborate here a lot. If you want to pretend
> administrator can decide this, you need to describe what the model is,
> and how administrator get enough information about the system state.

Ok, np. I can add something along the lines I drafted above.

Thanks,
Miroslav