lkml.org 
[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 1/1] livepatch: add (un)patch callbacks
Date
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.

> 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.

>> +
>> +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?

>
>> +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.

>> --- 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.

>> + * 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?

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?

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.)

Thanks,

-- Joe

\
 
 \ /
  Last update: 2017-08-14 16:54    [W:0.055 / U:7.584 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site