Message-ID: <alpine.LSU.2.21.1709111628210.13925@san.suse.cz>
Date:   Tue, 12 Sep 2017 10:53:00 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Joe Lawrence <joe.lawrence@...hat.com>
cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Petr Mladek <pmladek@...e.com>,
        Chris J Arges <chris.j.arges@...onical.com>
Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks


> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
> new file mode 100644
> index 000000000000..689b3f399696
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,594 @@
> +======================
> +(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 callbacks will need to be used in conjunction
> +with memory barriers and kernel synchronization primitives, like
> +mutexes/spinlocks, or even stop_machine(), to avoid concurrency issues.
> +
> +Callbacks differ from existing kernel facilities:
> +
> +  - Module init/exit code doesn't run when disabling and re-enabling a
> +    patch.
> +
> +  - A module notifier can't stop a to-be-patched module from loading.
> +
> +Callbacks are part of the klp_object structure and their implementation
> +is specific to that klp_object.  Other livepatch objects may or may not
> +be patched, irrespective of the target klp_object's current state.
> +
> +Callbacks can be registered for the following livepatch actions:
> +
> +  * Pre-patch    - before a klp_object is patched
> +
> +  * Post-patch   - after a klp_object has been patched and is active
> +                   across all tasks
> +
> +  * Pre-unpatch  - before a klp_object is unpatched (ie, patched code is
> +                   active), used to clean up post-patch callback
> +                   resources
> +
> +  * Post-unpatch - after a klp_object has been patched, all code has
> +                   been restored and no tasks are running patched code,
> +                   used to cleanup pre-patch callback resources
> +
> +Each callback action is optional, omitting one does not preclude
> +specifying any other.  Typical use cases however, pare a pre-patch with

s/pare/pair/ ?

> +a post-unpatch handler and a post-patch with a pre-unpatch handler in
> +symmetry: the patch handler acquires and configures resources and the
> +unpatch handler tears down and releases those same resources.

I think it is more than a typical use case. Test 9 shows that. Pre-unpatch 
callbacks are skipped if a transition is reversed. I don't have a problem 
with that per se, because it seems like a good approach, but maybe we 
should describe it properly here. Am I right?

Anyway, it relates to the next remark just below, which is another rule. 
So it is not totally arbitrary.

> +A callback is only executed if its host klp_object is loaded.  For
> +in-kernel vmlinux targets, this means that callbacks will always execute
> +when a livepatch is enabled/disabled.  For patch target kernel modules,
> +callbacks will only execute if the target module is loaded.  When a
> +module target is (un)loaded, its callbacks will execute only if the
> +livepatch module is enabled.
> +
> +The pre-patch callback, if specified, is expected to return a status
> +code (0 for success, -ERRNO on error).  An error status code indicates
> +to the livepatching core that patching of the current klp_object is not
> +safe and to stop the current patching request.  (When no pre-patch
> +callback is provided, the transition is assumed to be safe.)  If a
> +pre-patch callback returns failure, the kernel's module loader will:
> +
> +  - Refuse to load a livepatch, if the livepatch is loaded after
> +    targeted code.
> +
> +    or:
> +
> +  - Refuse to load a module, if the livepatch was already successfully
> +    loaded.
> +
> +No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> +for a given klp_object if its pre-patch callback returned non-zero
> +status.

Shouldn't this be changed to what Josh proposed? That is

  No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
  for a given klp_object if the object failed to patch, due to a failed
  pre_patch callback or for any other reason.

  If the object did successfully patch, but the patch transition never
  started for some reason (e.g., if another object failed to patch),
  only the post-unpatch callback will be called.

> +Test 1
> +------
> +
> +Test a combination of loading a kernel module and a livepatch that
> +patches a function in the first module.  (Un)load the target module
> +before the livepatch module:
> +
> +- load target module
> +- load livepatch
> +- disable livepatch
> +- unload target module
> +- unload livepatch
> +
> +First load a target module:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-mod.ko
> +  [   34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> +
> +On livepatch enable, before the livepatch transition starts, pre-patch
> +callbacks are executed for vmlinux and livepatch_callbacks_mod (those
> +klp_objects currently loaded).  After klp_objects are patched according
> +to the klp_patch, their post-patch callbacks run and the transition
> +completes:
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko
> +  [   36.503719] livepatch: enabling patch 'livepatch_callbacks_demo'
> +  [   36.504213] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition

s/unpatching/patching/

I guess it is a copy&paste error and you can find it elsewhere too.

Apart from these, the documentation is great!

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..58403a9af54b 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -87,24 +87,49 @@ struct klp_func {
>  	bool transition;
>  };
>  
> +struct klp_object;
> +
> +/**
> + * struct klp_callbacks - pre/post live-(un)patch callback structure
> + * @pre_patch:		executed before code patching
> + * @post_patch:		executed after code patching
> + * @pre_unpatch:	executed before code unpatching
> + * @post_unpatch:	executed after code unpatching
> + *
> + * All callbacks are optional.  Only the pre-patch callback, if provided,
> + * will be unconditionally executed.  If the parent klp_object fails to
> + * patch for any reason, including a non-zero error status returned from
> + * the pre-patch callback, no further callbacks will be executed.
> + */
> +struct klp_callbacks {
> +	int (*pre_patch)(struct klp_object *obj);
> +	void (*post_patch)(struct klp_object *obj);
> +	void (*pre_unpatch)(struct klp_object *obj);
> +	void (*post_unpatch)(struct klp_object *obj);
> +};
> +
>  /**
>   * struct klp_object - kernel object structure for live patching
>   * @name:	module name (or NULL for vmlinux)
>   * @funcs:	function entries for functions to be patched in the object
> + * @callbacks:	functions to be executed pre/post (un)patching
>   * @kobj:	kobject for sysfs resources
>   * @mod:	kernel module associated with the patched object
>   *		(NULL for vmlinux)
>   * @patched:	the object's funcs have been added to the klp_ops list
> + * @callbacks_enabled:	flag indicating if callbacks should be run

"flag indicating if post-unpatch callback should be run" ?

and then we could change the name to something like 
'pre-patch_callback_enabled' (but that's really ugly).

>   */
>  struct klp_object {
>  	/* external */
>  	const char *name;
>  	struct klp_func *funcs;
> +	struct klp_callbacks callbacks;
>  
>  	/* internal */
>  	struct kobject kobj;
>  	struct module *mod;
>  	bool patched;
> +	bool callbacks_enabled;
>  };

How about moving callbacks_enabled to klp_callbacks structure? It belongs 
there. It is true, that we'd mix internal and external members with that.

[...]

> @@ -871,13 +882,27 @@ int klp_module_coming(struct module *mod)
>  			pr_notice("applying patch '%s' to loading module '%s'\n",
>  				  patch->mod->name, obj->mod->name);
>  
> +			ret = klp_pre_patch_callback(obj);
> +			if (ret) {
> +				pr_warn("pre-patch callback failed for object '%s'\n",
> +					obj->name);
> +				goto err;
> +			}

There is a problem here. We cycle through all enabled patches (or 
klp_transition_patch) and call klp_pre_patch_callback() everytime an 
enabled patch contains a patch for a coming module. Now, it can easily 
happen that klp_pre_patch_callback() fails. And not the first one from the 
first relevant patch, but the next one. In that case we need to call 
klp_post_unpatch_callback() for all already processed relevant patches in 
the error path.

Unfortunately, we need to do the same for klp_patch_object() below, 
because there is the same problem and we missed it.

> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
>  					patch->mod->name, obj->mod->name, ret);
> +
> +				if (patch != klp_transition_patch)
> +					klp_post_unpatch_callback(obj);
> +
>  				goto err;

Here.

Could you do it as a part of the patch set (or send it separately), 
please?


> diff --git a/samples/livepatch/livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
> new file mode 100644
> index 000000000000..508fcfba3f22
> --- /dev/null
> +++ b/samples/livepatch/livepatch-callbacks-mod.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (C) 2017 Joe Lawrence <joe.lawrence@...hat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * livepatch-callbacks-mod.c - (un)patching callbacks demo support module
> + *
> + *
> + * Purpose
> + * -------
> + *
> + * Simple module to demonstrate livepatch (un)patching callbacks.
> + *
> + *
> + * Usage
> + * -----
> + *
> + * This module is not intended to be standalone.  See the "Usage"
> + * section of livepatch-callbacks-demo.c.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>


> +#include <linux/workqueue.h>
> +#include <linux/delay.h>

At least these two headers files are not needed here.

> +
> +static int livepatch_callbacks_mod_init(void)
> +{
> +	pr_info("%s\n", __func__);
> +	return 0;
> +}
> +
> +static void livepatch_callbacks_mod_exit(void)
> +{
> +	pr_info("%s\n", __func__);
> +}
> +
> +module_init(livepatch_callbacks_mod_init);
> +module_exit(livepatch_callbacks_mod_exit);
> +MODULE_LICENSE("GPL");

Miroslav

Powered by blists - more mailing lists