Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks

From: Josh Poimboeuf
Date: Mon Aug 14 2017 - 13:29:42 EST


On Mon, Aug 14, 2017 at 10:53:08AM -0400, Joe Lawrence wrote:
> On 08/11/2017 04:44 PM, Josh Poimboeuf wrote:
> > On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
> >> +++ b/Documentation/livepatch/callbacks.txt
> >> @@ -0,0 +1,75 @@
> >> +(Un)patching Callbacks
> >> +======================
> >> +
> >> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> >> +to execute callback functions when a kernel object is (un)patched.
> >
> > I think it would be helpful to put a little blurb here about why
> > callbacks are needed and when they might be used. Maybe steal some of
> > the description from the first two bullet points here:
> >
> > https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble
>
> Ok -- btw, can you explain this point: "patching otherwise unpatchable
> code (i.e., assembly)". I wasn't sure if you were referring to the
> actual code, or modifying the machine state as setup by some init time
> assembly.

I meant actually patching assembly code.

> > Also, I tested stop_machine() in the callbacks and it seemed to work
> > fine. It might be worth mentioning in the docs that it's an option.
>
> I'll file that under the "you better know what you're doing" section. :)
> If your test would be a better use-case example or sample module than
> what's currently in the patchset, feel free to send it over and I can
> incorporate it.

Well, it's questionable whether using stop_machine() is a good idea, and
it's one of those "use as a last resort" things, so maybe we don't need
to add it to the sample module.

> >> +These callbacks differ from existing kernel facilities:
> >> +
> >> + - Module init/exit code doesn't run when disabling and re-enabling a
> >> + patch.
> >> +
> >> + - A module notifier can't stop the to-be-patched module from loading.
> >> +
> >> +Callbacks are part of the klp_object structure and their implementation
> >> +is specific to the given object. Other livepatch objects may or may not
> >> +be patched, irrespective of the target klp_object's current state.
> >> +
> >> +Callbacks can be registered for the following livepatch actions:
> >> +
> >> + * Pre-patch - before klp_object is patched
> >> +
> >> + * Post-patch - after klp_object has been patched and is active
> >> + across all tasks
> >> +
> >> + * Pre-unpatch - before klp_object is unpatched, patched code is active
> >> +
> >> + * Post-unpatch - after klp_object has been patched, all code has been
> >> + restored and no tasks are running patched code
> >> +
> >> +Callbacks are only executed if its host klp_object is loaded. For
> >
> > "Callbacks are" -> "A callback is" ?
>
> Okay. What about the preceding plural-case instances?

I think it doesn't matter much, as long as each sentence is
grammatically consistent with itself.

> >> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> >> +{
> >> + if (!obj->patched && obj->callbacks.pre_patch)
> >> + return (*obj->callbacks.pre_patch)(obj);
> >> + return 0;
> >> +}
> >> +static inline void klp_post_patch_callback(struct klp_object *obj)
> >> +{
> >> + if (obj->patched && obj->callbacks.post_patch)
> >> + (*obj->callbacks.post_patch)(obj);
> >> +}
> >> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> >> +{
> >> + if (obj->patched && obj->callbacks.pre_unpatch)
> >> + (*obj->callbacks.pre_unpatch)(obj);
> >> +}
> >> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> >> +{
> >> + if (!obj->patched && obj->callbacks.post_unpatch)
> >> + (*obj->callbacks.post_unpatch)(obj);
> >> +}
> >> +
> >
> > Do these need the obj->patched checks? As far as I can tell they seem
> > to be called in the right places and the checks are superfluous.
>
> That is correct. I can leave them (defensive coding) or take them out
> and perhaps add comments above to explain their use and assumptions.

Personally I'd rather get rid of the checks as I found them confusing.

If we really wanted to get defensive, we could add some WARNs, but that
might be overkill.

> >> --- a/kernel/livepatch/transition.c
> >> +++ b/kernel/livepatch/transition.c
> >> @@ -109,9 +109,6 @@ static void klp_complete_transition(void)
> >> }
> >> }
> >>
> >> - if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> >> - module_put(klp_transition_patch->mod);
> >> -
> >> /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> >> if (klp_target_state == KLP_PATCHED)
> >> klp_synchronize_transition();
> >> @@ -130,6 +127,22 @@ static void klp_complete_transition(void)
> >> }
> >>
> >> done:
> >> + klp_for_each_object(klp_transition_patch, obj) {
> >> + if (klp_target_state == KLP_PATCHED)
> >> + klp_post_patch_callback(obj);
> >> + else if (klp_target_state == KLP_PATCHED)
> >
> > s/KLP_PATCHED/KLP_UNPATCHED
>
> Ahh, I was so focused on the loadable module cases in
> module_coming/going that I botched this case. Will fix for v3.
>
> >> + klp_post_unpatch_callback(obj);
> >> + }
> >> +
> >> + /*
> >> + * See complementary comment in __klp_enable_patch() for why we
> >> + * keep the module reference for immediate patches.
> >> + */
> >> + if (!klp_transition_patch->immediate) {
> >> + if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> >> + module_put(klp_transition_patch->mod);
> >> + }
> >> +
> >
> > Maybe combine these into a single 'if' for clarity:
> >
> > if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
> > !klp_transition_patch->immediate)
> > module_put(klp_transition_patch->mod);
>
> How about this arrangement:
>
> if (!klp_transition_patch->immediate &&
> klp_target_state == KLP_UNPATCHED && !immediate_func) {
> module_put(klp_transition_patch->mod);
> }
>
> 1) It leads with the klp_transition_patch->immediate variable, which the
> preceding comment and goto is all about and 2) brackets the multiline
> conditional part -- a personal preference, but I could drop for
> convention sake.

The bracket's fine with me. Personally I think it makes more sense to
bunch the immediate checks together.

> >> + * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch
> >> + * callback return status. Try setting up a non-zero status
> >> + * such as -19 (-ENODEV):
> >> + *
> >> + * # Load demo livepatch, vmlinux is patched
> >> + * insmod samples/livepatch/livepatch-callbacks-demo.ko
> >> + *
> >> + * # Setup next pre-patch callback to return -ENODEV
> >> + * echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
> >
> > Git complained about trailing whitespace here ^
> >
> >> + *
> >> + * # Module loader refuses to load the target module
> >> + * insmod samples/livepatch/livepatch-callbacks-mod.ko
> >
> > and here ^
>
> Oh hey, look who was too cool to run checkpatch, again.
>
> >> +/* Executed on object unpatching (ie, patch disablement) */
> >> +static void post_patch_callback(struct klp_object *obj)
> >
> > s/unpatching/patching/
> >
>
> Good catch.
>
> So v2 was a bit rushed to try and get something out there to talk about:
>
> Are the callback locations accurate to your v1 suggestions?

Yeah, it all looked good to me.

> How do you feel about a pre-patch callback potentially preventing the
> loading of a kernel module -or- the patch module itself depending on
> which is loaded first?

I think that makes sense.

> Is the pre-patch return status sufficient? (ie, I couldn't see how
> post-patch, pre-unpatch, post-patch callbacks could affect the actions
> already set in motion.)

I think so.

--
Josh