Re: [PATCH v1 1/5] xsplice: Design document.

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 10/27/2015 08:08 AM, Martin Pohlack wrote:
> On 06.10.2015 14:57, Ross Lagerwall wrote:
>> On 09/16/2015 10:01 PM, Konrad Rzeszutek Wilk wrote:
>>> +### xSplice interdependencies
>>> +
>>> +xSplice patches interdependencies are tricky.
>>> +
>>> +There are the ways this can be addressed:
>>> + * A single large patch that subsumes and replaces all previous ones.
>>> +   Over the life-time of patching the hypervisor this large patch
>>> +   grows to accumulate all the code changes.
>>> + * Hotpatch stack - where an mechanism exists that loads the hotpatches
>>> +   in the same order they were built in. We would need an build-id
>>> +   of the hypevisor to make sure the hot-patches are build against the
>>> +   correct build.
>>> + * Payload containing the old code to check against that. That allows
>>> +   the hotpatches to be loaded indepedently (if they don't overlap) - or
>>> +   if the old code also containst previously patched code - even if they
>>> +   overlap.
>>> +
>>> +The disadvantage of the first large patch is that it can grow over
>>> +time and not provide an bisection mechanism to identify faulty patches.
>>> +
>>> +The hot-patch stack puts stricts requirements on the order of the patches
>>> +being loaded and requires an hypervisor build-id to match against.
>>> +
>>> +The old code allows much more flexibility and an additional guard,
>>> +but is more complex to implement.
>>> +
>>
>> If the single large patch mechanism is used, a new REPLACE action is
>> needed to atomically replace one patch with another to prevent a window
>> where the hypervisor is unpatched. kpatch has a "replace" command for
>> this purpose. This may be useful even for the other mechanisms listed above.
>>
>>   From what I can tell:
>> * kSplice uses old code checking (method [3] above), although in
>> practice the userspace tools implement dependency logic to enforce a
>> linear stack of patches.
>> * kPatch and kGraft recommend using the single large patch mechanism
>> although there's nothing preventing two independent patches from being
>> loaded.
>
> To make sure this argument is not lost: I wrote in an earlier email
>
>> * There is a general (and mostly obscure) limitation on unloading
>>    hotpatches:
>>
>>    In contrast to normal kernel modules where the module code adheres
>>    to specific conventions around resource allocation and locking,
>>    hotpatches typically contain code from any context.  That code is
>>    usually not aware that it can be unloaded.
>>
>>    That code could leave behind in Xen references to itself, e.g., by
>>    installing a function pointer in a global data structure, without
>>    incrementing something like a usage count.  While most hotpatch code
>>    will probably be very simple and small, a similar effect could even
>>    be achieved by code called from the hotpatch in Xen, e.g., some code
>>    patch could dynamically generate a backtrace and later decide to
>>    inspect individual elements from the collected trace, later being a
>>    time, where the hotpatch has been unloaded again.
>>
>>    One could approach that proplem from multiple angles: code
>>    inspection of generated hotpatches, testing, and by making unloading
>>    a very special and exceptional operation.
>
> If we follow that argument, we should consider patch unloading to be a
> potentially unsafe operation which should not be part of standard
> workflows.  Consequently, a similar argument holds for REPLACE, because
> removing / unloading older code is part of replacement.
>
> One could now either aim to have special inspection and testing
> regarding unloading to lower the risk there, or:
>
> Structure hotpatches as an ever-increasing stack of modules that are
> loaded on top of each other.  This approach works around the unloading
> problems as well as the temporary-unsafe problem outlined above.  Memory
> usage would be similar to an ever-increasing monolithic hotpatch (+ some
> fragmentation, - the temporary additional memory requirement for
> replacing the monolithic hotpatch).
>

I agree that this could be an issue with the replace operation; in 
practice I don't believe it would be an issue (and it is the choice that 
both kGraft and kpatch use). Since it is trivial to implement the 
replace operation (it's already done in my branch), I think we should 
have both and let vendors choose.

I think we should also aim for a slightly more general concept than a 
stack, and let vendors implement the stack approach if they want. Here 
is what I wrote in an earlier mail:
* Each hypervisor has an embedded build id.
* Each binary patch has an embedded build id.
* The hypervisor should expose its build id and the build id of every 
loaded binary patch.
* Each binary patch specifies one or more build ids on which it depends. 
These build ids can be either a hypervisor build id or another patch 
build id. The dependencies could either identified automatically or by a 
developer.
* The userspace tool enforces dependency management (user can optionally 
force patch apply). I don't see any reason to involve the
hypervisor for dependency management.

The vendor could very easily implement a stack of patches with this 
approach (afaik this is sort of what ksplice does).

The one caveat with having multiple patches is that the patches need to 
be dynamically linked (e.g. consider a patch on top of a patch which 
introduces a new function). I've got a basically working version of 
dynamic linking of modules, but it is certainly more complex than static 
linking and we probably shouldn't aim for it in V1.

-- 
Ross Lagerwall