On Thu, Jul 13, 2017 at 08:46:40PM -0500, Josh Poimboeuf wrote: > Date: Thu, 13 Jul 2017 20:46:40 -0500 > From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx> > To: Joe Lawrence <joe.lawrence@xxxxxxxxxx> > Cc: live-patching@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Jessica Yu > <jeyu@xxxxxxxxxx>, Jiri Kosina <jikos@xxxxxxxxxx>, Miroslav Benes > <mbenes@xxxxxxx>, Petr Mladek <pmladek@xxxxxxxx>, Chris J Arges > <chris.j.arges@xxxxxxxxxxxxx> > Subject: Re: [PATCH] livepatch: add (un)patch hooks > User-Agent: Mutt/1.6.0.1 (2016-04-01) > > On Wed, Jul 12, 2017 at 10:10:00AM -0400, Joe Lawrence wrote: > > When the livepatch core executes klp_(un)patch_object, call out to a > > livepatch-module specified array of callback hooks. These hooks provide > > a notification mechanism for livepatch modules when klp_objects are > > (un)patching. This may be most interesting when another kernel module > > is a klp_object target and the livepatch module needs to execute code > > after the target is loaded, but before its module_init code is run. > > And it's also useful for vmlinux. Patch module load/unload is separate > from enable/disable, so the module init/exit functions can't be used for > patch-specific changes (e.g., global data changes). > > > The patch-hook executes right before patching objects and the > > unpatch-hook executes right after unpatching objects. > > > > Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx> > > Thanks for posting it. We found this to be a useful feature in the > past, not quite as useful as shadow data, but still good to have for > certain cases. > > Instead of "load hooks" I think it would be more accurate to call them > "enable/disable hooks". (Maybe "callbacks" would be better than > "hooks"? Not sure...) Hi Josh, I hesitataed in calling them "enable/disable" hooks as I associated those terms at the patch level -- a livepatch might be enabled, but callbacks for a module may not occur until its actually loaded. (I'm fine with whatever is most intuitive to the livepatching collective :) "Callbacks" vs. "hooks" is a good point though, as the latter has negative connotations, especially when callers of this facility will be mostly out of tree. > Even better, we might want to be specific: "pre enable hooks" and "post > disable hooks". (Or "pre patch hooks" and "post unpatch hooks"?) > Because we might eventually decide we need the corresponding "post > enable hooks" and "pre disable hooks" as well. "Pre-patch" and "post-unpatch" are a bit wordy, but a good description. I already felt it was important enough to document the order of operations in the doc file and commit msg, so I like this idea. > For the enable case, I think it would be a nice feature if we checked > the return code and aborted the patching operation on error. I think > that should be easy enough. Yeah, that should be easy. To be specific, you're only talking about the patching operation on the associated klp_object, not the entire klp_patch right? > For the unload case, it's too late to do anything, so I'd say a void > return code would be better. Otherwise it implies that we actually do > something about it. Maybe in that case we can leave it up to the user > to decide whether to print an error or WARN() or whatever. Good point. I can change that in v2. Thanks, -- Joe -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
