Re: [PATCH] livepatch: add (un)patch hooks

[Josh Poimboeuf]

On Fri, Jul 14, 2017 at 09:23:29AM -0400, Joe Lawrence wrote:
> > 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 :)

Yeah, "enable/disable" isn't quite right.

But also I think "load" is a bit confusing because it sounds (to me)
like the hooks are called when the *patch* module is loaded.  And in the
case where the hooks are for the vmlinux object, "load" doesn't make
sense.

I think "patch/unpatch hooks" (or "callbacks") would be better.  That
matches our current terminology (and is validated by the fact that the
hooks are applied in klp_{patch,unpatch}_object().

> "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?

Oh, right, I forgot about modules.  We can't stop the module from
loading, so forget that.  Maybe the load hook should just return void.

> > 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.

-- 
Josh