On Mon, Jun 04, 2018 at 01:57:38PM +0200, Miroslav Benes wrote: > 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). Ok, sounds reasonable! > > > If anybody wants to write the patch, please go ahead. Otherwise I will > > get around to it eventually. > > I'll do that. Thanks! -- 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
