re-enable a disabled patch module

[jpoimboe]

Currently KPATCH_FORCE_UNSAFE can be pretty annoying, because you can disable it but you can never rmmod it. Which means that future "kpatch load"s of the same module will always fail. It would be nice if we had the capability to re-enable a disabled but still insmodded patch, to allow re-applying a forced patch.

Not sure what the user interface should look like. Maybe "kpatch enable" which writes 1 to the "enabled" sysfs file. Or another option would be to have "kpatch load" detect somehow that the module file is identical to the one that's already been loaded, and then it would just write to the sysfs file instead of insmod.

[flaming-toast]

another option would be to have "kpatch load" detect somehow that the module file is identical to the one that's already been loaded, and then it would just write to the sysfs file instead of insmod.

Hm, there doesn't seem to be a script-friendly way of determining if a module is already loaded, other than grepping lsmod.

Here's a hacky solution; given a .ko file, you can extract the name of a module that the kernel uses internally in a module's .gnu.linkonce.this_module section, a special section that contains the module's name, and grep for that name in lsmod. Example:

$ readelf -p .gnu.linkonce.this_module kpatch-ext4_sysfs.ko | grep '\[.*\]' | awk '{ print $3 }'
kpatch_ext4_sysfs

You could also use regex on the file argument, but that's less reliable especially if the file has been renamed (Currently, if this happens, it will break kpatch unload). Checking insmod's exit status also doesn't tell us much other than the command failed, unless you want to manually inspect the error message.

Not sure what the user interface should look like. Maybe "kpatch enable" which writes 1 to the "enabled" sysfs file.

I think having an additional kpatch enable/disable command would be useful to have in general, especially for sysadmin-related troubleshooting. kpatch load/unload could then utilize the functions underlying these commands. So for example, load_module could do something like this:

module=$(readelf -p .gnu.linkonce.this_module .... $1)
if lsmod | grep -q $module ; then
     if [[ $(cat /sys/kernel/kpatch/patches/$module/enabled) -eq 0 ]]; then
        echo "Module already loaded, re-enabling..."
        enable_module $module
     else
         # error, already loaded and enabled
else # module is not loaded
      insmod $1

[jpoimboe]

Hm, there doesn't seem to be a script-friendly way of determining if a module is already loaded, other than grepping lsmod.

We could for example test for the existence of /sys/kernel/kpatch/patches/<module name>.

Here's a hacky solution; given a .ko file, you can extract the name of a module that the kernel uses internally in a module's .gnu.linkonce.this_module section, a special section that contains the module's name, and grep for that name in lsmod. Example:

$ readelf -p .gnu.linkonce.this_module kpatch-ext4_sysfs.ko | grep '[.*]' | awk '{ print $3 }'
kpatch_ext4_sysfs

You could also use regex on the file argument, but that's less reliable especially if the file has been renamed (Currently, if this happens, it will break kpatch unload). Checking insmod's exit status also doesn't tell us much other than the command failed, unless you want to manually inspect the error message.

I think having an additional kpatch enable/disable command would be useful to have in general, especially for sysadmin-related troubleshooting

True, but I worry that it would complicate the interfaces. enable/disable would be a subset of load/unload, which I think could be confusing. If a power user really wanted to enable/disable without the corresponding load/unload, they could write to sysfs directly.

. kpatch load/unload could then utilize the functions underlying these commands. So for example, load_module could do something like this:

module=$(readelf -p .gnu.linkonce.this_module .... $1)
if lsmod | grep -q $module ; then
if [[ $(cat /sys/kernel/kpatch/patches/$module/enabled) -eq 0 ]]; then
echo "Module already loaded, re-enabling..."
enable_module $module
else
# error, already loaded and enabled
else # module is not loaded
insmod $1

Yeah, something like that might work.

But... what happens if the filename matches, but the content doesn't? Like if somebody did a load of kpatch-foo.ko, disabled it, and then compiled another kpatch-foo.ko with the same name but using a different patch. Then "kpatch load" would confusingly report that it was successfully loaded, when in fact another module with the same name was enabled.

One option: we could have some kind of module content checksum which we store in a special ELF section which could be read from the loaded module's sysfs and also read from the binary using readelf. If the checksums match, do the above implicit enable-on-load, and otherwise report an error. But maybe that's too much trickery just to achieve the consistent interface? Not really sure what the best solution is. But I think this would be my preferred option atm.

[flaming-toast]

But... what happens if the filename matches, but the content doesn't?

Yikes, yeah. That's a loophole I forgot to consider..

One option: we could have some kind of module content checksum which we store in a special ELF section which could be read from the loaded module's sysfs and also read from the binary using readelf.

I don't think we need to create another ELF section. We could just stick in MODULE_VERSION macros in the patch modules themselves to generate a "srcversion" field in modinfo, which contains the checksum of the source files that made it. (see CONFIG_MODULE_SRCVERSION_ALL for more info on MODULE_VERSION) It'd look something like this:

$ modinfo btrfs
filename:       /lib/modules/2.6.32-22-generic/kernel/fs/btrfs/btrfs.ko
license:        GPL
srcversion:     809EFA84861CF7190ECDB22
depends:        libcrc32c,zlib_deflate
vermagic:       2.6.32-22-generic SMP mod_unload modversions

srcversion will change with the module's source code, so I think this will suit our needs. We could simply compare the checksums with modinfo -F srcversion or cat /sys/module/<module>/srcversion and if they match, re-enable the module, otherwise error.

[jpoimboe]

