We currently have a bug. When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with: module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' The livepatch module has a relocation which references a symbol in the _previous_ loading of nfsd. When apply_relocate_add() tries to replace the old relocation with a new one, it sees that the previous one is nonzero and it errors out. On ppc64le, we have a similar issue: module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd] livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8) livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd' Possible ways to fix it: 1) Remove the error check in apply_relocate_add(). I don't think we should do this, because the error is actually useful for detecting corrupt modules. And also, powerpc has the similar error so this wouldn't be a universal solution. 2) In klp_unpatch_object(), call an arch-specific arch_unpatch_object() which reverses any arch-specific patching: on x86, clearing all relocation targets to zero; on powerpc, converting the instructions after relative link branches to nops. I don't think we should do this because it's not a global solution and requires fidgety arch-specific patching code. 3) Don't allow patched modules to be removed. I think this makes the most sense. Nobody needs this functionality anyway (right?). Note that the module shouldn't be removable even if the patch is disabled, because you could still have the following scenario: 1) modprobe nfsd 2) patch enable 3) patch disable 4) rmmod nfsd (here the old relocation still exists) 5) patch enable 6) modprobe (error: nonzero relocation target) I vote #3. It could be implemented with a try_module_get() in klp_init_object_loaded(). Then we could get rid of klp_module_going(), which would be fantastic. The module reference could be released in klp_unregister_patch(). Any thoughts? If anybody wants to write the patch, please go ahead. Otherwise I will get around to it eventually. -- 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
