Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730745 > unrolled thread

Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Miroslav Benes <mbenes@suse.cz> - 2017-09-12 11:00 +0200
    Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-12 17:50 +0200
      Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-13 00:10 +0200
        Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-09-13 00:30 +0200
          Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Miroslav Benes <mbenes@suse.cz> - 2017-09-13 09:30 +0200
            Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-13 16:00 +0200
              [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-13 23:40 +0200
                Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming  fails Miroslav Benes <mbenes@suse.cz> - 2017-09-20 13:20 +0200
                  Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming  fails Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-20 17:20 +0200
      Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Miroslav Benes <mbenes@suse.cz> - 2017-09-13 09:30 +0200

#1730745 — 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@redhat.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

[toc] | [next] | [standalone]

#1730962

On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> 
>> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
>> ...
>> +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/ ?

Yup.  At least I didn't use "pear" :)

>> +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?

I think the text was a little fuzzy in regard to what "typical" was
referring to.  How about this edit:

--
Each callback is optional, omitting one does not preclude specifying any
other.  However, the livepatching core executes the handlers in
symmetry: pre-patch callbacks have a post-patch counterpart and
post-patch callbacks have a pre-unpatch counterpart.  An unpatch
callback will only be executed if its corresponding patch callback was
executed.  Typical use cases pair a patch handler that acquires and
configures resources with an unpatch handler tears down and releases
those same resources.
--

Does that clarify that the execution symmetry is fixed and that
implementing callbacks with that property in mind is up to the caller?

More on the reversed transition comment below ...

> 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.

Yeah, I thought I added to the doc, but apparently only coded it.  In
between these two sentences I'd like to include your suggestion about a
reversed-transition:

--
If a patch transition is reversed, no pre-unpatch handlers will be run
(this follows the previously mentioned symmetry -- pre-unpatch callbacks
will only occur if their corresponding post-patch callback executed).
--

I think it fits better down here with the collection of misc. rules and
notes.

>> +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.

Oh no!  This is a actually a bug from patch 3:

  void klp_init_transition(struct klp_patch *patch, int state)
  {
          ...

  	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);

  	pr_debug("'%s': initializing %s transition\n", patch->mod->name,
  		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");

Wow, that debug msg is going to be very confusing.  I can move this
down, or just print the target "state" as passed into the function.

> 
> Apart from these, the documentation is great!

Thanks, I find the test cases / doc more work than actually writing the
code.  So many combinations and corner cases to such a simple idea.

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

Since we removed all the extraneous checks (for post-patch and
pre-unpatch) against this value, it's probably clearest to rename it
"post_unpatch_callback_enabled".

Initially I preferred leaving the callbacks_enabled check in every
callback execution wrapper, but if those callers will be guaranteed not
to ever invoke these routines in the wrong contexts, then it's probably
clearest to call out "post-unpatch" in its name.

>>   */
>>  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.
> 
> [...]

No strong preferences here.  It's simple enough to change.  And it would
reduce the enable flag above to "post_unpatch_enabled"

>> @@ -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.

Good test case, if I understand you correctly:

 - Load target modules mod1 and mod2
 - Load a livepatch that targets mod1 and mod2
   - pre-patch succeeds for mod1
   - pre-patch fails for mod2

and then we should:

 - NOT run post-patch or pre/post-unpatch handlers for mod2
 - NOT run post-patch or pre-unpatch handlers for mod1
 - do run post-unpatch handler for mod1
 - Refuse to load the livepatch

Does that sound right?

> 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?

I can spin a v6... hopefully it's getting close to merge-able :)

>> diff --git a/samples/livepatch/livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
>> ...
> 
>> +#include <linux/workqueue.h>
>> +#include <linux/delay.h>
> 
> At least these two headers files are not needed here.

Removed, thanks.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1731256

On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> >> @@ -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.
> 
> Good test case, if I understand you correctly:
> 
>  - Load target modules mod1 and mod2
>  - Load a livepatch that targets mod1 and mod2
>    - pre-patch succeeds for mod1
>    - pre-patch fails for mod2
> 
> and then we should:
> 
>  - NOT run post-patch or pre/post-unpatch handlers for mod2
>  - NOT run post-patch or pre-unpatch handlers for mod1
>  - do run post-unpatch handler for mod1
>  - Refuse to load the livepatch
> 
> Does that sound right?

Erm, probably not...

> > 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?

I've re-read this a few times, and I think I might have been originally
off-base with what I thought you were concerned about.  But I think I
grok it now: the problem you pointed out arises because
klp_module_coming() iterates like so:

  for each klp_patch
    for each kobj in klp_patch

which means that we may have made pre-patch callbacks and patched a
given kobj for an earlier klp_patch that now fails for a later
klp_patch.

What should be the defined behavior in this case?  I would expect that
we need to unpatch all similar kobjs across klp_patches which have
already been successfully patched.  In turn, their post-unpatch
callbacks should be invoked.

If that's true, maybe this would make a better follow-on patch.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1731268

On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> > On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> > >> @@ -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.
> > 
> > Good test case, if I understand you correctly:
> > 
> >  - Load target modules mod1 and mod2
> >  - Load a livepatch that targets mod1 and mod2
> >    - pre-patch succeeds for mod1
> >    - pre-patch fails for mod2
> > 
> > and then we should:
> > 
> >  - NOT run post-patch or pre/post-unpatch handlers for mod2
> >  - NOT run post-patch or pre-unpatch handlers for mod1
> >  - do run post-unpatch handler for mod1
> >  - Refuse to load the livepatch
> > 
> > Does that sound right?
> 
> Erm, probably not...
> 
> > > 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?
> 
> I've re-read this a few times, and I think I might have been originally
> off-base with what I thought you were concerned about.  But I think I
> grok it now: the problem you pointed out arises because
> klp_module_coming() iterates like so:
> 
>   for each klp_patch
>     for each kobj in klp_patch
> 
> which means that we may have made pre-patch callbacks and patched a
> given kobj for an earlier klp_patch that now fails for a later
> klp_patch.
> 
> What should be the defined behavior in this case?  I would expect that
> we need to unpatch all similar kobjs across klp_patches which have
> already been successfully patched.  In turn, their post-unpatch
> callbacks should be invoked.
> 
> If that's true, maybe this would make a better follow-on patch.

The rabbit hole seems to be getting deeper, is it really worth it?  I'd
rather we just make the pre-patch handler return void and be done with
it, as Joe originally proposed.

So far, allowing the pre-patch handler to halt patching is a purely
theoretical feature, nobody even knows if we need it yet, and whether
it's worth the pain.  So I'd vote to just simplify this mess and let
whoever wants the feature try to implement it :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1731406

On Tue, 12 Sep 2017, Josh Poimboeuf wrote:

> On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
> > On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> > > On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> > > >> @@ -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.
> > > 
> > > Good test case, if I understand you correctly:
> > > 
> > >  - Load target modules mod1 and mod2
> > >  - Load a livepatch that targets mod1 and mod2
> > >    - pre-patch succeeds for mod1
> > >    - pre-patch fails for mod2
> > > 
> > > and then we should:
> > > 
> > >  - NOT run post-patch or pre/post-unpatch handlers for mod2
> > >  - NOT run post-patch or pre-unpatch handlers for mod1
> > >  - do run post-unpatch handler for mod1
> > >  - Refuse to load the livepatch
> > > 
> > > Does that sound right?
> > 
> > Erm, probably not...
> > 
> > > > 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?
> > 
> > I've re-read this a few times, and I think I might have been originally
> > off-base with what I thought you were concerned about.  But I think I
> > grok it now: the problem you pointed out arises because
> > klp_module_coming() iterates like so:
> > 
> >   for each klp_patch
> >     for each kobj in klp_patch
> > 
> > which means that we may have made pre-patch callbacks and patched a
> > given kobj for an earlier klp_patch that now fails for a later
> > klp_patch.

Yes, that's the scenario.
 
> > What should be the defined behavior in this case?  I would expect that
> > we need to unpatch all similar kobjs across klp_patches which have
> > already been successfully patched.  In turn, their post-unpatch
> > callbacks should be invoked.
> > 
> > If that's true, maybe this would make a better follow-on patch.

Yes, you'd need to loop back, unpatch everything and call post-unpatch 
callbacks too. Probably too much for this patch set, so we can deal with 
the problem later.

> The rabbit hole seems to be getting deeper, is it really worth it?  I'd
> rather we just make the pre-patch handler return void and be done with
> it, as Joe originally proposed.
> 
> So far, allowing the pre-patch handler to halt patching is a purely
> theoretical feature, nobody even knows if we need it yet, and whether
> it's worth the pain.  So I'd vote to just simplify this mess and let
> whoever wants the feature try to implement it :-)

Unfortunately, the problem is there even without Joe's callbacks. If it 
was only a problem of callbacks, I'd go along with you. I see two options.

1. we'll fix this for klp_patch_object(). Then callbacks' problem would be 
simple to solve, because the infrastructure would be already there.

2. we'll remove any error handling from klp_coming_module and we'll allow 
target modules to load even with a patching failure. This doesn't seem to 
be the right approach...

Miroslav

[toc] | [prev] | [next] | [standalone]

#1731617

On 09/13/2017 03:29 AM, Miroslav Benes wrote:
> On Tue, 12 Sep 2017, Josh Poimboeuf wrote:
> 
>> On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
>>> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
>>> I've re-read this a few times, and I think I might have been originally
>>> off-base with what I thought you were concerned about.  But I think I
>>> grok it now: the problem you pointed out arises because
>>> klp_module_coming() iterates like so:
>>>
>>>   for each klp_patch
>>>     for each kobj in klp_patch
>>>
>>> which means that we may have made pre-patch callbacks and patched a
>>> given kobj for an earlier klp_patch that now fails for a later
>>> klp_patch.
> 
> Yes, that's the scenario.
>  
>>> What should be the defined behavior in this case?  I would expect that
>>> we need to unpatch all similar kobjs across klp_patches which have
>>> already been successfully patched.  In turn, their post-unpatch
>>> callbacks should be invoked.
>>>
>>> If that's true, maybe this would make a better follow-on patch.
> 
> Yes, you'd need to loop back, unpatch everything and call post-unpatch 
> callbacks too. Probably too much for this patch set, so we can deal with 
> the problem later.

Completely untested/compiled, but something like (sans callbacks)?

--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -894,6 +894,29 @@ 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);
+
+	/*
+	 * Run back through the patch list and unpatch any klp_object
+	 * that matches the one we errored on above.
+	 */
+	list_for_each_entry(patch, &klp_patches, list) {
+
+		if (!patch->enabled || patch == klp_transition_patch)
+			continue;
+
+		klp_for_each_object(patch, obj) {
+
+			if (!obj->patched ||
+			    !klp_is_module(obj) ||
+			    strcmp(obj->name, mod->name))
+				continue;
+
+			klp_unpatch_object(obj);
+			/* post-unpatch callback would go here */
+
+			break;
+		}
+	}
+
 	mod->klp_alive = false;
 	klp_free_object_loaded(obj);
 	mutex_unlock(&klp_mutex);

