livepatch: introduce atomic replace

Hi,

While using 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 introduced 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,

While using 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 introduced 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

Thanks,

-Jason

v4-v5

(quoted diff) +0 -2

-re-base onto remove-immediate branch (removing immediate dependencies)
-replaced modules can be re-enabled by doing rmmod and then insmod

v3-v4:

(quoted diff) +0 -2

-add static patch, objects, funcs to linked lists to simplify iterator
-break-out pure function movement as patch 2/3

v2-v3:

(quoted diff) +0 -8

-refactor how the dynamic nops are calculated (Petr Mladek)
-move the creation of dynamic nops to enable/disable paths
-add klp_replaced_patches list to indicate patches that can be re-enabled
-dropped 'replaced' field
-renamed dynamic fields in klp_func, object and patch
-moved iterator implementation to kernel/livepatch/core.c
-'inherit' nop immediate flag
-update kobject_put free'ing logic (Petr Mladek)

v1-v2:

(quoted diff) +0 -6

-removed the func_iter and obj_iter (Petr Mladek)
-initialiing kobject structure for no_op functions using:
  klp_init_object() and klp_init_func()
-added a 'replace' field to klp_patch, similar to the immediate field
-a 'replace' patch now disables all previous patches
-tried to shorten klp_init_patch_no_ops()...
-Simplified logic klp_complete_transition (Petr Mladek)

Jason Baron (3):
livepatch: use lists to manage patches, objects and functions
livepatch: shuffle core.c function order
livepatch: add atomic replace

  include/linux/livepatch.h     |  25 +-
  kernel/livepatch/core.c       | 626 ++++++++++++++++++++++++++++++------------
  kernel/livepatch/core.h       |   6 +
  kernel/livepatch/patch.c      |  22 +-
  kernel/livepatch/patch.h      |   4 +-
  kernel/livepatch/transition.c |  49 +++-
  6 files changed, 537 insertions(+), 195 deletions(-)

Quoted 25 lines · Jason Baron

Hi,
  While using 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 introduced 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

Thanks,

Quoted 1 line

-Jason

Thanks a lot! Atomic replace is really crucial when using cumulative patches.

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

That is, I have prepared 2 binary patches, each has all 4 patch/unpatch callbacks.

When I load the first patch, its pre-patch and post-patch callbacks are called as expected.

Then I replace it with the second patch. Replacement is successful, the pre-patch and post-patch callbacks are called for the second patch, However, pre-unpatch and post-unpatch callbacks do not run for the first one. This makes it more difficult to clean up what its pre/post-patch callbacks have done.

It would be nice if pre-/post- unpatch callbacks were called for the first patch, perhaps, before/after the patch is actually disabled during replacement. I cannot see right now though, which way is the best to implement that.

Quoted 31 lines

Quoted 1 line

Quoted 1 line

v4-v5

(quoted diff) +0 -2

-re-base onto remove-immediate branch (removing immediate dependencies)
-replaced modules can be re-enabled by doing rmmod and then insmod

v3-v4:

(quoted diff) +0 -2

-add static patch, objects, funcs to linked lists to simplify iterator
-break-out pure function movement as patch 2/3

  v2-v3:

(quoted diff) +0 -8

-refactor how the dynamic nops are calculated (Petr Mladek)
-move the creation of dynamic nops to enable/disable paths
-add klp_replaced_patches list to indicate patches that can be re-enabled
-dropped 'replaced' field
-renamed dynamic fields in klp_func, object and patch
-moved iterator implementation to kernel/livepatch/core.c
-'inherit' nop immediate flag
-update kobject_put free'ing logic (Petr Mladek)

v1-v2:

(quoted diff) +0 -2

-removed the func_iter and obj_iter (Petr Mladek)
-initialiing kobject structure for no_op functions using:

  klp_init_object() and klp_init_func()

(quoted diff) +0 -4

-added a 'replace' field to klp_patch, similar to the immediate field
-a 'replace' patch now disables all previous patches
-tried to shorten klp_init_patch_no_ops()...
-Simplified logic klp_complete_transition (Petr Mladek)

