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

From: Evgenii Shatokhin
Date: Fri Jan 26 2018 - 06:29:52 EST


On 26.01.2018 13:23, Petr Mladek wrote:
On Fri 2018-01-19 16:10:42, Jason Baron wrote:


On 01/19/2018 02:20 PM, Evgenii Shatokhin wrote:
On 12.01.2018 22:55, Jason Baron wrote:
There is one more thing that might need attention here. In my
experiments with this patch series, I saw that unpatch callbacks are not
called for the older binary patch (the one being replaced).

So I think the pre_unpatch() can be called for any prior livepatch
modules from __klp_enable_patch(). Perhaps in reverse order of loading
(if there is more than one), and *before* the pre_patch() for the
livepatch module being loaded. Then, if it sucessfully patches in
klp_complete_transition() the post_unpatch() can be called for any prior
livepatch modules as well. I think again it makes sense to call the
post_unpatch() for prior modules *before* the post_patch() for the
current livepatch modules.

I played with this when working on v6. And I am not sure if it is
worth it.

The main reason is that we are talking about cumulative patches.
They are supposed to preserve most of the existing changes and
just remove and/or add few changes. The older patches might or
might not expect to be replaced this way.

If we would decide to run callbacks from the replaced patches
then it would make sense to run the one from the new patch
first. It is because we might need to do some hacks to preserve
the already existing changes.

We might need something like this for __klp_enable_patch():

static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
{
struct klp_patch *old_patch;
struct klp_object *old_obj;
int ret;

list_for_each_entry_reverse(old_patch, &klp_patches, list) {
if (!old_patch->enabled && old_patch != patch)
continue;

klp_for_each_object(old_patch, old_obj) {
if (!klp_is_object_loaded())
continue;

if (old_patch == patch) {
/* pre_patch from new patch */
ret = klp_pre_patch_callback(obj);
if (ret)
return ret;
if (!patch->replace)
return;
} else {
/* preunpatch from replaced patches */
klp_pre_unpatch_callback(obj);
}
}
}

return 0;
}

This was quite hairy. Alternative would be:

static void klp_run_pre_unpatch_callbacks_when_replacing(struct klp_patch *patch)
{
struct klp_patch *old_patch;
struct klp_object *old_obj;

if (WARN_ON(!patch->replace))
return;

list_for_each_entry_reverse(old_patch, &klp_patches, list) {
if (!old_patch->enabled || old_patch == patch)
continue;

klp_for_each_object(old_patch, old_obj) {
if (!klp_is_object_loaded())
continue;

klp_pre_unpatch_callback(obj);
}
}
}

static int klp_run_pre_patch_callbacks(struct klp_patch *patch)
{
struct klp_object *old_obj;
int ret;

klp_for_each_object(patch, old_obj) {
if (!klp_is_object_loaded())
continue;

ret = klp_pre_patch_callback(obj);
if (ret)
return ret;
}

if (patch->replace)
klp_run_pre_unpatch_callbacks_when_replacing(patch);

return 0;
}

2nd variant is easier to read but a lot of code. And this is only
what we would need for __klp_enable_patch(). But we would need
solution also for:

klp_cancel_transition();
klp_try_transition(); (2 variants for patching and unpatching)
klp_module_coming();
klp_module_going();



So, we are talking about a lot of rather non-trivial code.
IMHO, it might be easier to run just the callbacks from
the new patch. In reality, the author should always know
what it might be replacing and what needs to be done.

By other words, it might be much easier to handle all
situations in a single script in the new patch. Alternative
would be doing crazy hacks to prevent the older scripts from
destroying what we would like to keep. We would need to
keep in mind interactions between the scripts and
the order in which they are called.

Or do you plan to use cumulative patches to simply
get rid of any other "random" livepatches with something
completely different? In this case, it might be much more
safe to disable the older patches a normal way.

In my experience, it was quite convenient sometimes to just "replace all binary patches the user currently has loaded with this single one". No matter what these original binary patches did and where they came from.

Another problematic situation is when you need to actually downgrade a cumulative patch. Should be rare, but...

Well, I think we will disable the old patches explicitly in these cases, before loading of the new one. May be fragile but easier to maintain.


I would suggest to just document the current behavior.
We should create Documentation/livepatch/cummulative-patches.txt
anyway.

Yes, this would be helpful, because the behaviour is not very obvious.

Regards,
Evgenii


Best Regards,
Petr
.