On Fri, Jun 30, 2017 at 12:40:12PM -0400, Jason Baron wrote: > > > On 06/30/2017 11:49 AM, Josh Poimboeuf wrote: > > On Wed, Jun 28, 2017 at 04:36:13PM -0500, Josh Poimboeuf wrote: > > > On Wed, Jun 28, 2017 at 04:34:18PM -0500, Josh Poimboeuf wrote: > > > > On Wed, Jun 28, 2017 at 04:25:19PM -0500, Josh Poimboeuf wrote: > > > > > On Wed, Jun 28, 2017 at 02:02:09PM -0400, Jason Baron wrote: > > > > > > Hi, > > > > > > > > > > > > If I have kernel source S, apply a kpatch module A, but then want to > > > > > > 'revert' parts of A, by applying a cumulative patch B, such that > > > > > > some > > > > > > functions are no longer different from source kernel S, how do I do > > > > > > that? > > > > > > > > > > > > It seems like there are a few options: > > > > > > > > > > > > 1) remove or disable module A, then load module B. This leaves an > > > > > > un-patched window and rmmod isn't possible without reliable > > > > > > backtrace. > > > > > > I'm also not sure this works with the kpatch 'stacking'. > > > > > > > > > > > > 2) modify patch B such that any functions that are initially > > > > > > unchanged > > > > > > from S, are in fact modified but modified in an acceptable way. > > > > > > > > > > > > 3) maybe the kernel can understand that are not added to or > > > > > > modified by > > > > > > a cumulative patch should be reverted back to their original state? > > > > > > > > > > > > #3 seems to me to be the cleanest...but maybe I'm missing > > > > > > something... > > > > > > > > > > Hi Jason, > > > > > > > > > > We don't have a mechanism for reverting patches, so #2 will probably > > > > > be > > > > > your best bet. > > > > > > > > Sorry, that was both poorly worded and incorrect, let me try again :-) > > > > > > > > I meant to say we don't have a mechanism for atomically replacing the > > > > entire contents of patch A with patch B. > > > > > > > > But I was mistaken. Technically we do have a "replace" mechanism. It > > > > was deprecated, see https://github.com/dynup/kpatch/issues/456 for more > > > > details. But it should work in theory if you load the patch module > > > > manually with insmod and the 'replace=1' argument. We had some issues > > > > with it though, so #2 might be a safer bet. > > > > > > And I should note that the replace only works with kpatch.ko, not with > > > livepatch. We may add replace support to livepatch eventually. > > > > FYI, I'm proposing a new KPATCH_REVERT_FUNCTION macro which makes this a > > little easier, so you can use the macro instead of making a trivial > > change to the function: > > > > https://github.com/dynup/kpatch/pull/715 > > > > I have to explicitly mark functions as revert, right? > > I'm wondering though if the function is inlined then I have to go and mark > all the places where it was inlined as KPATCH_REVERT_FUNCTION()? And further > there may be other changes in some of those functions and not others.
Yes, there are still manual steps involved: 1) compile v1 and note the list of changed functions 2) compile v2 (without KPATCH_REVERT_FUNCTION macro) and note the list of changed functions 3) for those functions which changed in v1 and did not change in v2, annotate them with KPATCH_REVERT_FUNCTION We should document those steps in the patch author guide as part of the pull request. > So it seems like it would be nice to calculate this based on the previous > set of loaded modules, or from the state of currently patched kernel. I agree it would be nice to have some way to automate it. I don't think using the state of the currently patched kernel would work in most cases, because when building kpatches for a distro, the kernel isn't typically patched. Maybe we could allow the user to pass in any previously built modules on the command-line, and then kpatch-build could automatically do the same work as KPATCH_REVERT_FUNCTION. I opened an issue to track that idea: https://github.com/dynup/kpatch/issues/716 We could either do that, or get atomic replace working properly. But either way it's going to be a bigger piece of work, so in the meantime we can have KPATCH_REVERT_FUNCTION and its associated manual steps. -- Josh _______________________________________________ kpatch mailing list [email protected] https://www.redhat.com/mailman/listinfo/kpatch
