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

[Evgenii Shatokhin]

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
> .