Jason Baron (3):
   livepatch: use lists to manage patches, objects and functions
   livepatch: shuffle core.c function order
   livepatch: add atomic replace

Quoted 9 lines

  include/linux/livepatch.h     |  25 +-
  kernel/livepatch/core.c       | 626
++++++++++++++++++++++++++++++------------
  kernel/livepatch/core.h       |   6 +
  kernel/livepatch/patch.c      |  22 +-
  kernel/livepatch/patch.h      |   4 +-
  kernel/livepatch/transition.c |  49 +++-

  6 files changed, 537 insertions(+), 195 deletions(-)

Regards,
Evgenii

Quoted 4 lines · Evgenii Shatokhin

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.

Quoted 8 lines · Jason Baron

Quoted 4 lines · Evgenii Shatokhin

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

Quoted 8 lines

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
.

Quoted 30 lines · Petr Mladek

Quoted 8 lines · Jason Baron

Quoted 4 lines · Evgenii Shatokhin

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

Quoted 8 lines

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.

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.

Quoted 4 lines

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.

Quoted 31 lines · Evgenii Shatokhin

Quoted 30 lines · Petr Mladek

Quoted 8 lines · Jason Baron

Quoted 4 lines · Evgenii Shatokhin

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

Quoted 8 lines

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.

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.

Quoted 8 lines

Quoted 4 lines

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.

To be honest, I would feel better if the livepatch framework is more safe. It should prevent breaking the system by a patch that atomically replaces other random patches that rely on callbacks.

Well, combining random livepatches from random vendors is a call for troubles. It might easily fail when two patches add different changes to the same function.

I wonder if we should introduce some tags, keys, vendors. I mean to define an identification or dependencies that would say that some patches are compatible or incompatible.

We could allow to livepatch one function by two livepatches only if they are from the same vendor. But then the order still might be important. Also I am not sure if this condition is safe enough.

One the other hand, we could handle callbacks like the shadow variables. Every shadow variable has an ID. We have an API to add/read/update/remove them. We might allow to have more callbacks with different IDs. Then we could run callbacks only for IDs that are newly added or removed. Sigh, this would be very complex implementation as well. But it might make these features easier to use and maintain.

Alternatively, I thought about having two modes. One is stack of "random" patches. Second is a cumulative mode. IMHO, the combination of the two modes makes things very complex. It might be much easier if we allow to load patch with the replace flag enabled only on top of another patch with this flag enabled.

Quoted 2 lines

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

Good point. Well, especially the callbacks should be rare.

I would like to hear from people that have some experience or plans with using callbacks and cumulative patches.

I wonder how they plan to technically reuse a feature in the cummulative patches. IMHO, it should be very rare to add/remove callbacks. Then it might be safe to downgrade a cummulative patch if the callbacks are exactly the same.

Quoted 2 lines

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 am afraid that we will not be able to support all use cases and keep the code sane.

Best Regards,
Petr
.

Quoted 77 lines · Petr Mladek

Quoted 33 lines · Evgenii Shatokhin

Quoted 32 lines · Petr Mladek

Quoted 9 lines · Jason Baron

Quoted 5 lines · Evgenii Shatokhin

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

Quoted 9 lines

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.

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.

Quoted 8 lines

Quoted 4 lines

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.

To be honest, I would feel better if the livepatch framework is more safe. It should prevent breaking the system by a patch that atomically replaces other random patches that rely on callbacks.

Well, combining random livepatches from random vendors is a call for troubles. It might easily fail when two patches add different changes to the same function.

I wonder if we should introduce some tags, keys, vendors. I mean to define an identification or dependencies that would say that some patches are compatible or incompatible.

We could allow to livepatch one function by two livepatches only if they are from the same vendor. But then the order still might be important. Also I am not sure if this condition is safe enough.

One the other hand, we could handle callbacks like the shadow variables. Every shadow variable has an ID. We have an API to add/read/update/remove them. We might allow to have more callbacks with different IDs. Then we could run callbacks only for IDs that are newly added or removed. Sigh, this would be very complex implementation as well. But it might make these features easier to use and maintain.

Alternatively, I thought about having two modes. One is stack of "random" patches. Second is a cumulative mode. IMHO, the combination of the two modes makes things very complex. It might be much easier if we allow to load patch with the replace flag enabled only on top of another patch with this flag enabled.

