Date: Fri, 18 Aug 2017 15:58:16 +0200
From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.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: <20170818135816.GB25223@pathway.suse.cz>
References: <1502911024-16143-1-git-send-email-joe.lawrence@redhat.com>
 <1502911024-16143-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1502911024-16143-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5413
Lines: 165

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

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.

Best Regards,
Petr