Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751499AbdIMHWJ (ORCPT ); Wed, 13 Sep 2017 03:22:09 -0400 Received: from mx2.suse.de ([195.135.220.15]:49970 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750949AbdIMHWI (ORCPT ); Wed, 13 Sep 2017 03:22:08 -0400 Date: Wed, 13 Sep 2017 09:22:06 +0200 (CEST) From: Miroslav Benes To: Joe Lawrence cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Petr Mladek , Chris J Arges Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks In-Reply-To: Message-ID: References: <1504191233-2642-1-git-send-email-joe.lawrence@redhat.com> <1504191233-2642-2-git-send-email-joe.lawrence@redhat.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9016 Lines: 229 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