Re: [PATCH v3] livepatch: add (un)patch callbacks

[Petr Mladek]

On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > 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.
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > >   goto err;
> > >   }
> > >  
> > > + klp_post_patch_callback(obj);
> > 
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
> 
> Can you elaborate a bit on this scenario?  When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier?  Is it possible to load or unload a
> module like this?  I'd like to add this scenario to my test script if
> possible.

if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().

The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.


> > > +
> > >   break;
> > >   }
> > >   }
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > >   if (patch->enabled || patch == klp_transition_patch) {
> > >   pr_notice("reverting patch '%s' on unloading 
> > > module '%s'\n",
> > > patch->mod->name, obj->mod->name);
> > > +
> > > + klp_pre_unpatch_callback(obj);
> > 
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
> 
> Ditto.

This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.

Best Regards,
Petr

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Petr Mladek]

On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > 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.
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > >   goto err;
> > >   }
> > >  
> > > + klp_post_patch_callback(obj);
> > 
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
> 
> Can you elaborate a bit on this scenario?  When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier?  Is it possible to load or unload a
> module like this?  I'd like to add this scenario to my test script if
> possible.

if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().

The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.


> > > +
> > >   break;
> > >   }
> > >   }
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > >   if (patch->enabled || patch == klp_transition_patch) {
> > >   pr_notice("reverting patch '%s' on unloading 
> > > module '%s'\n",
> > > patch->mod->name, obj->mod->name);
> > > +
> > > + klp_pre_unpatch_callback(obj);
> > 
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
> 
> Ditto.

This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.

Best Regards,
Petr

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Joe Lawrence]

