kpatch script compatibility with livepatch

[jpoimboe]

Make the kpatch script compatible with both the kpatch core module and livepatch.

This might involve making the kpatch core module sysfs interface identical to the livepatch interface.

[flaming-toast]

It might be easier to just add a MODULE_INFO attribute to each type of patch module to indicate whether or not it is a livepatch or kpatch module. The kpatch script can then act accordingly based on this module attribute via modinfo -F. Then when the patch module loads, export a flag somewhere in /sys/module/$module_name/ to indicate whether or not it is a kpatch or livepatch module, so that the kpatch script can appropriately unload the module. PR to come soon.

[jpoimboe]

A livepatch-enabled kernel already has /sys/kernel/livepatch, so I don't think we need to annotate the patch module itself. If that dir exists, the kpatch script can do the livepatch logic, else do the kpatch logic.

[jpoimboe]

(Similarly, the kpatch core module creates the /sys/kernel/kpatch directory)

That said, it might be nice to have the livepatch interfaces and the kpatch interfaces be as similar as possible, to reduce the burden of supporting both for the foreseeable future.

[joe-lawrence]

/digging up this bug since I hit it using kpatch load on Ubuntu 16.04 (which has CONFIG_LIVEPATCH set.) Keying off of /sys/kernel/{live,k}patch is probably the easiest thing to code into the kpatch script, however I wondering about a situation where a livepatch-enabled kernel has kpatch.ko loaded. Should the kpatch script consider /sys/kernel/livepatch as the overriding patching mechanism? And should kpatch.ko successfully build/load on a livepatch-enabled kernel in the first place?

[jpoimboe]

I wondering about a situation where a livepatch-enabled kernel has kpatch.ko loaded. Should the kpatch script consider /sys/kernel/livepatch as the overriding patching mechanism?

Good question. kpatch-build already decides which type of module to build based on CONFIG_LIVEPATCH, so it shouldn't even be possible for kpatch modules to be built for a livepatch-enabled kernel. And nobody has complained about that. So I think looking at /sys/kernel/livepatch is fine.

In any case kpatch.ko will hopefully be obsolete soon, shortly after the livepatch consistency model gets merged.

And should kpatch.ko successfully build/load on a livepatch-enabled kernel in the first place?

No, if only for consistency with our current policy. Maybe add a CONFIG_LIVEPATCH check with all the other config checks at the top of kmod/core/core.c?

[jpoimboe]

BTW, another reason kpatch.ko shouldn't be supported for livepatch kernels is that we don't want people loading both types of modules on the same system, as fireworks may ensue.

[joe-lawrence]

So a patch to disable the kpatch.ko build when CONFIG_LIVEPATCH to avoid 💣 💥 🔥
And another for kpatch script to determine the sysfs directory (I was thinking of a global $SYSFS, set to /sys/kernel/{live,k}patch accordingly).

[jpoimboe]

Sounds good.

[flaming-toast]

@joe-lawrence Just a heads up, I think there might be some kpatch-specific checks in the kpatch script. The .kpatch.checksum check (verify_module_checksum) comes to mind; even though our kpatch-build script tacks on this section regardless of whether its building a kpatch or livepatch module, livepatch modules currently don't even know about it and don't export such a checksum to sysfs. So, we might also have to backport the checksum sysfs code from kpatch-patch-hook.c to livepatch-patch-hook.c

[jpoimboe]

@flaming-toast If I understand correctly, the checksum sysfs code can't be ported to the livepatch hook because the livepatch sysfs entries are created by the upstream livepatch code.

So then I guess we need to figure out if we care enough about the checksum feature to try to get it ported to upstream livepatch. It's Friday afternoon so I don't have an opinion yet :-)

[flaming-toast]

@jpoimboe Whoops, you are right. So looks like we can't do it from livepatch-patch-hook after all.

I needed to refresh my memory on why we added the checksum section :-) See here: #343 (comment)
I think the checksum was added to provide an extra safety check against the following situation:

1. User runs kpatch load kpatch-foo.ko
2. User runs echo 0 > /sys/kernel/kpatch/patches/foo/enabled
3. User compiles a different patch module also called kpatch-foo.ko
4. User runs kpatch load kpatch-foo.ko # sees different checksum in sysfs, correctly errors out
   So without the checksum, kpatch load could have incorrectly re-enabled the patch module from step 1.

I feel like we only have the checksum for the benefit of our userspace tools (kpatch script), so not sure what upstream would say. We also manually create and insert the checksum in our kpatch-build script, so the manual kbuild approach would miss out (or maybe change to use srcversion there).

[joe-lawrence]

@flaming-toast - does srcversion require the macro in order to show up? I've used it in the past in a DKMS-type module rebuilding script, but I didn't recall having to do anything special to add it (maybe the macros were already in place :)

Regardless of which unique identifier we end up picking, and in the interest of backwards compatibility, should the kpatch script only verify the identifier for those modules that have one? (For example, the livepatch modules in my Ubuntu setup do not have checksum.)

Also does .note.gnu.build-id buy us anything more or less than srcversion ?

[jpoimboe]

If there isn't an easy and simple way to support checksums, I would vote to just drop that feature, and assume that a patch with the same name has the same content. In practice, we do versioning by changing the patch name, and I think that is a common and reasonable approach.

[joe-lawrence]

If comparing checksums only when one is present in sysfs is sufficient (ie, kpatch only, or if livepatch ever adopts one), then this commit should do the trick. At least with this in place, load/install/unload* actions work on both CentOS (kpatch) and Ubuntu (livepatch) installs.

*unload doesn't actually remove livepatch modules... this could be clarified in a warn msg or left in place for when/if livepatch ever adds this functionality.