Quoted 4 lines

Quoted 2 lines

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

Good point. Well, especially the callbacks should be rare.

Yes, we will probably use them for the most important fixes only and only if there are no other suitable options. The patches with could be more difficult to maintain anyway.

Quoted 10 lines

I would like to hear from people that have some experience or plans with using callbacks and cumulative patches.

I wonder how they plan to technically reuse a feature in the cummulative patches. IMHO, it should be very rare to add/remove callbacks. Then it might be safe to downgrade a cummulative patch if the callbacks are exactly the same.

Quoted 5 lines

Quoted 2 lines

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 am afraid that we will not be able to support all use cases and keep the code sane.

Thats is OK.

I agree with you that the current behaviour of the 'replace' operation w.r.t. patch callbacks should stay as is. Making kernel code more complex than it should be is definitely a bad thing.

I cannot say much about generic policies for cumulative/non-cumulative patches here. In our particular case (Virtuozzo ReadyKernel), the cumulative patches are easily recognizable by their names, they have 'cumulative' substring there. Patch version is also included in the name and is easily obtainable.

So I think, I'll update our userspace tools (based on kpatch) so that their 'replace' command worked as follows:

* Explicitly disable all loaded non-cumulative patches first.
* If the new patch is non-cumulative, just disable all loaded patches explicitly and then load the new one.
* If the new patch is cumulative, explicitly disable the loaded cumulative patches that have smaller version numbers than the new one (if any). Then run replace as usual.

This way, the in-kernel 'replace' functionality will only be used when upgrading cumulative patches - and we can design their callbacks (if the callbacks are needed at some point) according to the current behaviour.

Patch downgrade operations should be very rare and could only happen if something goes wrong at the users' side. I think, it is acceptable to disable the loaded patch first in that case. Its callbacks, if present, will do proper cleanup. Then the older, "good" patch can be loaded normally. If such things happen, we'll have to investigate the affected nodes anyway.

Same for the custom (non-cumulative) patches. We sometimes use them to "patch things up" quickly before the cumulative patch with the appropriate fixes is officially released. It should be reasonably safe to disable these patches explicitly when loading a new cumulative patch.

So, I think, the current situation with 'replace' & callbacks is acceptable for us.

[ ... snip ... ]

Our main interest in 'atomic replace' is simply to make sure that cumulative patches work as expected in that they 'replace' any prior patches. We have an interest primarily in being able to apply patches from the stable trees via livepatch. I think the current situation with respect to 'replace' and callbacks is fine for us as well, but could change based on more experience with livepatch.

As an aside I was just wondering how you are making use of the callbacks using the tool you mentioned (that is based on kpatch)? I see in the upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as opposed to livepatch), and I do not see these sort of macros for the recently introduced livepatch callbacks. It seems it would be easy enough to add similar hooks for the livepatch callbacks. I was thinking of writing such a patch, but was wondering if there was an existing solution?

Quoted 8 lines · Jason Baron

[ ... snip ... ]

Our main interest in 'atomic replace' is simply to make sure that cumulative patches work as expected in that they 'replace' any prior patches. We have an interest primarily in being able to apply patches from the stable trees via livepatch. I think the current situation with respect to 'replace' and callbacks is fine for us as well, but could change based on more experience with livepatch.

Well the callback implementation was based somewhat on theoretical usage.. it was inspired by the kpatch macros that you talk about below, in which we had a few specific use-cases. Converting (un)patch notifiers to the livepatch model presented additional callback locations, and as such we ended up with pre-patch, post-patch, pre-unpatch and post-unpatch callbacks. Hopefully we'll get a better idea of their worth as we gain experience using them. At this point in time I would suggest keeping it as simple and safe as possible. :)

Quoted 9 lines

As an aside I was just wondering how you are making use of the callbacks using the tool you mentioned (that is based on kpatch)? I see in the upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as opposed to livepatch), and I do not see these sort of macros for the recently introduced livepatch callbacks. It seems it would be easy enough to add similar hooks for the livepatch callbacks. I was thinking of writing such a patch, but was wondering if there was an existing solution?

