[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LSU.2.20.1707211459480.6086@pobox.suse.cz>
Date: Fri, 21 Jul 2017 15:06:03 +0200 (CEST)
From: Miroslav Benes <mbenes@...e.cz>
To: Jason Baron <jbaron@...mai.com>
cc: linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
pmladek@...e.com
Subject: Re: [PATCH 0/3] livepatch: introduce atomic replace
On Wed, 19 Jul 2017, Jason Baron wrote:
> Hi,
>
> In testing livepatch, I found that when doing cumulative patches, if a patched
> function is completed reverted by a subsequent patch (back to its original state)
> livepatch does not revert the funtion to its original state. Specifically, if
> patch A introduces a change to function 1, and patch B reverts the change to
> function 1 and introduces changes to say function 2 and 3 as well, the change
> that patch A introducd to function 1 is still present. This could be addressed
> by first completely removing patch A (disable and then rmmod) and then inserting
> patch B (insmod and enable), but this leaves an unpatched window. In discussing
> this issue with Josh on the kpatch mailing list, he mentioned that we could get
> 'atomic replace working properly', and that is the direction of this patchset:
> https://www.redhat.com/archives/kpatch/2017-June/msg00005.html
Hi Jason,
this has been on my TODO list for a long time now, so thanks for working
on this. We have the same feature in kGraft and we use it heavily (in fact
we distribute our patches as cumulative and "replace_all" how we call it).
The forward port of the feature from kGraft is unfortunately not
straightforward. We do not have a concept of klp_target_state there, so we
can freely let functions to be patched or reverted in one go. We cannot do
the same upstream. At first glance, you used nop function exactly for this
case. Nice hack :).
> Patches:
>
> 1) livepatch: Add klp_object and klp_func iterators
> Just a prep patch for the 'atomic revert' feature
>
> 2) livepatch: add atomic replace
> Core feature
>
> 3) livepatch: Add a sysctl livepatch_mode for atomic replace
> Introduces a knob for enabling atomic replace. I hate knobs and perhaps
> its possible to default to cumulative replace? Although I suspect there
> are workflows relying on the existing behavior - I'm not sure. It may
> be desirable to associate the knob with the patch itself as in the
> 'immediate' flag, such that we don't introduce a global sysctl that
> likely would also need to built-in, if there are patches in the initrd.
Yes. I think it should be associated with the patch itself. This would
allow more flexible behaviour. You could stack more patches on top of
"atomic replace" patch.
Anyway, I'm on holiday next week, so I'll take a proper look the week
after.
Thanks,
Miroslav
Powered by blists - more mailing lists