Re: [PATCH 0/3] livepatch: introduce atomic replace

[Miroslav Benes]

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