On Fri, 10 Nov 2023, Petr Mladek wrote:
> The livepatch state API was added to help with maintaining:
>
> + changes done by livepatch callbasks
> + lifetime of shadow variables
>
> The original API was hard to use. Both objectives are better handled
> by the new per-state callbacks. They are called when the state is
> introduced or removed. There is also support for automatically freeing
> obsolete shadow variables.
>
> The new callbacks changed the view of compatibility. The livepatch
> can be replaced to any older one as long the current livepatch is
> able to disable the obsolete state.
>
> As a result, the new patch does not need to support the currently
> used states. The current patch will be able to disable them.
>
> The remaining question is what to do with the per-state version.
> It was supposed to allow doing more modifications on an existing
> state. The experience shows that it is not needed in practice.
>
> Well, it still might make sense to prevent downgrade when the state
> could not be disabled easily or when the author does not want to
> deal with it.
>
> Replace the per-state version with per-state block_disable flag.
> It allows to handle several scenarios:
I have no opinion to be honest. block_disable flag might be sufficient in
the end.
[...]
> @@ -159,7 +159,9 @@ struct klp_state {
> * @mod: reference to the live patch module
> * @objs: object entries for kernel objects to be patched
> * @states: system states that can get modified
> + * version: livepatch version (optional)
> * @replace: replace all actively used patches
> + *
> * @list: list node for global list of actively used patches
> * @kobj: kobject for sysfs resources
> * @obj_list: dynamic list of the object entries
> @@ -173,6 +175,7 @@ struct klp_patch {
> struct module *mod;
> struct klp_object *objs;
> struct klp_state *states;
> + unsigned int version;
> bool replace;
Is it still needed then? What would be the use case?
[...]
> /*
> * Allow to reverse a pending transition in both ways. It might be
> * necessary to complete the transition without forcing and breaking
> @@ -1097,10 +1104,10 @@ int klp_enable_patch(struct klp_patch *patch)
>
> if (!klp_is_patch_compatible(patch)) {
> pr_err("Livepatch patch (%s) is not compatible with the already
> installed livepatches.\n",
> - patch->mod->name);
> + patch->mod->name);
> mutex_unlock(&klp_mutex);
> return -EINVAL;
> - }
> + }
>
> if (!try_module_get(patch->mod)) {
> mutex_unlock(&klp_mutex);
> @@ -1111,17 +1118,17 @@ int klp_enable_patch(struct klp_patch *patch)
>
> ret = klp_init_patch(patch);
> if (ret)
> - goto err;
> + goto unlock_free;
>
> ret = __klp_enable_patch(patch);
> if (ret)
> - goto err;
> + goto unlock_free;
>
> mutex_unlock(&klp_mutex);
>
> return 0;
>
> -err:
> +unlock_free:
> klp_free_patch_start(patch);
Unrelated changes.
> /*
> * Check that the new livepatch will not break the existing system states.
> - * Cumulative patches must handle all already modified states.
> - * Non-cumulative patches can touch already modified states.
> + * The patch could replace existing patches only when the obsolete
> + * states can be disabled.
> */
> bool klp_is_patch_compatible(struct klp_patch *patch)
> {
> struct klp_patch *old_patch;
> struct klp_state *old_state;
>
> + /* Non-cumulative patches are always compatible. */
> + if (!patch->replace)
> + return true;
> +
Cumulative != atomic replace. Those are two different things.
Miroslav
