[kpatch] cumulative patch that reverts a function

[Josh Poimboeuf]

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