Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Mon 2018-12-17 10:27:29, Josh Poimboeuf wrote: > On Mon, Dec 17, 2018 at 05:07:09PM +0100, Petr Mladek wrote: > > On Thu 2018-12-13 17:06:52, Josh Poimboeuf wrote: > > > On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote: > > > > The atomic replace and cumulative patches were introduced as a more > > > > secure > > > > way to handle dependent patches. They simplify the logic: > > > > > > > > + Any new cumulative patch is supposed to take over shadow variables > > > > and changes made by callbacks from previous livepatches. > > > > > > > > + All replaced patches are discarded and the modules can be unloaded. > > > > As a result, there is only one scenario when a cumulative livepatch > > > > gets disabled. > > > > > > > > The different handling of "normal" and cumulative patches might cause > > > > confusion. It would make sense to keep only one mode. On the other hand, > > > > it would be rude to enforce using the cumulative livepatches even for > > > > trivial and independent (hot) fixes. > > > > > > > > This patch removes the stack of patches. The list of enabled patches > > > > is still needed but the ordering is not longer enforced. > > > > > > > > Note that it is not possible to catch all possible dependencies. It is > > > > the responsibility of the livepatch authors to decide. > > > > > > > > Nevertheless this patch prevents having two patches for the same > > > > function > > > > enabled at the same time after the transition finishes. It might help > > > > to catch obvious mistakes. But more importantly, we do not need to > > > > handle situation when a patch in the middle of the function stack > > > > (ops->func_stack) is being removed. > > > > > > I'm not sure about this patch. I like the removal of the stacking. But > > > do we really want to enforce no dependencies between non-cumulative > > > patches? It can be done correctly if the user is careful. > > > > > > Maybe we should just let users do it if they want to. And then that > > > also would mean less code for us to maintain. > > > > > > And as usual, I apologize if I'm either contradicting or repeating past > > > versions of myself. :-) > > > > This patch was actually motivated by you. On some conference, we > > discussed how to automatize the creation of livepatches. You wanted > > to make livepatching more safe in general (by tools, by checks, ...). > > Also you always wanted to make things easier and reduce possible > > scenarios. I thought that this might be in line with your wishes. > > > > The problem with this patch is that it forces people to use > > cumulative patches. I am not sure if everyone is ready for it. > > > > I do not resist on it. But I still think that it makes sense. > > I do remember suggesting the removal of the stacking. I think that's a > good idea. > > I don't remember suggesting the other part: trying to detect and prevent > dependencies for non-replace users. If I did suggest that, which is > very possible, I apologize for being wishy-washy :-) You remember it correctly. You proposed only the removing of the stacking. The preventing dependent patches was my idea. I thought that it might be in line with your vision. I was wrong ;-) > The way I currently see it, there are two classes of users: cumulative > and non-cumulative. IMO we should accept both as reasonable > possiblities. > > Cumulative users will use 'replace'. Non-cumulative users will do > whatever they want, and we shouldn't try to restrict them. > > So I would propose that we remove the stacking, and not try to enforce > patch dependencies in any way. OK, I will remove the restriction in v15. Best Regards, Petr
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Mon, Dec 17, 2018 at 05:07:09PM +0100, Petr Mladek wrote: > On Thu 2018-12-13 17:06:52, Josh Poimboeuf wrote: > > On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote: > > > The atomic replace and cumulative patches were introduced as a more secure > > > way to handle dependent patches. They simplify the logic: > > > > > > + Any new cumulative patch is supposed to take over shadow variables > > > and changes made by callbacks from previous livepatches. > > > > > > + All replaced patches are discarded and the modules can be unloaded. > > > As a result, there is only one scenario when a cumulative livepatch > > > gets disabled. > > > > > > The different handling of "normal" and cumulative patches might cause > > > confusion. It would make sense to keep only one mode. On the other hand, > > > it would be rude to enforce using the cumulative livepatches even for > > > trivial and independent (hot) fixes. > > > > > > This patch removes the stack of patches. The list of enabled patches > > > is still needed but the ordering is not longer enforced. > > > > > > Note that it is not possible to catch all possible dependencies. It is > > > the responsibility of the livepatch authors to decide. > > > > > > Nevertheless this patch prevents having two patches for the same function > > > enabled at the same time after the transition finishes. It might help > > > to catch obvious mistakes. But more importantly, we do not need to > > > handle situation when a patch in the middle of the function stack > > > (ops->func_stack) is being removed. > > > > I'm not sure about this patch. I like the removal of the stacking. But > > do we really want to enforce no dependencies between non-cumulative > > patches? It can be done correctly if the user is careful. > > > > Maybe we should just let users do it if they want to. And then that > > also would mean less code for us to maintain. > > > > And as usual, I apologize if I'm either contradicting or repeating past > > versions of myself. :-) > > This patch was actually motivated by you. On some conference, we > discussed how to automatize the creation of livepatches. You wanted > to make livepatching more safe in general (by tools, by checks, ...). > Also you always wanted to make things easier and reduce possible > scenarios. I thought that this might be in line with your wishes. > > The problem with this patch is that it forces people to use > cumulative patches. I am not sure if everyone is ready for it. > > I do not resist on it. But I still think that it makes sense. I do remember suggesting the removal of the stacking. I think that's a good idea. I don't remember suggesting the other part: trying to detect and prevent dependencies for non-replace users. If I did suggest that, which is very possible, I apologize for being wishy-washy :-) The way I currently see it, there are two classes of users: cumulative and non-cumulative. IMO we should accept both as reasonable possiblities. Cumulative users will use 'replace'. Non-cumulative users will do whatever they want, and we shouldn't try to restrict them. So I would propose that we remove the stacking, and not try to enforce patch dependencies in any way. -- Josh
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu 2018-12-13 17:06:52, Josh Poimboeuf wrote: > On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote: > > The atomic replace and cumulative patches were introduced as a more secure > > way to handle dependent patches. They simplify the logic: > > > > + Any new cumulative patch is supposed to take over shadow variables > > and changes made by callbacks from previous livepatches. > > > > + All replaced patches are discarded and the modules can be unloaded. > > As a result, there is only one scenario when a cumulative livepatch > > gets disabled. > > > > The different handling of "normal" and cumulative patches might cause > > confusion. It would make sense to keep only one mode. On the other hand, > > it would be rude to enforce using the cumulative livepatches even for > > trivial and independent (hot) fixes. > > > > This patch removes the stack of patches. The list of enabled patches > > is still needed but the ordering is not longer enforced. > > > > Note that it is not possible to catch all possible dependencies. It is > > the responsibility of the livepatch authors to decide. > > > > Nevertheless this patch prevents having two patches for the same function > > enabled at the same time after the transition finishes. It might help > > to catch obvious mistakes. But more importantly, we do not need to > > handle situation when a patch in the middle of the function stack > > (ops->func_stack) is being removed. > > I'm not sure about this patch. I like the removal of the stacking. But > do we really want to enforce no dependencies between non-cumulative > patches? It can be done correctly if the user is careful. > > Maybe we should just let users do it if they want to. And then that > also would mean less code for us to maintain. > > And as usual, I apologize if I'm either contradicting or repeating past > versions of myself. :-) This patch was actually motivated by you. On some conference, we discussed how to automatize the creation of livepatches. You wanted to make livepatching more safe in general (by tools, by checks, ...). Also you always wanted to make things easier and reduce possible scenarios. I thought that this might be in line with your wishes. The problem with this patch is that it forces people to use cumulative patches. I am not sure if everyone is ready for it. I do not resist on it. But I still think that it makes sense. Best Regards, Petr
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote: > The atomic replace and cumulative patches were introduced as a more secure > way to handle dependent patches. They simplify the logic: > > + Any new cumulative patch is supposed to take over shadow variables > and changes made by callbacks from previous livepatches. > > + All replaced patches are discarded and the modules can be unloaded. > As a result, there is only one scenario when a cumulative livepatch > gets disabled. > > The different handling of "normal" and cumulative patches might cause > confusion. It would make sense to keep only one mode. On the other hand, > it would be rude to enforce using the cumulative livepatches even for > trivial and independent (hot) fixes. > > This patch removes the stack of patches. The list of enabled patches > is still needed but the ordering is not longer enforced. > > Note that it is not possible to catch all possible dependencies. It is > the responsibility of the livepatch authors to decide. > > Nevertheless this patch prevents having two patches for the same function > enabled at the same time after the transition finishes. It might help > to catch obvious mistakes. But more importantly, we do not need to > handle situation when a patch in the middle of the function stack > (ops->func_stack) is being removed. I'm not sure about this patch. I like the removal of the stacking. But do we really want to enforce no dependencies between non-cumulative patches? It can be done correctly if the user is careful. Maybe we should just let users do it if they want to. And then that also would mean less code for us to maintain. And as usual, I apologize if I'm either contradicting or repeating past versions of myself. :-) -- Josh
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote:
> The atomic replace and cumulative patches were introduced as a more secure
> way to handle dependent patches. They simplify the logic:
>
> + Any new cumulative patch is supposed to take over shadow variables
> and changes made by callbacks from previous livepatches.
>
> + All replaced patches are discarded and the modules can be unloaded.
> As a result, there is only one scenario when a cumulative livepatch
> gets disabled.
>
> The different handling of "normal" and cumulative patches might cause
> confusion. It would make sense to keep only one mode. On the other hand,
> it would be rude to enforce using the cumulative livepatches even for
> trivial and independent (hot) fixes.
>
> This patch removes the stack of patches. The list of enabled patches
> is still needed but the ordering is not longer enforced.
^^
s/not longer/no longer
>
> Note that it is not possible to catch all possible dependencies. It is
> the responsibility of the livepatch authors to decide.
>
> Nevertheless this patch prevents having two patches for the same function
> enabled at the same time after the transition finishes. It might help
> to catch obvious mistakes. But more importantly, we do not need to
> handle situation when a patch in the middle of the function stack
s/handle situation/handle a situation
> (ops->func_stack) is being removed.
>
> Signed-off-by: Petr Mladek
> ---
Acked-by: Joe Lawrence
> Documentation/livepatch/cumulative-patches.txt | 11 ++
> Documentation/livepatch/livepatch.txt | 30 ---
> kernel/livepatch/core.c| 51
> --
> 3 files changed, 68 insertions(+), 24 deletions(-)
>
>
> [ ... snip ... ]
>
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index ba6e83a08209..3c150ab19b99 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -143,9 +143,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully
> supported by
> the kernel livepatching.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> -is in transition. Only a single patch (the topmost patch on the stack)
> -can be in transition at a given time. A patch can remain in transition
> -indefinitely, if any of the tasks are stuck in the initial patch state.
> +is in transition. Only a single patch can be in transition at a given
> +time. A patch can remain in transition indefinitely, if any of the tasks
> +are stuck in the initial patch state.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> @@ -329,9 +329,10 @@ The livepatch gets enabled by calling klp_enable_patch()
> typically from
> module_init() callback. The system will start using the new implementation
> of the patched functions at this stage.
>
> -First, the addresses of the patched functions are found according to their
> -names. The special relocations, mentioned in the section "New functions",
> -are applied. The relevant entries are created under
> +First, possible conflicts are checked for non-cummulative patches with
^^^
s/non-cummulative/non-cumulative
>
> [ ... snip ... ]
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 0ce752e9e8bb..d8f6f49ac6b3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -122,6 +122,47 @@ static struct klp_object *klp_find_object(struct
> klp_patch *patch,
> return NULL;
> }
>
> +static int klp_check_obj_conflict(struct klp_patch *patch,
> + struct klp_object *old_obj)
> +{
> + struct klp_object *obj;
> + struct klp_func *func, *old_func;
> +
> + obj = klp_find_object(patch, old_obj);
> + if (!obj)
> + return 0;
> +
> + klp_for_each_func(old_obj, old_func) {
> + func = klp_find_func(obj, old_func);
> + if (!func)
> + continue;
> +
> + pr_err("Function '%s,%lu' in object '%s' has already been
> livepatched.\n",
> +func->old_name, func->old_sympos ? func->old_sympos : 1,
> +obj->name ? obj->name : "vmlinux");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Should we add a self-test check for this condition? Let me know and I
can give it a go if you want to include with v15.
-- Joe
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote:
> The atomic replace and cumulative patches were introduced as a more secure
> way to handle dependent patches. They simplify the logic:
>
> + Any new cumulative patch is supposed to take over shadow variables
> and changes made by callbacks from previous livepatches.
>
> + All replaced patches are discarded and the modules can be unloaded.
> As a result, there is only one scenario when a cumulative livepatch
> gets disabled.
>
> The different handling of "normal" and cumulative patches might cause
> confusion. It would make sense to keep only one mode. On the other hand,
> it would be rude to enforce using the cumulative livepatches even for
> trivial and independent (hot) fixes.
>
> This patch removes the stack of patches. The list of enabled patches
> is still needed but the ordering is not longer enforced.
^^
s/not longer/no longer
>
> Note that it is not possible to catch all possible dependencies. It is
> the responsibility of the livepatch authors to decide.
>
> Nevertheless this patch prevents having two patches for the same function
> enabled at the same time after the transition finishes. It might help
> to catch obvious mistakes. But more importantly, we do not need to
> handle situation when a patch in the middle of the function stack
s/handle situation/handle a situation
> (ops->func_stack) is being removed.
>
> Signed-off-by: Petr Mladek
> ---
Acked-by: Joe Lawrence
> Documentation/livepatch/cumulative-patches.txt | 11 ++
> Documentation/livepatch/livepatch.txt | 30 ---
> kernel/livepatch/core.c| 51
> --
> 3 files changed, 68 insertions(+), 24 deletions(-)
>
>
> [ ... snip ... ]
>
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index ba6e83a08209..3c150ab19b99 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -143,9 +143,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully
> supported by
> the kernel livepatching.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> -is in transition. Only a single patch (the topmost patch on the stack)
> -can be in transition at a given time. A patch can remain in transition
> -indefinitely, if any of the tasks are stuck in the initial patch state.
> +is in transition. Only a single patch can be in transition at a given
> +time. A patch can remain in transition indefinitely, if any of the tasks
> +are stuck in the initial patch state.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> @@ -329,9 +329,10 @@ The livepatch gets enabled by calling klp_enable_patch()
> typically from
> module_init() callback. The system will start using the new implementation
> of the patched functions at this stage.
>
> -First, the addresses of the patched functions are found according to their
> -names. The special relocations, mentioned in the section "New functions",
> -are applied. The relevant entries are created under
> +First, possible conflicts are checked for non-cummulative patches with
^^^
s/non-cummulative/non-cumulative
>
> [ ... snip ... ]
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 0ce752e9e8bb..d8f6f49ac6b3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -122,6 +122,47 @@ static struct klp_object *klp_find_object(struct
> klp_patch *patch,
> return NULL;
> }
>
> +static int klp_check_obj_conflict(struct klp_patch *patch,
> + struct klp_object *old_obj)
> +{
> + struct klp_object *obj;
> + struct klp_func *func, *old_func;
> +
> + obj = klp_find_object(patch, old_obj);
> + if (!obj)
> + return 0;
> +
> + klp_for_each_func(old_obj, old_func) {
> + func = klp_find_func(obj, old_func);
> + if (!func)
> + continue;
> +
> + pr_err("Function '%s,%lu' in object '%s' has already been
> livepatched.\n",
> +func->old_name, func->old_sympos ? func->old_sympos : 1,
> +obj->name ? obj->name : "vmlinux");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Should we add a self-test check for this condition? Let me know and I
can give it a go if you want to include with v15.
-- Joe
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, 29 Nov 2018, Petr Mladek wrote: > The atomic replace and cumulative patches were introduced as a more secure > way to handle dependent patches. They simplify the logic: > > + Any new cumulative patch is supposed to take over shadow variables > and changes made by callbacks from previous livepatches. > > + All replaced patches are discarded and the modules can be unloaded. > As a result, there is only one scenario when a cumulative livepatch > gets disabled. > > The different handling of "normal" and cumulative patches might cause > confusion. It would make sense to keep only one mode. On the other hand, > it would be rude to enforce using the cumulative livepatches even for > trivial and independent (hot) fixes. > > This patch removes the stack of patches. The list of enabled patches > is still needed but the ordering is not longer enforced. > > Note that it is not possible to catch all possible dependencies. It is > the responsibility of the livepatch authors to decide. > > Nevertheless this patch prevents having two patches for the same function > enabled at the same time after the transition finishes. It might help > to catch obvious mistakes. But more importantly, we do not need to > handle situation when a patch in the middle of the function stack > (ops->func_stack) is being removed. > > Signed-off-by: Petr Mladek Acked-by: Miroslav Benes M
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, 29 Nov 2018, Petr Mladek wrote: > The atomic replace and cumulative patches were introduced as a more secure > way to handle dependent patches. They simplify the logic: > > + Any new cumulative patch is supposed to take over shadow variables > and changes made by callbacks from previous livepatches. > > + All replaced patches are discarded and the modules can be unloaded. > As a result, there is only one scenario when a cumulative livepatch > gets disabled. > > The different handling of "normal" and cumulative patches might cause > confusion. It would make sense to keep only one mode. On the other hand, > it would be rude to enforce using the cumulative livepatches even for > trivial and independent (hot) fixes. > > This patch removes the stack of patches. The list of enabled patches > is still needed but the ordering is not longer enforced. > > Note that it is not possible to catch all possible dependencies. It is > the responsibility of the livepatch authors to decide. > > Nevertheless this patch prevents having two patches for the same function > enabled at the same time after the transition finishes. It might help > to catch obvious mistakes. But more importantly, we do not need to > handle situation when a patch in the middle of the function stack > (ops->func_stack) is being removed. > > Signed-off-by: Petr Mladek Acked-by: Miroslav Benes M
[PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
The atomic replace and cumulative patches were introduced as a more secure way to handle dependent patches. They simplify the logic: + Any new cumulative patch is supposed to take over shadow variables and changes made by callbacks from previous livepatches. + All replaced patches are discarded and the modules can be unloaded. As a result, there is only one scenario when a cumulative livepatch gets disabled. The different handling of "normal" and cumulative patches might cause confusion. It would make sense to keep only one mode. On the other hand, it would be rude to enforce using the cumulative livepatches even for trivial and independent (hot) fixes. This patch removes the stack of patches. The list of enabled patches is still needed but the ordering is not longer enforced. Note that it is not possible to catch all possible dependencies. It is the responsibility of the livepatch authors to decide. Nevertheless this patch prevents having two patches for the same function enabled at the same time after the transition finishes. It might help to catch obvious mistakes. But more importantly, we do not need to handle situation when a patch in the middle of the function stack (ops->func_stack) is being removed. Signed-off-by: Petr Mladek --- Documentation/livepatch/cumulative-patches.txt | 11 ++ Documentation/livepatch/livepatch.txt | 30 --- kernel/livepatch/core.c| 51 -- 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/Documentation/livepatch/cumulative-patches.txt b/Documentation/livepatch/cumulative-patches.txt index a8089f7fe306..ca1fbb4351c8 100644 --- a/Documentation/livepatch/cumulative-patches.txt +++ b/Documentation/livepatch/cumulative-patches.txt @@ -7,8 +7,8 @@ to do different changes to the same function(s) then we need to define an order in which the patches will be installed. And function implementations from any newer livepatch must be done on top of the older ones. -This might become a maintenance nightmare. Especially if anyone would want -to remove a patch that is in the middle of the stack. +This might become a maintenance nightmare. Especially when more patches +modified the same function in different ways. An elegant solution comes with the feature called "Atomic Replace". It allows to create so called "Cumulative Patches". They include all wanted changes @@ -26,11 +26,9 @@ for example: .replace = true, }; -Such a patch is added on top of the livepatch stack when enabled. - All processes are then migrated to use the code only from the new patch. Once the transition is finished, all older patches are automatically -disabled and removed from the stack of patches. +disabled. Ftrace handlers are transparently removed from functions that are no longer modified by the new cumulative patch. @@ -57,8 +55,7 @@ The atomic replace allows: + Remove eventual performance impact caused by core redirection for functions that are no longer patched. - + Decrease user confusion about stacking order and what code -is actually in use. + + Decrease user confusion about dependencies between livepatches. Limitations: diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index ba6e83a08209..3c150ab19b99 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -143,9 +143,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully supported by the kernel livepatching. The /sys/kernel/livepatch//transition file shows whether a patch -is in transition. Only a single patch (the topmost patch on the stack) -can be in transition at a given time. A patch can remain in transition -indefinitely, if any of the tasks are stuck in the initial patch state. +is in transition. Only a single patch can be in transition at a given +time. A patch can remain in transition indefinitely, if any of the tasks +are stuck in the initial patch state. A transition can be reversed and effectively canceled by writing the opposite value to the /sys/kernel/livepatch//enabled file while @@ -329,9 +329,10 @@ The livepatch gets enabled by calling klp_enable_patch() typically from module_init() callback. The system will start using the new implementation of the patched functions at this stage. -First, the addresses of the patched functions are found according to their -names. The special relocations, mentioned in the section "New functions", -are applied. The relevant entries are created under +First, possible conflicts are checked for non-cummulative patches with +disabled replace flag. The addresses of the patched functions are found +according to their names. The special relocations, mentioned in the section +"New functions", are applied. The relevant entries are created under /sys/kernel/livepatch/. The patch is rejected when any above operation fails. @@ -345,11
[PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
The atomic replace and cumulative patches were introduced as a more secure way to handle dependent patches. They simplify the logic: + Any new cumulative patch is supposed to take over shadow variables and changes made by callbacks from previous livepatches. + All replaced patches are discarded and the modules can be unloaded. As a result, there is only one scenario when a cumulative livepatch gets disabled. The different handling of "normal" and cumulative patches might cause confusion. It would make sense to keep only one mode. On the other hand, it would be rude to enforce using the cumulative livepatches even for trivial and independent (hot) fixes. This patch removes the stack of patches. The list of enabled patches is still needed but the ordering is not longer enforced. Note that it is not possible to catch all possible dependencies. It is the responsibility of the livepatch authors to decide. Nevertheless this patch prevents having two patches for the same function enabled at the same time after the transition finishes. It might help to catch obvious mistakes. But more importantly, we do not need to handle situation when a patch in the middle of the function stack (ops->func_stack) is being removed. Signed-off-by: Petr Mladek --- Documentation/livepatch/cumulative-patches.txt | 11 ++ Documentation/livepatch/livepatch.txt | 30 --- kernel/livepatch/core.c| 51 -- 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/Documentation/livepatch/cumulative-patches.txt b/Documentation/livepatch/cumulative-patches.txt index a8089f7fe306..ca1fbb4351c8 100644 --- a/Documentation/livepatch/cumulative-patches.txt +++ b/Documentation/livepatch/cumulative-patches.txt @@ -7,8 +7,8 @@ to do different changes to the same function(s) then we need to define an order in which the patches will be installed. And function implementations from any newer livepatch must be done on top of the older ones. -This might become a maintenance nightmare. Especially if anyone would want -to remove a patch that is in the middle of the stack. +This might become a maintenance nightmare. Especially when more patches +modified the same function in different ways. An elegant solution comes with the feature called "Atomic Replace". It allows to create so called "Cumulative Patches". They include all wanted changes @@ -26,11 +26,9 @@ for example: .replace = true, }; -Such a patch is added on top of the livepatch stack when enabled. - All processes are then migrated to use the code only from the new patch. Once the transition is finished, all older patches are automatically -disabled and removed from the stack of patches. +disabled. Ftrace handlers are transparently removed from functions that are no longer modified by the new cumulative patch. @@ -57,8 +55,7 @@ The atomic replace allows: + Remove eventual performance impact caused by core redirection for functions that are no longer patched. - + Decrease user confusion about stacking order and what code -is actually in use. + + Decrease user confusion about dependencies between livepatches. Limitations: diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index ba6e83a08209..3c150ab19b99 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -143,9 +143,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully supported by the kernel livepatching. The /sys/kernel/livepatch//transition file shows whether a patch -is in transition. Only a single patch (the topmost patch on the stack) -can be in transition at a given time. A patch can remain in transition -indefinitely, if any of the tasks are stuck in the initial patch state. +is in transition. Only a single patch can be in transition at a given +time. A patch can remain in transition indefinitely, if any of the tasks +are stuck in the initial patch state. A transition can be reversed and effectively canceled by writing the opposite value to the /sys/kernel/livepatch//enabled file while @@ -329,9 +329,10 @@ The livepatch gets enabled by calling klp_enable_patch() typically from module_init() callback. The system will start using the new implementation of the patched functions at this stage. -First, the addresses of the patched functions are found according to their -names. The special relocations, mentioned in the section "New functions", -are applied. The relevant entries are created under +First, possible conflicts are checked for non-cummulative patches with +disabled replace flag. The addresses of the patched functions are found +according to their names. The special relocations, mentioned in the section +"New functions", are applied. The relevant entries are created under /sys/kernel/livepatch/. The patch is rejected when any above operation fails. @@ -345,11
