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