>> The rabbit hole seems to be getting deeper, is it really worth it?  I'd
>> rather we just make the pre-patch handler return void and be done with
>> it, as Joe originally proposed.
>>
>> So far, allowing the pre-patch handler to halt patching is a purely
>> theoretical feature, nobody even knows if we need it yet, and whether
>> it's worth the pain.  So I'd vote to just simplify this mess and let
>> whoever wants the feature try to implement it :-)

"Rabbit hole" is an apt description :) the question is whether this is
the last hurdle, or just one of many more coming our way.  I'd like to
think the former, but I'm the guy down in the rabbit hole, so my
perspective is tainted.

Ripping this out of the code would be relatively easy.  Re-doing the
tests/documentation/comments will be a bit of work to reword everything.

> Unfortunately, the problem is there even without Joe's callbacks. If it 
> was only a problem of callbacks, I'd go along with you. I see two options.
> 
> 1. we'll fix this for klp_patch_object(). Then callbacks' problem would be 
> simple to solve, because the infrastructure would be already there.

If the bugfix is like the above, then it's not too bad a diversion.  I
can run some tests this afternoon to try and tackle this.

> 2. we'll remove any error handling from klp_coming_module and we'll allow 
> target modules to load even with a patching failure. This doesn't seem to 
> be the right approach...
I agree.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1731910 — [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

Hi Miroslav,

I worked out the code that I posted earlier today and I think this could
address the multiple-patch module_coming() issue you pointed out.

Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
callbacks" patchset, so it includes unpatching callbacks.  I can easily
strip those out (and remove the additional debugging pr_'s) and make
this a stand-alone patch that would apply before the callback patchset.

See the test case below.

-- Joe

Test X
------

Multiple livepatches targeting the same klp_objects may be loaded at
the same time.  If a target module loads and any of the livepatch's
pre-patch callbacks fail, then the module is not allowed to load.
Furthermore, any livepatches that that did succeed will be reverted
(only the incoming module / klp_object) and their pre/post-unpatch
callbacks executed.

  - load livepatch
  - load livepatch2
  - load livepatch3
  - setup livepatch3 pre-patch return of -ENODEV
  - load target module (should fail)
  - disable livepatch3
  - disable livepatch2
  - disable livepatch
  - unload livepatch3
  - unload livepatch2
  - unload livepatch


Load three livepatches, each target a livepatch_callbacks_mod module and
vmlinux:

  % insmod samples/livepatch/livepatch-callbacks-demo.ko 
  [   26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
  [   26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
  [   26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
  [   26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
  [   27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
  [   27.744130] livepatch: 'livepatch_callbacks_demo': patching complete

  % insmod samples/livepatch/livepatch-callbacks-demo2.ko 
  [   29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
  [   29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
  [   29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
  [   30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
  [   30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete

  % insmod samples/livepatch/livepatch-callbacks-demo3.ko 
  [   32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
  [   32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
  [   32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
  [   33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
  [   33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete

Setup the third livepatch to fail its pre-patch callback when the target
module is loaded:

  % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret

Load the target module:

  % insmod samples/livepatch/livepatch-callbacks-mod.ko 

The first livepatch pre-patch callback succeeds, the klp_object is
patched, and its post-patch callback is executed:

  [   38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
  [   38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
  [   38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

Likewise for the second livepatch:

  [   38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
  [   38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
  [   38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

But the third livepatch fails its pre-patch callback:

  [   38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
  [   38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'

We refuse to load the target module:

  [   38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'

So we double back and unpatch (including pre-unpatch and post-unpatch
callbacks) the first livepatch, then the second:

  [   38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
  [   38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
  [   38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
  [   38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

Finally the module loader reports an error:

  [   38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device

Clean it all up:

  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
  [   41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
  [   41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
  [   42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
  [   42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
  
  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
  [   47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
  [   47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
  [   48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
  [   48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete

  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
  [   53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
  [   53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
  [   54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
  [   54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete

  % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
  % rmmod samples/livepatch/livepatch-callbacks-demo.ko


-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

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_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;
+		}
+	}
+
 	mutex_unlock(&klp_mutex);
 
 	return ret;
-- 
2.7.5

[toc] | [prev] | [next] | [standalone]

#1735724 — Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

On Wed, 13 Sep 2017, Joe Lawrence wrote:

> Hi Miroslav,

Hi,

sorry for the late response. I'm also travelling now and we have SUSECon 
conference next week, so just a quick answer. It looks ok at first glance, 
but I need to take a proper look.

> I worked out the code that I posted earlier today and I think this could
> address the multiple-patch module_coming() issue you pointed out.
> 
> Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> callbacks" patchset, so it includes unpatching callbacks.  I can easily
> strip those out (and remove the additional debugging pr_'s) and make
> this a stand-alone patch that would apply before the callback patchset.

I think this would be better. Strip callbacks out and send this either 
separately (and base callbacks patch set on this), or make it 1/n of the 
series.

> See the test case below.
> 
> -- Joe
> 
> Test X
> ------
> 
> Multiple livepatches targeting the same klp_objects may be loaded at
> the same time.  If a target module loads and any of the livepatch's
> pre-patch callbacks fail, then the module is not allowed to load.
> Furthermore, any livepatches that that did succeed will be reverted
> (only the incoming module / klp_object) and their pre/post-unpatch
> callbacks executed.
> 
>   - load livepatch
>   - load livepatch2
>   - load livepatch3
>   - setup livepatch3 pre-patch return of -ENODEV
>   - load target module (should fail)
>   - disable livepatch3
>   - disable livepatch2
>   - disable livepatch
>   - unload livepatch3
>   - unload livepatch2
>   - unload livepatch
> 
> 
> Load three livepatches, each target a livepatch_callbacks_mod module and
> vmlinux:
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo.ko 
>   [   26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
>   [   26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
>   [   26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
>   [   26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
>   [   27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
>   [   27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo2.ko 
>   [   29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
>   [   29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
>   [   29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
>   [   30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
>   [   30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo3.ko 
>   [   32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
>   [   32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
>   [   32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
>   [   33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
>   [   33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
> 
> Setup the third livepatch to fail its pre-patch callback when the target
> module is loaded:
> 
>   % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
> 
> Load the target module:
> 
>   % insmod samples/livepatch/livepatch-callbacks-mod.ko 
> 
> The first livepatch pre-patch callback succeeds, the klp_object is
> patched, and its post-patch callback is executed:
> 
>   [   38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
>   [   38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
>   [   38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> Likewise for the second livepatch:
> 
>   [   38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
>   [   38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
>   [   38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> But the third livepatch fails its pre-patch callback:
> 
>   [   38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
>   [   38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
> 
> We refuse to load the target module:
> 
>   [   38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
> 
> So we double back and unpatch (including pre-unpatch and post-unpatch
> callbacks) the first livepatch, then the second:
> 
>   [   38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
>   [   38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
>   [   38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> Finally the module loader reports an error:
> 
>   [   38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
> 
> Clean it all up:
> 
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
>   [   41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
>   [   41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
>   [   42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
>   [   42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
>   
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
>   [   47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
>   [   47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
>   [   48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
>   [   48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> 
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
>   [   53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
>   [   53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
>   [   54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
>   [   54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
> 
>   % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
>   % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
>   % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> 
> 
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> 
> >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) {

I think it would be safer to use 
list_for_each_entry_{continue,from}_reverse() iterator (probably 
_continue_reverse(), because the current patch failed). That would unpatch 
the objects in the correct order (see your test case above) and it is 
also an optimization because you'd process only those patches which were 
walked through during the first loop.

> +
> +		if (!patch->enabled || patch == klp_transition_patch)
> +			continue;

Is the second part with klp_transition_patch correct? Yes, we need to skip 
disabled patches. No question about that. But klp_transition_patch seems 
odd. It is true, that (if I am not mistaken) klp_transition_patch is the 
last patch in patches list which is relevant (because we cannot 
enable/disable any random patch in the list). If that failed to patch, 
you wouldn't need to worry about it anyway, because you need to process 
previous patches only. Am I missing something? So I think it can stay, 
yes. But I'd like to understand it.

Miroslav

> +		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;
> +		}
> +	}
> +
>  	mutex_unlock(&klp_mutex);
>  
>  	return ret;
> -- 
> 2.7.5
> 

[toc] | [prev] | [next] | [standalone]

#1735825 — Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

On Wed, Sep 20, 2017 at 01:19:05PM +0200, Miroslav Benes wrote:
> On Wed, 13 Sep 2017, Joe Lawrence wrote:
> 
> > Hi Miroslav,
> 
> Hi,
> 
> sorry for the late response. I'm also travelling now and we have SUSECon 
> conference next week, so just a quick answer. It looks ok at first glance, 
> but I need to take a proper look.

No problem, thanks for the update and safe travels.
 
> > I worked out the code that I posted earlier today and I think this could
> > address the multiple-patch module_coming() issue you pointed out.
> > 
> > Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> > callbacks" patchset, so it includes unpatching callbacks.  I can easily
> > strip those out (and remove the additional debugging pr_'s) and make
> > this a stand-alone patch that would apply before the callback patchset.
> 
> I think this would be better. Strip callbacks out and send this either 
> separately (and base callbacks patch set on this), or make it 1/n of the 
> series.

Agreed.

> > See the test case below.
> > 
> > -- Joe
> > 
> > Test X
> > ------
> > 
> > Multiple livepatches targeting the same klp_objects may be loaded at
> > the same time.  If a target module loads and any of the livepatch's
> > pre-patch callbacks fail, then the module is not allowed to load.
> > Furthermore, any livepatches that that did succeed will be reverted
> > (only the incoming module / klp_object) and their pre/post-unpatch
> > callbacks executed.
> > 
> >   - load livepatch
> >   - load livepatch2
> >   - load livepatch3
> >   - setup livepatch3 pre-patch return of -ENODEV
> >   - load target module (should fail)
> >   - disable livepatch3
> >   - disable livepatch2
> >   - disable livepatch
> >   - unload livepatch3
> >   - unload livepatch2
> >   - unload livepatch
> > 
> > 
> > Load three livepatches, each target a livepatch_callbacks_mod module and
> > vmlinux:
> > 
> >   % insmod samples/livepatch/livepatch-callbacks-demo.ko 
> >   [   26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
> >   [   26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
> >   [   26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> >   [   26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
> >   [   27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
> >   [   27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
> > 
> >   % insmod samples/livepatch/livepatch-callbacks-demo2.ko 
> >   [   29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
> >   [   29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> >   [   29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> >   [   30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> >   [   30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
> > 
> >   % insmod samples/livepatch/livepatch-callbacks-demo3.ko 
> >   [   32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
> >   [   32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
> >   [   32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
> >   [   33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
> >   [   33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
> > 
> > Setup the third livepatch to fail its pre-patch callback when the target
> > module is loaded:
> > 
> >   % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
> > 
> > Load the target module:
> > 
> >   % insmod samples/livepatch/livepatch-callbacks-mod.ko 
> > 
> > The first livepatch pre-patch callback succeeds, the klp_object is
> > patched, and its post-patch callback is executed:
> > 
> >   [   38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
> >   [   38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> >   [   38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > 
> > Likewise for the second livepatch:
> > 
> >   [   38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
> >   [   38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> >   [   38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > 
> > But the third livepatch fails its pre-patch callback:
> > 
> >   [   38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
> >   [   38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
> > 
> > We refuse to load the target module:
> > 
> >   [   38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
> > 
> > So we double back and unpatch (including pre-unpatch and post-unpatch
> > callbacks) the first livepatch, then the second:
> > 
> >   [   38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> >   [   38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >   [   38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> >   [   38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > 
> > Finally the module loader reports an error:
> > 
> >   [   38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
> > 
> > Clean it all up:
> > 
> >   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
> >   [   41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
> >   [   41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
> >   [   42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
> >   [   42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
> >   
> >   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> >   [   47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
> >   [   47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> >   [   48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
> >   [   48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> > 
> >   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
> >   [   53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
> >   [   53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
> >   [   54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
> >   [   54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
> > 
> >   % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
> >   % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> >   % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> > 
> > 
> > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> > 
> > >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) {
> 
> I think it would be safer to use 
> list_for_each_entry_{continue,from}_reverse() iterator (probably 
> _continue_reverse(), because the current patch failed). That would unpatch 
> the objects in the correct order (see your test case above) and it is 
> also an optimization because you'd process only those patches which were 
> walked through during the first loop.

I had originally tested with list_for_each_entry_reverse(), but then
noticed that klp_module_going() iterates through the patches using
list_for_each_entry().  Strictly speaking, there is also the matter of
the klp_objects, but we don't have a klp_for_each_object_reverse() macro
that would complete the mirrored-symmetry.

For pre/post-(un)patch callbacks, they are supposed to be independent
from each other anyway, so theoretically their execution order shouldn't
matter.

That said, maybe we can compromise on list_for_each_entry_reverse() for
both klp_module_going() and the klp_module_coming() error path above?

> > +
> > +		if (!patch->enabled || patch == klp_transition_patch)
> > +			continue;
> 
> Is the second part with klp_transition_patch correct? Yes, we need to skip 
> disabled patches. No question about that. But klp_transition_patch seems 
> odd. It is true, that (if I am not mistaken) klp_transition_patch is the 
> last patch in patches list which is relevant (because we cannot 
> enable/disable any random patch in the list). If that failed to patch, 
> you wouldn't need to worry about it anyway, because you need to process 
> previous patches only. Am I missing something? So I think it can stay, 
> yes. But I'd like to understand it.

You might be correct here, I basically copy/pasted it from the code
above with the understanding that the klp_transition_patch was handled
by klp_complete_transition().  If it is an unneeded check, then I can
remove it.

Thanks,

-- Joe 

[toc] | [prev] | [next] | [standalone]

#1731405

On Tue, 12 Sep 2017, Joe Lawrence wrote:

> On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> 
> >> +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?
> 
> I think the text was a little fuzzy in regard to what "typical" was
> referring to.  How about this edit:
> 
> --
> Each callback is optional, omitting one does not preclude specifying any
> other.  However, the livepatching core executes the handlers in
> symmetry: pre-patch callbacks have a post-patch counterpart and

s/post-patch/post-unpatch/

> post-patch callbacks have a pre-unpatch counterpart.  An unpatch
> callback will only be executed if its corresponding patch callback was
> executed.  Typical use cases pair a patch handler that acquires and
> configures resources with an unpatch handler tears down and releases
> those same resources.
> --
> 
> Does that clarify that the execution symmetry is fixed and that
> implementing callbacks with that property in mind is up to the caller?

Yes, thank you.
 
> More on the reversed transition comment below ...
> 
> > 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.
> 
> Yeah, I thought I added to the doc, but apparently only coded it.  In
> between these two sentences I'd like to include your suggestion about a
> reversed-transition:
> 
> --
> If a patch transition is reversed, no pre-unpatch handlers will be run
> (this follows the previously mentioned symmetry -- pre-unpatch callbacks
> will only occur if their corresponding post-patch callback executed).
> --
> 
> I think it fits better down here with the collection of misc. rules and
> notes.

Yes.
 
> >> +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.
> 
> Oh no!  This is a actually a bug from patch 3:
> 
>   void klp_init_transition(struct klp_patch *patch, int state)
>   {
>           ...
> 
>   	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
> 
>   	pr_debug("'%s': initializing %s transition\n", patch->mod->name,
>   		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> 
> Wow, that debug msg is going to be very confusing.  I can move this
> down, or just print the target "state" as passed into the function.

Oh, it is a bug. You're right. I'd move it down. Target state could 
cause confusion. User shouldn't need to know anything about live patching 
internals.
 
> > 
> > Apart from these, the documentation is great!
> 
> Thanks, I find the test cases / doc more work than actually writing the
> code.  So many combinations and corner cases to such a simple idea.
> 
> > 
> >> 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).
> 
> Since we removed all the extraneous checks (for post-patch and
> pre-unpatch) against this value, it's probably clearest to rename it
> "post_unpatch_callback_enabled".
> 
> Initially I preferred leaving the callbacks_enabled check in every
> callback execution wrapper, but if those callers will be guaranteed not
> to ever invoke these routines in the wrong contexts, then it's probably
> clearest to call out "post-unpatch" in its name.
> 
> >>   */
> >>  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.
> > 
> > [...]
> 
> No strong preferences here.  It's simple enough to change.  And it would
> reduce the enable flag above to "post_unpatch_enabled"

If everyone agrees, I'd move it to klp_callbacks structure and call it as 
you propose.
 
Thanks,
Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web