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

[Joe Lawrence]

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