On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > 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.
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 194991ef9347..500dc9b2b361 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -138,6 +154,71 @@ struct klp_patch {
> >  func->old_name || func->new_func || func->old_sympos; \
> >  func++)
> >  
> > +/**
> > + * klp_is_object_loaded() - is klp_object currently loaded?
> > + * @obj:   klp_object pointer
> > + *
> > + * Return: true if klp_object is loaded (always true for vmlinux)
> > + */
> > +static inline bool klp_is_object_loaded(struct klp_object *obj)
> > +{
> > +   return !obj->name || obj->mod;
> > +}
> > +
> > +/**
> > + * klp_pre_patch_callback - execute before klp_object is patched
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Return: status from callback
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline int klp_pre_patch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.pre_patch)
> > +   return (*obj->callbacks.pre_patch)(obj);
> > +   return 0;
> > +}
> > +
> > +/**
> > + * klp_post_patch_callback() - execute after klp_object is patched
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_post_patch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.post_patch)
> > +   (*obj->callbacks.post_patch)(obj);
> > +}
> > +
> > +/**
> > + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> > + *  and is active across all tasks
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.pre_unpatch)
> > +   (*obj->callbacks.pre_unpatch)(obj);
> > +}
> > +
> > +/**
> > + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> > + *   all code has been restored and no tasks
> > + *   are running patched code
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.post_unpatch)
> > +   (*obj->callbacks.post_unpatch)(obj);
> > +}
> 
> I guess that we do not want to make these function usable
> outside livepatch code. Thefore these inliners should go
> to kernel/livepatch/core.h or so.

Okay, I can stash them away in an internal header file like core.h.

> > +
> >  int klp_register_patch(struct klp_patch *);
> >  int klp_unregister_patch(struct klp_patch *);
> >  int klp_enable_patch(struct klp_patch *);
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..ddb23e18a357 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > goto err;
> > }
> >  
> > +   klp_post_patch_callback(obj);
> 
> This should be called only if (patch != klp_transition_patch).
> Otherwise, it would be called too early.

Can you elaborate a bit on this scenario?  When would the transition
patch (as I understand it, a livepatch not quite fully (un)patched) hit
the module coming/going notifier?  Is it possible to load or unload a
module like this?  I'd like to add this scenario to my test script if
possible.
 
> > +
> > break;
> > }
> > }
> > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > if (patch->enabled || patch == klp_transition_patch) {
> > pr_notice("reverting patch '%s' on unloading 
> > module '%s'\n",
> >   patch->mod->name, obj->mod->name);
> > +
> > +   klp_pre_unpatch_callback(obj);
> 
> Also the pre_unpatch() callback should be called only
> if (patch != klp_transition_patch). Otherwise, it should have
> already been called. It is not the current case but see below.

Ditto.

> > klp_unpatch_object(obj);
> > +   klp_post_unpatch_callback(obj);
> > }
> >  
> > klp_free_object_loaded(obj);
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index 52c4e907c14b..0eed0df6e6d9 100644
> > --- 

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Joe Lawrence]

On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > 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.
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 194991ef9347..500dc9b2b361 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -138,6 +154,71 @@ struct klp_patch {
> >  func->old_name || func->new_func || func->old_sympos; \
> >  func++)
> >  
> > +/**
> > + * klp_is_object_loaded() - is klp_object currently loaded?
> > + * @obj:   klp_object pointer
> > + *
> > + * Return: true if klp_object is loaded (always true for vmlinux)
> > + */
> > +static inline bool klp_is_object_loaded(struct klp_object *obj)
> > +{
> > +   return !obj->name || obj->mod;
> > +}
> > +
> > +/**
> > + * klp_pre_patch_callback - execute before klp_object is patched
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Return: status from callback
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline int klp_pre_patch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.pre_patch)
> > +   return (*obj->callbacks.pre_patch)(obj);
> > +   return 0;
> > +}
> > +
> > +/**
> > + * klp_post_patch_callback() - execute after klp_object is patched
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_post_patch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.post_patch)
> > +   (*obj->callbacks.post_patch)(obj);
> > +}
> > +
> > +/**
> > + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> > + *  and is active across all tasks
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.pre_unpatch)
> > +   (*obj->callbacks.pre_unpatch)(obj);
> > +}
> > +
> > +/**
> > + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> > + *   all code has been restored and no tasks
> > + *   are running patched code
> > + * @obj:   invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> > +{
> > +   if (obj->callbacks.post_unpatch)
> > +   (*obj->callbacks.post_unpatch)(obj);
> > +}
> 
> I guess that we do not want to make these function usable
> outside livepatch code. Thefore these inliners should go
> to kernel/livepatch/core.h or so.

Okay, I can stash them away in an internal header file like core.h.

> > +
> >  int klp_register_patch(struct klp_patch *);
> >  int klp_unregister_patch(struct klp_patch *);
> >  int klp_enable_patch(struct klp_patch *);
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..ddb23e18a357 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > goto err;
> > }
> >  
> > +   klp_post_patch_callback(obj);
> 
> This should be called only if (patch != klp_transition_patch).
> Otherwise, it would be called too early.

Can you elaborate a bit on this scenario?  When would the transition
patch (as I understand it, a livepatch not quite fully (un)patched) hit
the module coming/going notifier?  Is it possible to load or unload a
module like this?  I'd like to add this scenario to my test script if
possible.
 
> > +
> > break;
> > }
> > }
> > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > if (patch->enabled || patch == klp_transition_patch) {
> > pr_notice("reverting patch '%s' on unloading 
> > module '%s'\n",
> >   patch->mod->name, obj->mod->name);
> > +
> > +   klp_pre_unpatch_callback(obj);
> 
> Also the pre_unpatch() callback should be called only
> if (patch != klp_transition_patch). Otherwise, it should have
> already been called. It is not the current case but see below.

Ditto.

> > klp_unpatch_object(obj);
> > +   klp_post_unpatch_callback(obj);
> > }
> >  
> > klp_free_object_loaded(obj);
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index 52c4e907c14b..0eed0df6e6d9 100644
> > --- 

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Petr Mladek]

On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> 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.
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..500dc9b2b361 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -138,6 +154,71 @@ struct klp_patch {
>func->old_name || func->new_func || func->old_sympos; \
>func++)
>  
> +/**
> + * klp_is_object_loaded() - is klp_object currently loaded?
> + * @obj: klp_object pointer
> + *
> + * Return: true if klp_object is loaded (always true for vmlinux)
> + */
> +static inline bool klp_is_object_loaded(struct klp_object *obj)
> +{
> + return !obj->name || obj->mod;
> +}
> +
> +/**
> + * klp_pre_patch_callback - execute before klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_patch)
> + return (*obj->callbacks.pre_patch)(obj);
> + return 0;
> +}
> +
> +/**
> + * klp_post_patch_callback() - execute after klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_patch)
> + (*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> + *  and is active across all tasks
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_unpatch)
> + (*obj->callbacks.pre_unpatch)(obj);
> +}
> +
> +/**
> + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> + *   all code has been restored and no tasks
> + *   are running patched code
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_unpatch)
> + (*obj->callbacks.post_unpatch)(obj);
> +}

I guess that we do not want to make these function usable
outside livepatch code. Thefore these inliners should go
to kernel/livepatch/core.h or so.

> +
>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..ddb23e18a357 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
>   goto err;
>   }
>  
> + klp_post_patch_callback(obj);

This should be called only if (patch != klp_transition_patch).
Otherwise, it would be called too early.

> +
>   break;
>   }
>   }
> @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
>   if (patch->enabled || patch == klp_transition_patch) {
>   pr_notice("reverting patch '%s' on unloading 
> module '%s'\n",
> patch->mod->name, obj->mod->name);
> +
> + klp_pre_unpatch_callback(obj);

Also the pre_unpatch() callback should be called only
if (patch != klp_transition_patch). Otherwise, it should have
already been called. It is not the current case but see below.


>   klp_unpatch_object(obj);
> + klp_post_unpatch_callback(obj);
>   }
>  
>   klp_free_object_loaded(obj);
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 52c4e907c14b..0eed0df6e6d9 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
>   klp_for_each_func(obj, func) {
>   ret = klp_patch_func(func);
>   if (ret) {
> + klp_pre_unpatch_callback(obj);

This looks strange (somehow asymetric). IMHO, it should not be
needed. klp_pre_unpatch_callback() should revert changes done
by klp_post_patch_callback() that has not run yet.

>   klp_unpatch_object(obj);
>   return ret;
>   }
> @@ -271,6 +272,8 @@ void 

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Petr Mladek]

On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> 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.
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..500dc9b2b361 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -138,6 +154,71 @@ struct klp_patch {
>func->old_name || func->new_func || func->old_sympos; \
>func++)
>  
> +/**
> + * klp_is_object_loaded() - is klp_object currently loaded?
> + * @obj: klp_object pointer
> + *
> + * Return: true if klp_object is loaded (always true for vmlinux)
> + */
> +static inline bool klp_is_object_loaded(struct klp_object *obj)
> +{
> + return !obj->name || obj->mod;
> +}
> +
> +/**
> + * klp_pre_patch_callback - execute before klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_patch)
> + return (*obj->callbacks.pre_patch)(obj);
> + return 0;
> +}
> +
> +/**
> + * klp_post_patch_callback() - execute after klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_patch)
> + (*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> + *  and is active across all tasks
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_unpatch)
> + (*obj->callbacks.pre_unpatch)(obj);
> +}
> +
> +/**
> + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> + *   all code has been restored and no tasks
> + *   are running patched code
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_unpatch)
> + (*obj->callbacks.post_unpatch)(obj);
> +}

I guess that we do not want to make these function usable
outside livepatch code. Thefore these inliners should go
to kernel/livepatch/core.h or so.

> +
>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..ddb23e18a357 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
>   goto err;
>   }
>  
> + klp_post_patch_callback(obj);

This should be called only if (patch != klp_transition_patch).
Otherwise, it would be called too early.

> +
>   break;
>   }
>   }
> @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
>   if (patch->enabled || patch == klp_transition_patch) {
>   pr_notice("reverting patch '%s' on unloading 
> module '%s'\n",
> patch->mod->name, obj->mod->name);
> +
> + klp_pre_unpatch_callback(obj);

Also the pre_unpatch() callback should be called only
if (patch != klp_transition_patch). Otherwise, it should have
already been called. It is not the current case but see below.


>   klp_unpatch_object(obj);
> + klp_post_unpatch_callback(obj);
>   }
>  
>   klp_free_object_loaded(obj);
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 52c4e907c14b..0eed0df6e6d9 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
>   klp_for_each_func(obj, func) {
>   ret = klp_patch_func(func);
>   if (ret) {
> + klp_pre_unpatch_callback(obj);

This looks strange (somehow asymetric). IMHO, it should not be
needed. klp_pre_unpatch_callback() should revert changes done
by klp_post_patch_callback() that has not run yet.

>   klp_unpatch_object(obj);
>   return ret;
>   }
> @@ -271,6 +272,8 @@ void 

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Josh Poimboeuf]

On Wed, Aug 16, 2017 at 03:17:04PM -0400, Joe Lawrence wrote:
> 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|  87 
>  include/linux/livepatch.h|  81 
>  kernel/livepatch/core.c  |  37 --
>  kernel/livepatch/patch.c |   5 +-
>  kernel/livepatch/transition.c|  21 ++-
>  samples/livepatch/Makefile   |   2 +
>  samples/livepatch/livepatch-callbacks-demo.c | 190 
> +++
>  samples/livepatch/livepatch-callbacks-mod.c  |  53 
>  8 files changed, 462 insertions(+), 14 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 ..e18f2678f3bb
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,87 @@
> +(Un)patching Callbacks
> +==
> +
> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> +to execute callback functions when a kernel object is (un)patched.  They
> +can be considered a "power feature" that extends livepatching abilities
> +to include:
> +
> +  - Safe updates to global data
> +
> +  - "Patches" to init and probe functions
> +
> +  - Patching otherwise unpatchable code (i.e. assembly)
> +
> +In most cases, (un)patch hooks will need to be used in conjunction with

s/hooks/callbacks/

(though I wouldn't be opposed to changing them back to "hooks" anyway as
it's one fewer syllable :-) but no strong opinion either way on that)

-- 
Josh

Re: [PATCH v3] livepatch: add (un)patch callbacks

[Josh Poimboeuf]

On Wed, Aug 16, 2017 at 03:17:04PM -0400, Joe Lawrence wrote:
> 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|  87 
>  include/linux/livepatch.h|  81 
>  kernel/livepatch/core.c  |  37 --
>  kernel/livepatch/patch.c |   5 +-
>  kernel/livepatch/transition.c|  21 ++-
>  samples/livepatch/Makefile   |   2 +
>  samples/livepatch/livepatch-callbacks-demo.c | 190 
> +++
>  samples/livepatch/livepatch-callbacks-mod.c  |  53 
>  8 files changed, 462 insertions(+), 14 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 ..e18f2678f3bb
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,87 @@
> +(Un)patching Callbacks
> +==
> +
> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> +to execute callback functions when a kernel object is (un)patched.  They
> +can be considered a "power feature" that extends livepatching abilities
> +to include:
> +
> +  - Safe updates to global data
> +
> +  - "Patches" to init and probe functions
> +
> +  - Patching otherwise unpatchable code (i.e. assembly)
> +
> +In most cases, (un)patch hooks will need to be used in conjunction with

s/hooks/callbacks/

(though I wouldn't be opposed to changing them back to "hooks" anyway as
it's one fewer syllable :-) but no strong opinion either way on that)

-- 
Josh