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

[Miroslav Benes]

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