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
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
