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
> > --- 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.
I was skeptical about this call, too. After reading your comment, I
realize it shouldn't be needed here.
> > klp_unpatch_object(obj);
> > return ret;
> > }
> > @@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
> > struct klp_object *obj;
> >
> > klp_for_each_object(patch, obj)
> > - if (obj->patched)
> > + if (obj->patched) {
> > + klp_pre_unpatch_callback(obj);
>
> This is even more strange. The corresponding klp_post_patch_callback()
> is called at the very end of the transaction when the patch is already
> used by the entire system. Therefore the operation should be reverted
> before we start disabling the patch.
>
> IMHO, klp_pre_unpatch_callback() should get called in
> __klp_disable_patch(). I would put it right after
> klp_init_transition(patch, KLP_UNPATCHED);
Okay, looking at the transition code, this makes sense. I'll move the
pre-(un)patch calls into the __enable_patch() / __disable_patch()
functions after initalizing the transition. I think that should clean
up some of the strange ordering of kernel log msgs as well.
> Another advantage of this logic is that we actually do not need
> to care about these callbacks in klp_reverse_transition().
> But we should probably mention it in the documentation
> how the actions done by the patch and unpatch callbacks
> are related.
>
> Otherwise, it looks fine to me.
Thanks for reviewing.
-- Joe
