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

Miroslav Benes
2017-09-13T07:22:09Z

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