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

[Petr Mladek]

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.

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

Best Regards,
Petr