kmod: add support for in-kernel livepatch hooks

[joe-lawrence]

Upstream 4.15 kernels will provide support for pre and post (un)livepatch callbacks, inspired by the kpatch load hooks. Add support for them in the livepatch-patch-hook.

[joe-lawrence]

It's not very elegant due to all the copy/paste for pre-patch, post-patch, pre-unpatch, and post-unpatch callback typedefs (code refactoring suggestions welcome!) but this essentially copies the same mechanisms that the kpatch load hooks used. I tested the attached patch against 4.15-rc3 with CONFIG_LIVEPATCH set and then not set to test livepatch and kpatch style modules.

macro-hooks.patch.txt

[euspectre]

I tried to add the patch/unpatch callbacks to both fs/proc/meminfo.c and fs/overlay/super.c and the binary patch fails to build:

./fs/overlayfs/super.o: In function `ovl_fill_super':
/temp/kernel/fs/overlayfs/super.c:1098: multiple definition of `kpatch_post_unpatch_data'
./fs/proc/meminfo.o:(.kpatch.hooks.post_unpatch+0x0): first defined here

Same for the remaining 3 kinds of callbacks.
Perhaps, kpatch_pre_patch_data and such could be renamed to something like _fn##_data or kpatch_data_##_fn to avoid this.

I used kernel 4.14 with all livepatch-related patches backported from 4.15 (and relaxed requirement on the minimum kernel version).

[joe-lawrence]

@euspectre - thanks for testing!

I tried to add the patch/unpatch callbacks to both fs/proc/meminfo.c and fs/overlay/super.c and the binary patch fails to build

So this was testing multiple load hooks within the same livepatch? I don't think I checked that scenario. (BTW, do the KPATCH_LOAD_HOOK macros suffer the same problem?) I'll give the kpatch_data_##_fn rename suggestion a try when I get some time.

[euspectre]

So this was testing multiple load hooks within the same livepatch?

Yes, exactly. Such things are likely when using cumulative patches.

BTW, do the KPATCH_LOAD_HOOK macros suffer the same problem?

Haven't tested that yet, but they might. The macros are very similar.

As for kpatch_data_##_fn - I have just built the binary patch with this change in kpatch-macros.h. Seems to work OK on my machine, both sets of callbacks (for meminfo and overlayfs) are called when needed.

[joe-lawrence]

As for kpatch_data_##_fn - I have just built the binary patch with this change in kpatch-macros.h. Seems to work OK on my machine, both sets of callbacks (for meminfo and overlayfs) are called when needed.

Is overlayfs compiled as a module? I ask because adding ##_fn to the kpatch_data macro does let us compile multiple hooks in a livepatch, but does nothing to prevent two hooks to the same object file. (The __xyz_patchtest macros will fail for two hooks in the same .c file, but won't stop anyone from hooking from two different .c files.) So supporting pre-patch hooks for vmlinux, overlayfs.ko, other mods.ko should be allowed... but how to handle multiple pre-patch hooks for the same klp_object?

I can't think of anyway to fail at kernel build time ... I might have to add an extra check to create-diff-object.c (livepatch build) or worst case livepatch-patch-hook.c (livepatch load).

edit: well create-diff-object.c operates per object file, not per klp_object so I don't think we can check there ... I think I'll modify the assignments in livepatch-patch-hook.c :: patch_set_klp_callbacks() to error if the lobject callback is already set. It would be much nicer to catch this before livepatch load time, but I don't see an easy way at the moment.

[joe-lawrence]

v2:

• Fix build errors are reported by @euspectre, allows for multiple hooks per livepatch
• In the livepatch-hook.c init code, bail out with an error if a hook is already set, disallows multiple hooks against the same object.

Verified with the following test patches:
multiple-hooks.patch.txt
multiple-hooks-nfsd-fail.patch.txt
multiple-hooks-vmlinux-fail.patch.txt

[euspectre]

