Hi, On Sat, 2 Jun 2018, Josh Poimboeuf wrote: > 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. That's the solution which I immediately thought about. > 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? Our customers definitely unload modules. I can only speculate on why and how often. I do not want to break any reasonable use case and in that regard I'd go with 2), but arch-specific code is not the best either. So we could do 3) and if it broke something somewhere (hopefully not), we could implement 2) (or similar). > If anybody wants to write the patch, please go ahead. Otherwise I will > get around to it eventually. I'll do that. Thanks, Miroslav -- 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
