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

[Josh Poimboeuf]

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 

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

[Josh Poimboeuf]

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 

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

[Joe Lawrence]

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

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

[Joe Lawrence]

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

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

[Josh Poimboeuf]

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

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.

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

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

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

> + 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);

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

> +/* Executed on object unpatching (ie, patch disablement) */
> +static void post_patch_callback(struct klp_object *obj)

s/unpatching/patching/

-- 
Josh

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

[Josh Poimboeuf]

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

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.

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

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

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

> + 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);

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

> +/* Executed on object unpatching (ie, patch disablement) */
> +static void post_patch_callback(struct klp_object *obj)

s/unpatching/patching/

-- 
Josh

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

[Joe Lawrence]

Provide livepatch modules a klp_object (un)patching notification
mechanism.  Pre and post-(un)patch callbacks allow livepatch modules to
setup or synchronize changes that would be difficult to support in only
patched-or-unpatched code contexts.

Callbacks can be registered for target module or vmlinux klp_objects,
but each implementation is klp_object specific.

  - Pre-(un)patch callbacks run before any (un)patching action takes
place.

  - Post-(un)patch callbacks run once an object has been (un)patched and
the klp_patch fully transitioned to its target state.

Example use cases include modification of global data and registration
of newly available services/handlers.

See Documentation/livepatch/callback.txt for details.

Signed-off-by: Joe Lawrence 
---
 Documentation/livepatch/callbacks.txt|  75 +++
 include/linux/livepatch.h|  38 ++
 kernel/livepatch/core.c  |  32 -
 kernel/livepatch/patch.c |   5 +-
 kernel/livepatch/transition.c|  19 ++-
 samples/livepatch/Makefile   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 190 +++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 
 8 files changed, 405 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/livepatch/callbacks.txt
 create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
 create mode 100644 samples/livepatch/livepatch-callbacks-mod.c

diff --git a/Documentation/livepatch/callbacks.txt 
b/Documentation/livepatch/callbacks.txt
new file mode 100644
index ..d78f9ba9f74c
--- /dev/null
+++ 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.
+
+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
+in-kernel vmlinux targets, this means that callbacks will always execute
+when a livepatch is enabled/disabled.
+
+For kernel module targets, callbacks will only execute if the target
+module is loaded.  When a kernel module target is (un)loaded, its
+callbacks will execute only if the livepatch module is enabled.
+
+The pre-patch callback is expected to return a status code (0 for
+success, -ERRNO on error).  An error status code will indicate to the
+livepatching core that patching of the current klp_object is not safe
+and to stop the current patching request.  If the problematic klp_object
+is already loaded (i.e. a livepatch is loaded after target code), the
+kernel's module loader will refuse to load the livepatch.  On the other
+hand, if the problematic klp_object is already in place (i.e. a target
+module is loaded after a livepatch), then the module loader will refuse
+to load the target kernel module.
+
+
+Example Use-cases
+-
+
+1 - Update global data
+
+A pre-patch callback can be useful to update a global variable.  For
+example, 75ff39ccc1bd ("tcp: make challenge acks less predictable")
+changes a global sysctl, as well as patches the tcp_send_challenge_ack()
+function.
+
+In this case, if we're being super paranoid, it might make sense to
+patch the data *after* patching is complete with a post-patch callback,
+so that tcp_send_challenge_ack() could first be changed to read
+sysctl_tcp_challenge_ack_limit with READ_ONCE.
+
+
+2 - Support __init and probe function patches
+
+Although __init and probe functions are not directly livepatch-able, it
+may be possible to implement similar updates via pre/post-patch
+callbacks.
+
+48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST") change the way that
+virtnet_probe() initialized its driver's net_device features.  A
+pre/post-patch callback could iterate over all such devices, making a
+similar change to their hw_features value.  (Client functions of the
+value may need to be updated accordingly.)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..5fb8925fc488 

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

[Joe Lawrence]

Provide livepatch modules a klp_object (un)patching notification
mechanism.  Pre and post-(un)patch callbacks allow livepatch modules to
setup or synchronize changes that would be difficult to support in only
patched-or-unpatched code contexts.

Callbacks can be registered for target module or vmlinux klp_objects,
but each implementation is klp_object specific.

  - Pre-(un)patch callbacks run before any (un)patching action takes
place.

  - Post-(un)patch callbacks run once an object has been (un)patched and
the klp_patch fully transitioned to its target state.

Example use cases include modification of global data and registration
of newly available services/handlers.

See Documentation/livepatch/callback.txt for details.

Signed-off-by: Joe Lawrence 
---
 Documentation/livepatch/callbacks.txt|  75 +++
 include/linux/livepatch.h|  38 ++
 kernel/livepatch/core.c  |  32 -
 kernel/livepatch/patch.c |   5 +-
 kernel/livepatch/transition.c|  19 ++-
 samples/livepatch/Makefile   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 190 +++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 
 8 files changed, 405 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/livepatch/callbacks.txt
 create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
 create mode 100644 samples/livepatch/livepatch-callbacks-mod.c

diff --git a/Documentation/livepatch/callbacks.txt 
b/Documentation/livepatch/callbacks.txt
new file mode 100644
index ..d78f9ba9f74c
--- /dev/null
+++ 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.
+
+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
+in-kernel vmlinux targets, this means that callbacks will always execute
+when a livepatch is enabled/disabled.
+
+For kernel module targets, callbacks will only execute if the target
+module is loaded.  When a kernel module target is (un)loaded, its
+callbacks will execute only if the livepatch module is enabled.
+
+The pre-patch callback is expected to return a status code (0 for
+success, -ERRNO on error).  An error status code will indicate to the
+livepatching core that patching of the current klp_object is not safe
+and to stop the current patching request.  If the problematic klp_object
+is already loaded (i.e. a livepatch is loaded after target code), the
+kernel's module loader will refuse to load the livepatch.  On the other
+hand, if the problematic klp_object is already in place (i.e. a target
+module is loaded after a livepatch), then the module loader will refuse
+to load the target kernel module.
+
+
+Example Use-cases
+-
+
+1 - Update global data
+
+A pre-patch callback can be useful to update a global variable.  For
+example, 75ff39ccc1bd ("tcp: make challenge acks less predictable")
+changes a global sysctl, as well as patches the tcp_send_challenge_ack()
+function.
+
+In this case, if we're being super paranoid, it might make sense to
+patch the data *after* patching is complete with a post-patch callback,
+so that tcp_send_challenge_ack() could first be changed to read
+sysctl_tcp_challenge_ack_limit with READ_ONCE.
+
+
+2 - Support __init and probe function patches
+
+Although __init and probe functions are not directly livepatch-able, it
+may be possible to implement similar updates via pre/post-patch
+callbacks.
+
+48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST") change the way that
+virtnet_probe() initialized its driver's net_device features.  A
+pre/post-patch callback could iterate over all such devices, making a
+similar change to their hw_features value.  (Client functions of the
+value may need to be updated accordingly.)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..5fb8925fc488 100644
---