Is overlayfs compiled as a module?

Yes.

I ask because adding ##_fn to the kpatch_data macro does let us compile multiple hooks in a livepatch, but does nothing to prevent two hooks to the same object file.

Indeed.

Thanks, I'll try this version of the patch when I have time.

So, we cannot have 2 or more sets of hooks for a single klp_object, e.g., for vmlinux, right?

As it is very easy for a patch developer to add the hooks in the different source files of the kernel, I'd say, Documentation/livepatch/callbacks.txt should state that explicitly. Or, perhaps, I missed it there. Anyway, this can be done later.

If I uderstand it correctly, one could still enforce that "one set of hooks per klp_object" rule at build time but that can also be done in some future patch set. Something like a post-build step that reads the appropriate sections of the built patch module and checks the object names for the hooks.

[joe-lawrence]

So, we cannot have 2 or more sets of hooks for a single klp_object, e.g., for vmlinux, right?

Yup, that's accurate.

As it is very easy for a patch developer to add the hooks in the different source files of the kernel, I'd say, Documentation/livepatch/callbacks.txt should state that explicitly.

This is a good point and one that I hit yesterday when I was testing your macro suggestion. (Why not add two hooks to vmlinux, it's copy/paste so easy!) However, the kernel livepatch documentation doesn't have anything to say about kpatch macros, so I think we should keep a disclaimer local to the kpatch notes. That documentation will need to be updated in a follow up patch for the new macros, so I'll make a mental note to add a warning there.

[euspectre]

I have rebuilt the same patch for meminfo & overlay with v2 - works OK for me. Thanks!

I think we should keep a disclaimer local to the kpatch notes.

That will do.

Still, as far as I can see in the definitions of livepatch-related structures in the kernel, klp_object can have only one set of callbacks associated with it (a single field struct klp_callbacks callbacks). So, the restriction actually comes from the kernel, not from the implementation of these macros or the build tools.

[almostivan]

I think it would be interesting to be able to have more than one callback per klp_object hook. For example, for vmlinux, one might easily want to have multiple callbacks, such that they can be separated into separate source files. A way to do this would be to register register a 'generic' callback that walks a list or the section with the all the callbacks and then only calls those callbacks that match the kobject name.

[euspectre]

I think it would be interesting to be able to have more than one callback per klp_object hook.

Probably, yes. That would make it easier to change global variables in the different places of the kernel, for instance.

However, I think, it could be done later, in a separate patchset on top of this one.

[joe-lawrence]

@almostivan , @euspectre : to recap,
In this PR, the kpatch-callbacks are matched 1:1 to livepatch-callbacks. From an execution perspective, following from the kernel's livepatch core out to the callback invocation is pretty straightforward. Since there is only a single callback, a kpatch developer would need to consolidate hooks into a single callback in a single .c file, then add symbol visibility as required (maybe resorting to ugly kallsyms lookup?)

So the suggestion is to allow for multiple callbacks located in potentially multiple files. This makes the kpatch developer's life easier as hooks can be located in their most suitable file(s). However, adding a generic shim to call out to a set of callbacks adds a layer of complexity: livepatch-callbacks invoke the shim, which in turn invokes a set of kpatch-callbacks. Also note from a build perspective, this also adds more modified object files to kpatch-building process. If these are trade offs we'd like to make...

I think we should enforce similar rules as the in-kernel livepatch callbacks themselves -- ie, they all need to be agnostic of each other so we don't have to worry about order of operations (or do we?).

If I remember correctly, the livepatch callbacks are invoked (for patching and unpatching) with a simple klp_for_each_object ... callback() loop. That means klp_object callback order will look like: patching: A, B, C, then unpatching: A', B', C' and not C', B', A'. If callbacks are so complicated that fined-grain ordering is required, I think it may be simplest to go back to a single combined callback that takes care of everything.

As far as implementing this, I need to think a little about the details. One thing that pops out in my mind is how to handle hooks for two similarly named functions from two files. I don't think the current macros in this PR can handle that situation. Maybe adding the filename somehow is sufficient, if not, maybe we add a unique identifier parameter to the macro API and leave it up to the developer. Any ideas?

I also agree with @euspectre here in that perhaps this feature can be done in a follow up commit. Livepatch callbacks are relatively new, introduced upstream in v4.15. It would be nice to maintain a backwards compatible macro API if we do implement multiple hooks.

[almostivan]

If there are two similarly named hooks then the macro can be made 'static' and this will avoid any conflicts. Something like:

static struct kpatch_load kpatch_load_data __section(.kpatch.hooks.load) __used = { ....

In terms of the ordering, a problematic issue, would be if one of the callbacks returns an error, in this case, we may want to undo the affects of previous callbacks. One way to address this might be to allow for 'cleanup' functions for each callback type.

[joe-lawrence]

Here is a hacked up proof of concept that could sit on top of this PR. The feature is not complete yet, but changes are really simple:

• Fix the macro as @almostivan suggested to allow for similar names
• Setup a common set of callbacks for every object we're patching
• Each callback searches its appropriate list for a matching callback
• Pre-patch callback return status is ignored *

* This brings up another implementation wrinkle:

In the livepatch callback space, we consider a klp_object's pre-patch and post-unpatch callbacks counterparts, ie, the latter cleans up after the former. Any pre-patch callback failure halts application of the klp_patch and only the pre-patch callbacks that actually ran successfully will have their post-unpatch callbacks invoked.

How should this work if we support multiple kpatch callbacks? If we want to support a similar model where the pre-patch callback can return status, then there needs to be an association between these callback pairs. Off the top of my head, I'm thinking more macro magic that connects the pre-patch and post-unpatch callbacks. Maybe the KPATCH_PRE_PATCH_HOOK takes an argument that clarifies this relationship? Seems a little ugly from an API point of view.

edit: added sample patch file
multiple-hooks-same-name-vmlinux.patch.txt

[jpoimboe]

I'd prefer to stick with one callback per object for now. The callback semantics are already complicated enough as it is.

If this limitation is shown to be too painful in the real world (which has not yet been shown as far as I can see), then we can revisit the "multiple callbacks per object" discussion and then decide whether it makes sense to do it in livepatch, or emulate it in kpatch.

[jpoimboe]

Despite the comment above this code, they aren't function pointers, they're actually the struct symbol names. Instead of hook_func_ptrs, maybe call them hook_struct_syms?

Also, I think there's a bug here, the names are wrong: kpatch_pre_patch_data was renamed to kpatch_pre_patch_data_##_fn in v2 of the patch.

However, for this code, instead of comparing the struct names, maybe we can just compare sym->sec->name with the hook section names, since they will always be in those sections. That way we don't have to hardcode the struct names at all.

[joe-lawrence]

Yes, you're right about function pointers vs. struct names. Also the ##_fn was an artifact of trying to support multiple pre-patch hooks, so I can fix that bug when I clean that up.

However, for this code, instead of comparing the struct names, maybe we can just compare sym->sec->name with the hook section names, since they will always be in those sections. That way we don't have to hardcode the struct names at all.

It's been a few weeks since I was in this code, so bear with me :) The first part of kpatch_include_hook_elements() rips through the ELF sections looking for the hook sections (comparing to the hook_sections[] strings). That part would stay as is.

The seond part of kpatch_include_hook_elements() strips the hook structures by running through all the ELF symbols and comparing to hook_func_ptrs[] ... so you are suggesting that the second set of strings isn't really necessary as we could just look at sym->sec->name and compare with the hook_sections[] strings?

[jpoimboe]

Right :-)

[jpoimboe]

I think the ##_fn isn't necessary, and instead the struct can be made static.

[euspectre]

If kpatch_{pre|post}_{|un}patch_data become static, we probably need to make sure the compiler won't optimize them away.

I am currently experimenting with the same test patch as above and, it seems, there are no ".kpatch.hooks*" sections in the patched object file. Something like this could help, perhaps:

static struct kpatch_pre_patch kpatch_pre_patch_data __used __section(.kpatch.hooks.pre_patch)

[euspectre]

Yes, __used helped.

[jpoimboe]

What do you think about converging our existing hook code to be more livepatch-esque? For example:

• Get rid of KPATCH_LOAD_HOOK for kpatch.ko in favor of the same KPATCH_{PRE,POST}_PATCH_HOOK interfaces we have for livepatch (and a similar change for the unload hook)

• Rename "hook" to "callback" throughout the code base

I think that would make things less confusing, to have the same terminology used by upstream livepatch, and also to have the same interfaces for kpatch and livepatch.

[joe-lawrence]

Using the same terminology would help. To be clear about the interface, you are suggesting that kpatch deprecate its hooks in lieu of adopting pre-and-post, patch-and-unpatch callbacks like livepatch? I think this would definitely simplify the code (and documentation).

[jpoimboe]

To be clear about the interface, you are suggesting that kpatch deprecate its hooks in lieu of adopting pre-and-post, patch-and-unpatch callbacks like livepatch?

Right.

[joe-lawrence]

Still working on this (at glacial pace, sorry). I've got the API sync'd, however there will be one interesting difference: for traditional kpatches, I don't think we can use the pre-patch status return value since they are invoked by way of module-notifier. We can still check for non-zero status in the kpatch core, but maybe all we can do is issue a WARN on non-zero status? Should we do something similar even in the kpatch pre-patch / vmlinux case?

[jpoimboe]

Still working on this (at glacial pace, sorry). I've got the API sync'd, however there will be one interesting difference: for traditional kpatches, I don't think we can use the pre-patch status return value since they are invoked by way of module-notifier. We can still check for non-zero status in the kpatch core, but maybe all we can do is issue a WARN on non-zero status?

Yeah, I think it makes sense to WARN in the module notifier.

Should we do something similar even in the kpatch pre-patch / vmlinux case?

I don't think so, might as well reject the patch there.

[euspectre]

@joe-lawrence By the way, I have updated your patch a bit, taking the suggestions from this discussion into account. It has been working with my simple tests so far. I did not change the implementation of Kpatch hooks though, only livepatch callbacks.

Here is the modified version of your patch, in case it could be helpful: euspectre@d38a479.

I made the data structures static as suggested, added __used (see above) and updated kpatch_include_hook_elements() to use hook_sections[] only.

[joe-lawrence]

@euspectre thanks for posting the mods.

If you were curious, I pushed up my v3 work-in-progress which I think incorporates those changes. At this point, it can build and run kpatch and livepatches from a similar test patch (see the attachment) using the same macro API. There is a still a lot of copy/paste involved in the code, a few function pointer casts I'm not happy with, and a few "JL TODO" notes where I still need to handle error conditions. And figure out if kpatch callbacks should support the pre-patch return value, as I mentioned earlier.

callbacks.patch.txt

[joe-lawrence]

Coming up for air ...

v3:

• address @jpoimboe 's comments
• s/hook/callback/g
• convert kpatch to use livepatch callbacks
• remove callback lists
• handle pre-patch callback failure

I didn't go too crazy testing, but I did put together a kpatch that adds callbacks to vmlinux and then two modules. The last module to load returns -ENODEV from its pre-patch handler. This test compares handling from already loaded modules (normal patch application) and loading modules (via module notifier). The patch and results attached below.

callbacks.patch.txt
tests.txt

[jpoimboe]

Unlike livepatch, in kpatch, the object->name is vmlinux, instead of blank. So there's no need to check for a NULL name.

[jpoimboe]

Also. the WARN() is a bit drastic, and anyway I think it isn't necessary, since the paths branching to here already did pr_err().

[joe-lawrence]

You're right about object->name, I'll make that change. WARN() - I can drop it, I just thought it would be consistent with the module notifier case.

[jpoimboe]

But it's different from the module notifier case, because here we can recover from the error by rejecting the patch.

[jpoimboe]

Better to do this kpatch_state_finish() before the goto, because otherwise it'll be done twice in the NMI activeness safety check failure case.

[joe-lawrence]

Ah, that's right.

[jpoimboe]

This needs to be done before the state is set to KPATCH_STATE_SUCCESS, which is when the patch removal goes live to NMIs.

[joe-lawrence]

OK - I wasn't too clear about the NMI interaction, but that makes sense.

[joe-lawrence]

BTW, what then happens if we run pre-unpatch callbacks before the NMI check and then the latter fails? We're still patched, but already ran those callbacks.

[jpoimboe]

I guess it's similar to the case in livepatch where the pre-unpatch callback is called, but the unpatching never completes (or is reversed). Since the post-patch callback is the reverse of the pre-unpatch callback, should we call the post-patch callback if the NMI check fails?

[joe-lawrence]

I think so. Just for the record, this is getting weird.

[jpoimboe]

Also need a RHEL 7.5 check

[jpoimboe]

Another place where object->name can never be NULL. There are several more in this function. It would be nice to make kpatch consistent with livepatch there, though maybe that's out of scope for this PR.

[jpoimboe]

Is it me or does this function not actually do anything? :-)

