Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states

[Chris J Arges]

On Tue, Apr 12, 2016 at 12:16:00PM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 12, 2016 at 09:44:43AM -0500, Chris J Arges wrote:
> > On Fri, Mar 25, 2016 at 02:34:55PM -0500, Josh Poimboeuf wrote:
> > > Once we have a consistency model, patches and their objects will be
> > > enabled and disabled at different times.  For example, when a patch is
> > > disabled, its loaded objects' funcs can remain registered with ftrace
> > > indefinitely until the unpatching operation is complete and they're no
> > > longer in use.
> > > 
> > > It's less confusing if we give them different names: patches can be
> > > enabled or disabled; objects (and their funcs) can be patched or
> > > unpatched:
> > > 
> > > - Enabled means that a patch is logically enabled (but not necessarily
> > >   fully applied).
> > > 
> > > - Patched means that an object's funcs are registered with ftrace and
> > >   added to the klp_ops func stack.
> > > 
> > > Also, since these states are binary, represent them with booleans
> > > instead of ints.
> > > 
> > 
> > Josh,
> > 
> > Awesome work here first of all!
> > 
> > Looking through the patchset a bit I see the following bools:
> > - functions: patched, transitioning
> > - objects: patched
> > - patches: enabled
> > 
> > It seems this reflects the following states at a patch level:
> > 	disabled - module inserted, not yet logically enabled
> > 	enabled - logically enabled, but not all objects/functions patched
> > 	transitioning - objects/functions are being applied or reverted
> > 	patched - all objects/functions patched
> > 
> > However each object and function could have the same state and the parent object
> > just reflects the 'aggregate state'. For example if all funcs in an object are
> > patched then the object is also patched.
> > 
> > Perhaps we need more states (or maybe there will be more in the future), but
> > wouldn't this just be easier to have something like for each patch, object, and
> > function?
> > 
> > enum klp_state{
> > 	KLP_DISABLED,
> > 	KLP_ENABLED,
> > 	KLP_TRANSITION,
> > 	KLP_PATCHED,
> > }
> > 
> > 
> > I'm happy to help out too.
> 
> Thanks for the comments.  First let me try to explain why I chose two
> bools rather than a single state variable.
> 
> At a func level, it's always in one of the following states:
> 
>   patched=0 transition=0: unpatched
>   patched=0 transition=1: unpatched, temporary starting state
>   patched=1 transition=1: patched, may be visible to some tasks
>   patched=1 transition=0: patched, visible to all tasks
> 
> And for unpatching, it goes in the reverse order:
> 
>   patched=1 transition=0: patched, visible to all tasks
>   patched=1 transition=1: patched, may be visible to some tasks
>   patched=0 transition=1: unpatched, temporary ending state
>   patched=0 transition=0: unpatched
> 
> (note to self, put the above in a comment somewhere)
>

This is helpful and makes more sense.
 
> Now, we could convert the states from two bools into a single enum.  But
> I think it would complicate the code.  Because nowhere in the code does
> it need to access the full state.  In some places it accesses
> func->patched and in other places it accesses func->transition, but it
> never needs to access both.
> 
> So for example, the following check in klp_ftrace_handler():
> 
> 	if (unlikely(func->transition)) { 
> 
> would change to:
> 
> 	if (unlikely(func->state == KLP_ENABLED || func->state == KLP_TRANSITION)) {
> 
> Sure, we could use a helper function to make that more readable.  But
> with the bools its clearer and you don't need a helper function.
> 
> As another example, see the following code in klp_complete_transition():
> 
> 	klp_for_each_object(klp_transition_patch, obj)
> 		klp_for_each_func(obj, func)
> 			func->transition = false;
> 
> The last line would have to be changed to something like:
> 
> 			if (patching...)
> 				func->state = KLP_PATCHED;
> 			else /* unpatching */
> 				func->state = KLP_DISABLED;
> 		
> So that's why I picked two bools over a single state variable: it seems
> to make the code simpler.
> 
> As to the other idea about copying the func states to the object and
> patch level, I get the feeling that would also complicate the code.  We
> patch and transition at a function granularity, so the "real" state is
> at the func level.  Proliferating that state to objects and patches
> might be tricky to get right, and it could make it harder to understand
> exactly what's going on in the code.  And I don't really see a benefit
> to doing that.
> 
> -- 
> Josh
> 

I think keeping it simple makes a ton of sense, thanks for the explanation.

I'm also envisioning how to troubleshoot patches that don't converge.

So if someone wanted to check if a function has been fully patched on all tasks
they would check:

/sys/kernel/livepatch/<patch>/transition
/sys/kernel/livepatch/<patch>/patched

And see if patched=1 and transition=0 things are applied.

However if patched=1 and transition=1 then if a user wanted to dig down and see
which pid's we were waiting on we could do:

 cat /proc/*/patch_status

and check if any pid's patch_status values still contain KLP_UNIVERSE_OLD.

If we wanted to see which function in the task needs patching we could:
 cat /proc/<pid>/stack
and see if anything in that stack contains the functions in question.

Anyway I'll keep looking at this patchset, patching using the samples/livepatch
code works for me without issue so far.

--chris