Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model

[Josh Poimboeuf]

On Mon, May 09, 2016 at 02:23:03PM +0200, Petr Mladek wrote:
> On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> > On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > > I have missed that the two commands are called with preemption
> > > disabled. So, I had the following crazy scenario in mind:
> > > 
> > > 
> > > CPU0				CPU1
> > > 
> > > klp_enable_patch()
> > > 
> > >   klp_target_state = KLP_PATCHED;
> > > 
> > >   for_each_task()
> > >      set TIF_PENDING_PATCH
> > > 
> > > 				# task 123
> > > 
> > > 				if (klp_patch_pending(current)
> > > 				  klp_patch_task(current)
> > > 
> > >                                     clear TIF_PENDING_PATCH
> > > 
> > > 				    smp_rmb();
> > > 
> > > 				    # switch to assembly of
> > > 				    # klp_patch_task()
> > > 
> > > 				    mov klp_target_state, %r12
> > > 
> > > 				    # interrupt and schedule
> > > 				    # another task
> > > 
> > > 
> > >   klp_reverse_transition();
> > > 
> > >     klp_target_state = KLP_UNPATCHED;
> > > 
> > >     klt_try_to_complete_transition()
> > > 
> > >       task = 123;
> > >       if (task->patch_state == klp_target_state;
> > >          return 0;
> > > 
> > >     => task 123 is in target state and does
> > >     not block conversion
> > > 
> > >   klp_complete_transition()
> > > 
> > > 
> > >   # disable previous patch on the stack
> > >   klp_disable_patch();
> > > 
> > >     klp_target_state = KLP_UNPATCHED;
> > >   
> > >   
> > > 				    # task 123 gets scheduled again
> > > 				    lea %r12, task->patch_state
> > > 
> > > 				    => it happily stores an outdated
> > > 				    state
> > > 
> > 
> > Thanks for the clear explanation, this helps a lot.
> > 
> > > This is why the two functions should get called with preemption
> > > disabled. We should document it at least. I imagine that we will
> > > use them later also in another context and nobody will remember
> > > this crazy scenario.
> > > 
> > > Well, even disabled preemption does not help. The process on
> > > CPU1 might be also interrupted by an NMI and do some long
> > > printk in it.
> > > 
> > > IMHO, the only safe approach is to call klp_patch_task()
> > > only for "current" on a safe place. Then this race is harmless.
> > > The switch happen on a safe place, so that it does not matter
> > > into which state the process is switched.
> > 
> > I'm not sure about this solution.  When klp_complete_transition() is
> > called, we need all tasks to be patched, for good.  We don't want any of
> > them to randomly switch to the wrong state at some later time in the
> > middle of a future patch operation.  How would changing klp_patch_task()
> > to only use "current" prevent that?
> 
> You are right that it is pity but it really should be safe because
> it is not entirely random.
> 
> If the race happens and assign an outdated value, there are two
> situations:
> 
> 1. It is assigned when there is not transition in the progress.
>    Then it is OK because it will be ignored by the ftrace handler.
>    The right state will be set before the next transition starts.
> 
> 2. It is assigned when some other transition is in progress.
>    Then it is OK as long as the function is called from "current".
>    The "wrong" state will be used consistently. It will switch
>    to the right state on another safe state.

Maybe it would be safe, though I'm not entirely convinced.  Regardless I
think we should avoid these situations entirely because they create
windows for future bugs and races.

> > > By other words, the task state might be updated only
> > > 
> > >    + by the task itself on a safe place
> > >    + by other task when the updated on is sleeping on a safe place
> > > 
> > > This should be well documented and the API should help to avoid
> > > a misuse.
> > 
> > I think we could fix it to be safe for future callers who might not have
> > preemption disabled with a couple of changes to klp_patch_task():
> > disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> > before changing the patch state:
> > 
> >   void klp_patch_task(struct task_struct *task)
> >   {
> >   	preempt_disable();
> >   
> >   	if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> >   		task->patch_state = READ_ONCE(klp_target_state);
> >   
> >   	preempt_enable();
> >   }
> 
> It reduces the race window a bit but it is still there. For example,
> NMI still might add a huge delay between reading klp_target_state
> and assigning task->patch state.

Maybe you missed this paragraph from my last email:

| We would also need a synchronize_sched() after the patching is complete,
| either at the end of klp_try_complete_transition() or in
| klp_complete_transition().  That would make sure that all existing calls
| to klp_patch_task() are done.

So a huge NMI delay wouldn't be a problem here.  The call to
synchronize_sched() in klp_complete_transition() would sleep until the
NMI handler returns and the critical section of klp_patch_task()
finishes.  So once a patch is complete, we know that it's really
complete.

> What about the following?
> 
> /*
>  * This function might assign an outdated value if the transaction
> `* is reverted and finalized in parallel. But it is safe. If the value
>  * is assigned outside of a transaction, it is ignored and the next
>  * transaction will set the right one. Or if it gets assigned
>  * inside another transaction, it will repeat the cycle and
>  * set the right state.
>  */
> void klp_update_current_patch_state()
> {
> 	while (test_and_clear_tsk_thread_flag(current, TIF_PATCH_PENDING))
> 		current->patch_state = READ_ONCE(klp_target_state);
> }

I'm not sure how this would work.  How would the thread flag get set
again after it's been cleared?

Also I really don't like the idea of randomly updating a task's patch
state after the transition has been completed.

> Note that the disabled preemption helped only partially,
> so I think that it was not really needed.
> 
> Hmm, it means that the task->patch_state  might be either
> KLP_PATCHED or KLP_UNPATCHED outside a transition. I wonder
> if the tristate really brings some advantages.
> 
> 
> Alternatively, we might synchronize the operation with klp_mutex.
> The function is called in a slow path and in a safe context.
> Well, it might cause contention on the lock when many CPUs are
> trying to update their tasks.

I don't think a mutex would work because at least the ftrace handler
(and maybe more) can't sleep.  Maybe a spinlock could work but I think
that would be overkill.

-- 
Josh