[joe-lawrence]

It's not you... I'm not sure what I was trying to do there, but it doesn't look like anything productive.

[joe-lawrence]

Ugh, actually there's an important side effect of patch_find_object_by_name() and that it calls patch_alloc_new_object() which adds to the patch_objects list. If there are no code changes to those same functions, then the callbacks will be dropped as they never make it onto the patch_objects list.

[jpoimboe]

This is still confusing. patch_find_object_by_name() should be renamed (patch_find_or_add_object?). And I suppose patch_add_callbacks_to_object() also needs to be renamed (and maybe commented?) as it doesn't actually add any callbacks. And why is it split up into two functions anyway (this one and patch_set_klp_callbacks)?

[sm00th]

This seems to require !object->callbacks_enabled check, otherwise we are calling post_unpatch_callback() even in the case when pre_patch_callback() fails which is not consistent with it's behavior in livepatch and kpatch's kpatch_remove_patch().

[joe-lawrence]

If a pre-patch callback fails, the object iterator is re-used in this list traversal (ie, _continue, and _reverse), so it should only go back over the previous objects that did not fail.

I can explain as much in the "run any user-defined post-unpatch callbacks" comment, or just modify the list iteration to go over the entire list, skipping the non-enabled ones.

[sm00th]

Right, that's just me missing the "_continue" part. A comment explaining this might make it easier to spot.

