hook unload function doesn't run when module is replaced #456

jpoimboe · 2014-10-07T21:24:30Z

To recreate:

kpatch load kpatch-macro-hooks.ko
kpatch replace kpatch-meminfo-string.ko <-- first patch's hook unload doesn't run

I don't see a straightforward way to fix this without a little bit of redesign. I wonder if we should just get rid of "kpatch replace", because of all the complexity it adds to the code. Is it really important to have an atomic replace for upgrading cumulative modules, vs just doing kpatch unload old.ko; kpatch load new.ko?

The text was updated successfully, but these errors were encountered:

sjenning · 2014-10-07T21:29:04Z

I've never really been a fan of it myself. Unless you have an exploit running continuously on the machine trying to get into the gap between the unload and load, I can't see how an atomic replace adds anything. It would also simplify the core module code to which is an advantage for getting upstream. 👍

jpoimboe · 2014-10-07T21:39:32Z

Unless you have an exploit running continuously on the machine trying to get into the gap between the unload and load

Yeah. It's unlikely, but it is possible, and a small security risk. Which is why I'm kind of on the fence about this one.

Eventually maybe it won't be an issue if we can figure out how to do incremental patching properly.

jpoimboe · 2015-10-29T22:01:45Z

Instead of kpatch replace you can just load a new patch which is a superset of the previous patch. So I think an atomic replace isn't really necessary. I think we should remove it.

"kpatch replace" is complex, buggy, and probably unnecessary. And upstream livepatch has nothing like it. Remove it from the kpatch utility, but leave the infrastructure in place in the patch module and the core module for now. Fixes: dynup#456

This reverts commit 8e8de47. kpatch replace can still be convenient to have to safely replace binary patches. The recommended way is to make sure the newer patch is a superset of the old one (to be exact, the set of functions changed by the newer patch should include the functions changed by the older one). Then one can load the newer patch and leave the older one in memory indefinitely. Not everyone likes this solution it seems. The only issue with 'kpatch replace' I kmow about so far is dynup#456: "hook unload function doesn't run when module is replaced". As long as the unload hooks are not used, this should not be a problem.

This partially reverts the following commit (except the changes in Readme, which are OK): ----------- commit 8e8de47 Author: Josh Poimboeuf <jpoimboe@redhat.com> Date: Mon Nov 16 09:38:38 2015 -0600 kpatch: deprecate the replace command "kpatch replace" is complex, buggy, and probably unnecessary. And upstream livepatch has nothing like it. Remove it from the kpatch utility, but leave the infrastructure in place in the patch module and the core module for now. Fixes: dynup#456 ----------- 'kpatch replace' is crucial when one mostly uses cumulative patches, i.e. the patches that provide all the needed fixes in a single patch module. It is very important to update such patches safely. That is, either the new patch should be active after the update (if it succeeds) or the old one should be used (if the update fails). Unloading of the old patch and then loading the new one is more fragile because loading could fail and the system would be left without patches at all. The recommended workaround was to make sure the newer patch is a superset of the old one. To be exact, the set of functions changed by the newer patch should include all the functions changed by the older one. This is not always possible even when one does not remove fixes from the patch. By the way, "atomic replace" has been proposed for livepatch as well: https://lkml.org/lkml/2018/2/6/152 The only issue with 'kpatch replace' I know about at the moment is dynup#456: "hook unload function doesn't run when module is replaced". The current implementation of "atomic replace" for livepatch has the same issue, by the way. It seems, it is difficult to resolve properly at the kernel side. However, if the patches need load/unload hooks (callbacks), we could probably design these callbacks in our cumulative patched according to the current behaviour of Kpatch/livepatch core. Then, upgrade of our patches would work properly. Downgrade (replacement with a previous version) could be implemented as unload + load, it should be a rare operation anyway.

Atomic replacement of the binary patches results in non-obvious behaviour w.r.t. patch/unpatch callbacks (hooks in the "old" kpatch). That is, when patch1 is atomically replaced with patch2, the unpatch callbacks from patch1 will not be called. It looks like this will remain this way, see the details below: * for the "old" kpatch: dynup#456 * for livepatch: https://lkml.org/lkml/2018/1/31/642 We do need atomic replace in ReadyKernel and we might need patch/unpatch callbacks in some patches. Without the guarantee that unpatch callbacks will be called, we'll have to be careful when using them. If these callbacks are used to clean up something that other callbacks have done, the last patch to unload must do all cleaning up. This should work if we are only replacing our cumulative ReadyKernel patch with a newer version and that version is aware of all previous versions, what is needed to clean up there. However, things become rather difficult if we try to downgrade the patch (the older patch is unlikely to know what needs cleaning up for the newer patch) or when dealing with some unifficial or non-cumulative patches (for the same reason as for downgrade). To sort out this mess somehow, I have rewritten "kpatch replace" command so that atomic replace will only be used to replace an official cumulative ReadyKernel patch with a newer version. This is the most common case and if we decide to use callbacks in ReadyKernel patches, we will be able to design these callbacks accordingly. In all other cases, atomic replace is not used, the previous patches are disabled first, then the new patch is loaded normally. The unpatch callbacks will be called for the previous patches, then patch callbacks will be called for the new patch. This should be OK. So, "kpatch replace" now works as follows: 1. If one tries to use "kpatch replace" to load a non-official or a non-cumulative patch (this is currently detected by the names of patch modules), all currently loaded patches are disabled first. Then the new patch is loaded normally. 2. If one tries to load an official cumulative patch, all loaded patches except the official cumulative patches with smaller version-release are disabled first. Then atomic replace is used. If the new patch fails to load, the previously disabled patches will remain disabled. If it was not a patch upgrade operation, the system would end up having no active binary patches at all. Far from ideal but the risk seems acceptable. Downgrade and operations with non-official patches should be rare anyway. I reiterate: if the patch is upgraded, everything should work as before. If "kpatch replace" succeeds, the new patch will be loaded and active. Otherwise the older patch will remain loaded and active. By the way, all this complexity indicates that we should consider using callbacks in the most important fixes only and only if there is no other acceptable way to provide a fix. Done in the scope of https://jira.sw.ru/browse/PSBM-79803.

jpoimboe mentioned this issue Nov 16, 2015

kpatch: deprecate the replace command #557

Merged

sjenning closed this as completed in #557 Nov 16, 2015

hook unload function doesn't run when module is replaced #456

hook unload function doesn't run when module is replaced #456

jpoimboe commented Oct 7, 2014

sjenning commented Oct 7, 2014

jpoimboe commented Oct 7, 2014

jpoimboe commented Oct 29, 2015

hook unload function doesn't run when module is replaced #456

hook unload function doesn't run when module is replaced #456

Comments

jpoimboe commented Oct 7, 2014

sjenning commented Oct 7, 2014

jpoimboe commented Oct 7, 2014

jpoimboe commented Oct 29, 2015