DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4E034C047B8C
Date: Mon, 21 Aug 2017 17:18:58 -0400
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@kernel.org>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH v3] livepatch: add (un)patch callbacks
Message-ID: <20170821211858.n5wsweei7zew6bi5@redhat.com>
References: <1502911024-16143-1-git-send-email-joe.lawrence@redhat.com>
 <1502911024-16143-2-git-send-email-joe.lawrence@redhat.com>
 <20170818135816.GB25223@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170818135816.GB25223@pathway.suse.cz>
User-Agent: Mutt/1.6.2-neo (2016-08-08)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6543
Lines: 184

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