I've got a work in progress PR for this very feature:
https://github.com/dynup/kpatch/pull/780

Evgenii has already provided some helpful feedback. Another set of eyes/testing would be appreciated!

Quoted 8 lines · Jason Baron

[ ... snip ... ]

Our main interest in 'atomic replace' is simply to make sure that cumulative patches work as expected in that they 'replace' any prior patches. We have an interest primarily in being able to apply patches from the stable trees via livepatch. I think the current situation with respect to 'replace' and callbacks is fine for us as well, but could change based on more experience with livepatch.

Well the callback implementation was based somewhat on theoretical usage.. it was inspired by the kpatch macros that you talk about below, in which we had a few specific use-cases. Converting (un)patch notifiers to the livepatch model presented additional callback locations, and as such we ended up with pre-patch, post-patch, pre-unpatch and post-unpatch callbacks. Hopefully we'll get a better idea of their worth as we gain experience using them. At this point in time I would suggest keeping it as simple and safe as possible. :)

Quoted 9 lines

As an aside I was just wondering how you are making use of the callbacks using the tool you mentioned (that is based on kpatch)? I see in the upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as opposed to livepatch), and I do not see these sort of macros for the recently introduced livepatch callbacks. It seems it would be easy enough to add similar hooks for the livepatch callbacks. I was thinking of writing such a patch, but was wondering if there was an existing solution?

I've got a work in progress PR for this very feature:
https://github.com/dynup/kpatch/pull/780

Evgenii has already provided some helpful feedback. Another set of eyes/testing would be appreciated!

Regards,

-- Joe
.

Quoted 4 lines · Evgenii Shatokhin

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.

To be honest, I would feel better if the livepatch framework is more safe. It should prevent breaking the system by a patch that atomically replaces other random patches that rely on callbacks.

Well, combining random livepatches from random vendors is a call for troubles. It might easily fail when two patches add different changes to the same function.

I wonder if we should introduce some tags, keys, vendors. I mean to define an identification or dependencies that would say that some patches are compatible or incompatible.

We could allow to livepatch one function by two livepatches only if they are from the same vendor. But then the order still might be important. Also I am not sure if this condition is safe enough.

One the other hand, we could handle callbacks like the shadow variables. Every shadow variable has an ID. We have an API to add/read/update/remove them. We might allow to have more callbacks with different IDs. Then we could run callbacks only for IDs that are newly added or removed. Sigh, this would be very complex implementation as well. But it might make these features easier to use and maintain.

Quoted 23 lines · Petr Mladek

Quoted 4 lines · Evgenii Shatokhin

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.

To be honest, I would feel better if the livepatch framework is more safe. It should prevent breaking the system by a patch that atomically replaces other random patches that rely on callbacks.

Well, combining random livepatches from random vendors is a call for troubles. It might easily fail when two patches add different changes to the same function.

I wonder if we should introduce some tags, keys, vendors. I mean to define an identification or dependencies that would say that some patches are compatible or incompatible.

We could allow to livepatch one function by two livepatches only if they are from the same vendor. But then the order still might be important. Also I am not sure if this condition is safe enough.

Quoted 7 lines

One the other hand, we could handle callbacks like the shadow variables. Every shadow variable has an ID. We have an API to add/read/update/remove them. We might allow to have more callbacks with different IDs. Then we could run callbacks only for IDs that are newly added or removed. Sigh, this would be very complex implementation as well. But it might make these features easier to use and maintain.

Interesting ideas, but I wonder if this could be accomplished at the livepatch module level? ie, leave it to kpatch-build (or a livepatch equivalent) to produce a module that does this automatically. I guess it would then be completely opt-in checking, but transfers the complexity out of the kernel livepatching core.

I don't see a simple way to provide flexibility of when/if calling redundant callbacks without making the code even more complicated. I don't have any use-cases off hand that would require such features, but I guess if I did absolutely needed them, I might be inclined to say prepare ahead of time and write callbacks so that they may be disabled externally -- either by a new livepatch module's init() or some other means. Then whatever crazy policy I need is contained to my modules, not provided or enforced by the kernel.

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.

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.