@flaming-toast Yes! That is so much better. Thanks :-)

[flaming-toast]

@jpoimboe Np!

As for re-enabling the patch module via sysfs, this should be straightforward to do in patch_enabled_store (i.e., if val && !kpmod.enabled, call kpatch_register). But I'm concerned about possible race conditions; we don't want to call kpatch_{register,unregister} while kpmod is actively being manipulated. I'm mostly concerned about patch_init -- what would happen if a call to kpatch_register (via sysfs) were to happen after sysfs_create_file() but before kpmod.objects is fully initialized?

Perhaps we could move the call to sysfs_create_file() after the initial kpatch_register() call in patch_init(), or have kpatch_register() use strong_try_module_get() instead of try_module_get() so it would fail if we're still in the middle of the patch module's initcall.

I was also initially concerned about possible races in patch_exit() (i.e. a call to kpatch_register() made while kpmod.objects is being freed) but try_module_get() should fail if a module's state is exiting.

Am I missing any other potential issues?

[jpoimboe]

Yeah, I think moving the sysfs_create_file() call to after the kpatch_register() call sounds like the best way to do it.

Another issue is that patch_enabled_store() can potentially be called by multiple threads at the same time, so the check of kpmod.enabled in that function is racy. Instead, I think kpatch_register() should ensure that kpmod->enabled is false after calling try_module_get(), rather than its current behavior of initializing it to false (which should be unnecessary: kpmod is statically initialized so all its values default to zero). Also, kpatch_unregister() should do its enabled check after getting the mutex rather than before.

[flaming-toast]

I just tried generating srcversions for some patch modules. It appears that modpost is rejecting the linker script file (kpatch.lds) as it tries to creates the checksum :-( This may require either upstream changes to modpost to support linker files (not backwards-compatible with current kernel releases, should the changes be eventually accepted upstream), or a workaround on our part (simplest way is to not rely on a linker script, but that doesn't seem possible atm), or we could create our own checksum. In any case, this isn't as simple as I thought it'd be. :-(

[jpoimboe]

Bummer, sounds like we need to think about this some more. Thanks for looking at it @flaming-toast.

[jpoimboe]

Side note: I've heard that unloading kernel modules isn't really supported in Linux (even though it's a very common operation). I'm not sure if it's dangerous or what, guess I need to learn more about it. So it's possible that in the future kpatch unload will only disable and will never rmmod. Which will make this feature even more important.

[flaming-toast]

Hmm..I haven't had issues with rmmod yet. I'm concerned though that cruft might build up, as updates are applied over time; we'll have a bunch of patch modules just sitting in memory.

Back to checksumming though. I am very tempted to suggest just running a very stripped down version of modpost (actually, I'd just reuse the checksumming portion of modpost, but have it account for the linker file) that kpatch-build would invoke as the last step in the patch module build process. Either that, or we could write our own helper program that manually 1) calculate the checksum, 2) insert it in an elf section and have the patch module be able to access it, and 3) have the patch module export its checksum via sysfs as well as add a field in modinfo. The first option is easier since we're leveraging existing code and srcversion is automatically exported to sysfs + a field is automatically created in modinfo. The second option might take more thinking and manual work. In any case having a checksum will be useful if we want to differentiate between two modules with the same name, which may be even more important if we eventually plan on not rmmod'ing patch modules.

[jpoimboe]

Hmm..I haven't had issues with rmmod yet. I'm concerned though that cruft might build up, as updates are applied over time; we'll have a bunch of patch modules just sitting in memory.

Yeah, with our current approach to cumulative updates, that could be a problem. @mhiramathitachi (who has a proposal for kpatch without stop_machine) has suggested that we move to incremental updates instead, but to do that properly we'll need to figure out how to safely deal with module dependencies.

Back to checksumming though. I am very tempted to suggest just running a very stripped down version of modpost (actually, I'd just reuse the checksumming portion of modpost, but have it account for the linker file) that kpatch-build would invoke as the last step in the patch module build process. Either that, or we could write our own helper program that manually 1) calculate the checksum, 2) insert it in an elf section and have the patch module be able to access it, and 3) have the patch module export its checksum via sysfs as well as add a field in modinfo. The first option is easier since we're leveraging existing code and srcversion is automatically exported to sysfs + a field is automatically created in modinfo. The second option might take more thinking and manual work.

Yeah, if we can hack up modpost to do what we need, that might be a good solution.

[flaming-toast]

Dug into modpost a bit more. Realized that the checksum generated is purely source based, i.e., if we have two modules built from two different patches but with the same patch module source, the srcversion checksum will be the same (makes sense actually...). So I guess we can't use that approach.

Perhaps we can insert, say, a .kpatch.checksum section in the final output.o object using objcopy --add-section before we build the final patch module. We could have a variable defined in kpatch.lds that then points to the checksum string in .kpatch.checksum. The patch module can access that variable and export the checksum string to sysfs. Thoughts? The upside is that this wouldn't require a lot of code:

# kpatch-build
ld -r -o ../patch/output.o $(find . -name "*.o") >> "$LOGFILE" 2>&1 || die
md5sum ../patch/output.o | awk '{print $1}' > checksum.tmp
objcopy --add-section .kpatch.checksum=checksump.tmp ../patch/output.o
# call make, build the final kpatch-patch.ko module, etc"

[jpoimboe]

And the saga continues :-)

Sounds like a good idea to me...

[flaming-toast]

Heh heh, indeed. I think we're getting close though. Submitted #398