[joe-lawrence]

lightly tested with the same callbacks test patchfile on pre-production RHEL7.5

v4:

• In kpatch, object->name is "vmlinux" and not NULL
• In kpatch_apply_patch(), move the kpatch_state_finish call up into the pre-patch callback loop error case ... so it's not called twice if the NMI activeness check fails
• Expand "run any user-defined post-unpatch callbacks" to indicate that we are going back through the object list
• In kpatch_remove_patch(), move the pre-unpatch callbacks to before the NMI check
• In kpatch_remove_patch(), call the post-patch callbacks if the NMI check fails
• Add RHEL7.5 to list of kernels that HAS_LIVEPATCH_CALLBACKS

[jpoimboe]

I'm not sure about this list_for_each_entry_continue_reverse() thing. If we come here because of an NMI activeness safety error, will the continue still work?

[jpoimboe]

Same question here

[jpoimboe]

This stuff hurts my brain, but I think callbacks_enabled should always be true, unless there was an error with the pre_patch callback, right? So shouldn't callbacks_enabled be set unconditionally before checking for object->pre_patch_callback?

[jpoimboe]

This is still confusing. patch_find_object_by_name() should be renamed (patch_find_or_add_object?). And I suppose patch_add_callbacks_to_object() also needs to be renamed (and maybe commented?) as it doesn't actually add any callbacks. And why is it split up into two functions anyway (this one and patch_set_klp_callbacks)?

