Date: Wed, 27 Sep 2017 14:17:20 +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: [RFC] livepatch: unpatch all klp_objects if klp_module_coming
 fails
Message-ID: <20170927121720.GN21048@pathway.suse.cz>
References: <6ed9c3ac-ac8f-8678-a884-754cdd07b447@redhat.com>
 <1505338598-16041-1-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1505338598-16041-1-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: 3995
Lines: 128

On Wed 2017-09-13 17:36:38, Joe Lawrence wrote:
> >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@redhat.com>
> Date: Wed, 13 Sep 2017 16:51:13 -0400
> Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
> 
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fail to patch for whatever
> reason, klp_module_coming()'s error path should double back and unpatch
> any previous kernel object that was patched for a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index aca62c4b8616..7f5192618cc8 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
>  				goto err;
>  			}
>  
> +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> @@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
>  	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
>  		patch->mod->name, obj->mod->name, obj->mod->name);
>  	mod->klp_alive = false;
> -	klp_free_object_loaded(obj);
> +
> +	/*
> +	 * Run back through the patch list and unpatch any klp_object that
> +	 * was patched before hitting an error above.
> +	 */
> +
> +	list_for_each_entry(patch, &klp_patches, list) {
> +
> +		if (!patch->enabled || patch == klp_transition_patch)
> +			continue;

klp_init_object_loaded() is called even the patch is not enabled.
Therefore we need to call klp_free_object_loaded() for all
objects where this was called.

> +		klp_for_each_object(patch, obj) {
> +
> +			if (!obj->patched || !klp_is_module(obj) ||
> +			    strcmp(obj->name, mod->name))
> +				continue;
> +
> +			klp_pre_unpatch_callback(obj);
> +pr_err("JL: klp_unpatch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +			klp_unpatch_object(obj);
> +			klp_post_unpatch_callback(obj);
> +			klp_free_object_loaded(obj);
> +
> +			break;
> +		}
> +	}

Well, we basically need to do the same as we do in
__klp_module_coming(). I would suggest to somehow
reuse the code.

The question is how to detect the patches that need
the revert. I would suggest to use the same trick that
we use in klp_free_funcs_limited() and do something like:

void __klp_module_going_limited(struct module *mod,
				struct klp_patch *limit)
{
	struct klp_patch *patch;
	struct klp_object *obj;

	/*
	 * Each module has to know that klp_module_going()
	 * has been called. We never know what module will
	 * get patched by a new patch.
	 */
	mod->klp_alive = false;

	list_for_each_entry(patch, &klp_patches, list) {
		if (patch == limit)
			break;

		klp_for_each_object(patch, obj) {
			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
				continue;

			/*
			 * Only unpatch the module if the patch is enabled or
			 * is in transition.
			 */
			if (patch->enabled || patch == klp_transition_patch) {

				if (patch != klp_transition_patch)
					klp_pre_unpatch_callback(obj);

				pr_notice("reverting patch '%s' on unloading module '%s'\n",
					  patch->mod->name, obj->mod->name);
				klp_unpatch_object(obj);
				klp_post_unpatch_callback(obj);
			}

			klp_free_object_loaded(obj);
			break;
		}
	}
}

Also please make this the first patch in the series. It fixes
an existing problem. We might want to get this in earlier
than the rest of the patchset.

Best Regards,
Petr