[joe-lawrence]

v5:

• kpatch core: Rework the callback execution code, drop the non-intuitive list_for_each_entry_continue_reverse traversal and just iterate straight through the list, even in error paths as livepatch does.

• livepatch hook: Rename patch_add_callbacks_to_object() to add_callbacks_to_patch_objects() and combine with patch_set_klp_callbacks().

[joe-lawrence]

This still probably isn't the most intuitive logic ... but we need to cover a bunch of possibilities, including a patch which doesn't have a pre-patch callback defined. In the end, it distills to : Disable the other callbacks only if this object has a pre-patch callback and it fails.

[joe-lawrence]

It's worth noting that I couldn't use the livepatch notion of post_unpatch_enabled here:

• The kpatch-specific inconsistent NMI check in kpatch_remove_patch() could possibly abort the patch removal and then call the post-patch callbacks.
• Kpatch can't prevent a module from loading, so in kpatch_remove_patch() we need to be careful not to call a pre-unpatch callback for an object who's pre-patch callback failed when its module loaded.

The net effect is that the post-patch, pre-unpatch and post-unpatch callbacks need to test that the pre-patch callback didn't fail.

[jpoimboe]

Looks good to me, though it now has conflicts with the master branch. Also the commit log mentions that the documentation and tests still need to be updated.

[joe-lawrence]

v6:

• fix merge conflicts (f1d71ac changed the kpatch_include_symbol() interface)
• updated the patch author guide, hopefully all English
• modified the Fedora and CentOS integration tests (I think the Debian tests deserve their own update and PR)

[jpoimboe]

Actually I think post-unpatch complements prepatch and pre-unpatch complements post-patch? Otherwise, a big 👍

[joe-lawrence]

Oh right, I've typed this post-pre-callback-hook-unpatch nonsense so long it all starts to run together. Pushed an update to fix that up.

[joe-lawrence]

And big thanks for making it through all these reviews 🍻

[jpoimboe]

Np, I bet you're ready to be done with this feature ;-)

[joe-lawrence]

@euspectre , others, any further comments before I merge?

[euspectre]

Livepatch-related part looks good to me.
I haven't tried the latest variant yet but the previous version of your patchset is not that different w.r.t. livepatch and works well for me. If there are any bugs there, they can be fixed later.

As for the changes to the old Kpatch, I like the idea to have the same callback API as livepatch. But my concern is that these changes break API and ABI of the Kpatch core.

• API: the older source patches that use hooks should be updated so that the new kpatch-build could prepare binary patches from them. Not a big deal in our particular case, because we rarely use load/unload hooks in our ReadyKernel patches for VZ. We rely on atomic replace and that makes it rather complex to use the hooks properly, as you know.

• ABI: this one is more important, from my point of view. If I understand it right, the patches built for the previous revisions of Kpatch core module will fail to load with the new core module, and vice versa. So one needs to make sure the users can either get new-style patches with the new Kpatch core or the old core and the patches compatible with it.

If that is the case, the change in ABI should be indicated somehow, at least. For example, you could bump the version of kpatch in a follow-up commit - that would make it easier to adjust the requirements in the packages with patches, etc.

[euspectre]

@joe-lawrence: Please bump kpatch version or use something else to make it clear that KPatch core ABI has changed. This will help the maintainers of the distro packages a lot.

[joe-lawrence]

@euspectre - no worries, this is tracked as #814 , I think @jpoimboe was hoping to collect any pending PRs that may be closed soon (this week?) into that version, too.

[euspectre]

@joe-lawrence: Thanks! This is really helpful.