The atomic replace runs pre/post (un)install callbacks only from the new
livepatch. There are several reasons for this:
+ Simplicity: clear ordering of operations, no interactions between
old and new callbacks.
+ Reliability: only new livepatch knows what changes can already be made
by older livepatches and how to take over the state.
+ Testing: the atomic replace can be properly tested only when a newer
livepatch is available. It might be too late to fix unwanted effect
of callbacks from older livepatches.
It might happen that an older change is not enough and the same system
state has to be modified another way. Different changes need to get
distinguished by a version number added to struct klp_state.
The version can also be used to prevent loading incompatible livepatches.
The check is done when the livepatch is enabled. The rules are:
+ Any completely new system state modification is allowed.
+ System state modifications with the same or higher version are allowed
for already modified system states.
+ Cumulative livepatches must handle all system state modifications from
already installed livepatches.
+ Non-cumulative livepatches are allowed to touch already modified
system states.
Signed-off-by: Petr Mladek <[email protected]>
---
include/linux/livepatch.h | 2 ++
kernel/livepatch/core.c | 8 ++++++++
kernel/livepatch/state.c | 40 +++++++++++++++++++++++++++++++++++++++-
kernel/livepatch/state.h | 9 +++++++++
4 files changed, 58 insertions(+), 1 deletion(-)
create mode 100644 kernel/livepatch/state.h
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 591abdee30d7..8bc4c6cc3f3f 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -135,10 +135,12 @@ struct klp_object {
/**
* struct klp_state - state of the system modified by the livepatch
* @id: system state identifier (non zero)
+ * @version: version of the change (non-zero)
* @data: custom data
*/
struct klp_state {
int id;
+ int version;
void *data;
};
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 24c4a13bd26c..614642719825 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -21,6 +21,7 @@
#include <asm/cacheflush.h>
#include "core.h"
#include "patch.h"
+#include "state.h"
#include "transition.h"
/*
@@ -1003,6 +1004,13 @@ int klp_enable_patch(struct klp_patch *patch)
mutex_lock(&klp_mutex);
+ if(!klp_is_patch_compatible(patch)) {
+ pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
+ patch->mod->name);
+ mutex_unlock(&klp_mutex);
+ return -EINVAL;
+ }
+
ret = klp_init_patch_early(patch);
if (ret) {
mutex_unlock(&klp_mutex);
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
index f8822b71f96e..b54a69b9e4b4 100644
--- a/kernel/livepatch/state.c
+++ b/kernel/livepatch/state.c
@@ -12,7 +12,9 @@
#include "transition.h"
#define klp_for_each_state(patch, state) \
- for (state = patch->states; state && state->id; state++)
+ for (state = patch->states; \
+ state && state->id && state->version; \
+ state++)
/**
* klp_get_state() - get information about system state modified by
@@ -81,3 +83,39 @@ struct klp_state *klp_get_prev_state(int id)
return last_state;
}
EXPORT_SYMBOL_GPL(klp_get_prev_state);
+
+/* Check if the patch is able to deal with the given system state. */
+static bool klp_is_state_compatible(struct klp_patch *patch,
+ struct klp_state *state)
+{
+ struct klp_state *new_state;
+
+ new_state = klp_get_state(patch, state->id);
+
+ if (new_state)
+ return new_state->version < state->version ? false : true;
+
+ /* Cumulative livepatch must handle all already modified states. */
+ return patch->replace ? false : true;
+}
+
+/*
+ * Check if 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.
+ */
+bool klp_is_patch_compatible(struct klp_patch *patch)
+{
+ struct klp_patch *old_patch;
+ struct klp_state *state;
+
+
+ klp_for_each_patch(old_patch) {
+ klp_for_each_state(old_patch, state) {
+ if (!klp_is_state_compatible(patch, state))
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/kernel/livepatch/state.h b/kernel/livepatch/state.h
new file mode 100644
index 000000000000..49d9c16e8762
--- /dev/null
+++ b/kernel/livepatch/state.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LIVEPATCH_STATE_H
+#define _LIVEPATCH_STATE_H
+
+#include <linux/livepatch.h>
+
+bool klp_is_patch_compatible(struct klp_patch *patch);
+
+#endif /* _LIVEPATCH_STATE_H */
--
2.16.4
> +/* Check if the patch is able to deal with the given system state. */
> +static bool klp_is_state_compatible(struct klp_patch *patch,
> + struct klp_state *state)
> +{
> + struct klp_state *new_state;
> +
> + new_state = klp_get_state(patch, state->id);
> +
> + if (new_state)
> + return new_state->version < state->version ? false : true;
return new_state->version >= state->version;
?
> + /* Cumulative livepatch must handle all already modified states.
*/
> + return patch->replace ? false : true;
return !patch->replace;
?
> + * Check if 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.
> + */
> +bool klp_is_patch_compatible(struct klp_patch *patch)
make C=1 kernel/livepatch/state.o
kernel/livepatch/state.c:107:6: warning: symbol 'klp_is_patch_compatible' was not declared. Should it be static?
#include "state.h" in state.c would solve it.
Miroslav
On Tue, Jun 11, 2019 at 03:56:25PM +0200, Petr Mladek wrote:
> The atomic replace runs pre/post (un)install callbacks only from the new
> livepatch. There are several reasons for this:
>
> + Simplicity: clear ordering of operations, no interactions between
> old and new callbacks.
>
> + Reliability: only new livepatch knows what changes can already be made
> by older livepatches and how to take over the state.
>
> + Testing: the atomic replace can be properly tested only when a newer
> livepatch is available. It might be too late to fix unwanted effect
> of callbacks from older livepatches.
>
> It might happen that an older change is not enough and the same system
> state has to be modified another way. Different changes need to get
> distinguished by a version number added to struct klp_state.
>
> The version can also be used to prevent loading incompatible livepatches.
> The check is done when the livepatch is enabled. The rules are:
>
> + Any completely new system state modification is allowed.
>
> + System state modifications with the same or higher version are allowed
> for already modified system states.
>
More word play: would it be any clearer to drop the use of
"modification" when talking about klp_states? Sometimes I read
modification to mean a change to a klp_state itself rather than the
system at large.
In my mind, "modification" is implied, but I already know where this
patchset is going, so perhaps I'm just trying to be lazy and not type
the whole thing out :) I wish I could come up with a nice, succinct
alternative, but "state" or "klp_state" would work for me. /two cents
> + Cumulative livepatches must handle all system state modifications from
> already installed livepatches.
>
> + Non-cumulative livepatches are allowed to touch already modified
> system states.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> include/linux/livepatch.h | 2 ++
> kernel/livepatch/core.c | 8 ++++++++
> kernel/livepatch/state.c | 40 +++++++++++++++++++++++++++++++++++++++-
> kernel/livepatch/state.h | 9 +++++++++
> 4 files changed, 58 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 591abdee30d7..8bc4c6cc3f3f 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -135,10 +135,12 @@ struct klp_object {
> /**
> * struct klp_state - state of the system modified by the livepatch
> * @id: system state identifier (non zero)
> + * @version: version of the change (non-zero)
> * @data: custom data
> */
> struct klp_state {
> int id;
> + int version;
> void *data;
> };
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 24c4a13bd26c..614642719825 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -21,6 +21,7 @@
> #include <asm/cacheflush.h>
> #include "core.h"
> #include "patch.h"
> +#include "state.h"
> #include "transition.h"
>
> /*
> @@ -1003,6 +1004,13 @@ int klp_enable_patch(struct klp_patch *patch)
>
> mutex_lock(&klp_mutex);
>
> + if(!klp_is_patch_compatible(patch)) {
> + pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> + patch->mod->name);
> + mutex_unlock(&klp_mutex);
> + return -EINVAL;
> + }
> +
> ret = klp_init_patch_early(patch);
> if (ret) {
> mutex_unlock(&klp_mutex);
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index f8822b71f96e..b54a69b9e4b4 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -12,7 +12,9 @@
> #include "transition.h"
>
> #define klp_for_each_state(patch, state) \
> - for (state = patch->states; state && state->id; state++)
> + for (state = patch->states; \
> + state && state->id && state->version; \
> + state++)
Minor git bookkeeping here, but this could be moved to the patch that
introduced the macro.
>
> /**
> * klp_get_state() - get information about system state modified by
> @@ -81,3 +83,39 @@ struct klp_state *klp_get_prev_state(int id)
> return last_state;
> }
> EXPORT_SYMBOL_GPL(klp_get_prev_state);
> +
> +/* Check if the patch is able to deal with the given system state. */
> +static bool klp_is_state_compatible(struct klp_patch *patch,
> + struct klp_state *state)
> +{
> + struct klp_state *new_state;
> +
> + new_state = klp_get_state(patch, state->id);
> +
> + if (new_state)
> + return new_state->version < state->version ? false : true;
> +
> + /* Cumulative livepatch must handle all already modified states. */
> + return patch->replace ? false : true;
> +}
> +
> +/*
> + * Check if the new livepatch will not break the existing system states.
suggestion: "Check that the new livepatch will not break" or
"Check if the new livepatch will break"
-- Joe
On Fri, Jun 21, 2019 at 10:09:11AM -0400, Joe Lawrence wrote:
> More word play: would it be any clearer to drop the use of
> "modification" when talking about klp_states? Sometimes I read
> modification to mean a change to a klp_state itself rather than the
> system at large.
After reading through the rest of the series, maybe I was premature
about this. "System state modification" is used consistently throughout
the series, so without having any better suggestion, ignore my comment.
-- Joe
Petr Mladek <[email protected]> writes:
> ---
> include/linux/livepatch.h | 2 ++
> kernel/livepatch/core.c | 8 ++++++++
> kernel/livepatch/state.c | 40 +++++++++++++++++++++++++++++++++++++++-
> kernel/livepatch/state.h | 9 +++++++++
> 4 files changed, 58 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 591abdee30d7..8bc4c6cc3f3f 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -135,10 +135,12 @@ struct klp_object {
> /**
> * struct klp_state - state of the system modified by the livepatch
> * @id: system state identifier (non zero)
> + * @version: version of the change (non-zero)
> * @data: custom data
> */
> struct klp_state {
> int id;
> + int version;
> void *data;
> };
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 24c4a13bd26c..614642719825 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -21,6 +21,7 @@
> #include <asm/cacheflush.h>
> #include "core.h"
> #include "patch.h"
> +#include "state.h"
> #include "transition.h"
>
> /*
> @@ -1003,6 +1004,13 @@ int klp_enable_patch(struct klp_patch *patch)
>
> mutex_lock(&klp_mutex);
>
> + if(!klp_is_patch_compatible(patch)) {
> + pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> + patch->mod->name);
> + mutex_unlock(&klp_mutex);
> + return -EINVAL;
> + }
> +
> ret = klp_init_patch_early(patch);
> if (ret) {
> mutex_unlock(&klp_mutex);
Just as a remark: klp_reverse_transition() could still transition back
to a !klp_is_patch_compatible() patch.
I don't think it's much of a problem, because for live patches
introducing completely new states to the system, it is reasonable
to assume that they'll start applying incompatible changes only from
their ->post_patch(), I guess.
For state "upgrades" to higher versions, it's not so clear though and
some care will be needed. But I think these could still be handled
safely at the cost of some complexity in the new live patch's
->post_patch().
Another detail is that ->post_unpatch() will be called for the new live
patch which has been unpatched due to transition reversal and one would
have to be careful not to free shared state from under the older, still
active live patch. How would ->post_unpatch() distinguish between
transition reversal and "normal" live patch disabling? By
klp_get_prev_state() != NULL?
Perhaps transition reversal should be mentioned in the documentation?
Thanks,
Nicolai
--
SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB
21284 (AG Nürnberg)
On Tue 2019-06-11 15:56:25, Petr Mladek wrote:
> It might happen that an older change is not enough and the same system
> state has to be modified another way. Different changes need to get
> distinguished by a version number added to struct klp_state.
>
> The version can also be used to prevent loading incompatible livepatches.
> The check is done when the livepatch is enabled. The rules are:
>
> + Any completely new system state modification is allowed.
>
> + System state modifications with the same or higher version are allowed
> for already modified system states.
>
> + Cumulative livepatches must handle all system state modifications from
> already installed livepatches.
>
> + Non-cumulative livepatches are allowed to touch already modified
> system states.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> include/linux/livepatch.h | 2 ++
> kernel/livepatch/core.c | 8 ++++++++
> kernel/livepatch/state.c | 40 +++++++++++++++++++++++++++++++++++++++-
> kernel/livepatch/state.h | 9 +++++++++
> 4 files changed, 58 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 591abdee30d7..8bc4c6cc3f3f 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -135,10 +135,12 @@ struct klp_object {
> /**
> * struct klp_state - state of the system modified by the livepatch
> * @id: system state identifier (non zero)
> + * @version: version of the change (non-zero)
> * @data: custom data
> */
> struct klp_state {
> int id;
As suggested by Nicolay, there will be in v2:
unsigned long id;
> + int version;
It would make sense to make "version" unsigned as well.
I am just unsure about the size:
+ "unsigned long" looks like an overhead to me
+ "u8" might be enough
But I would stay on the safe side and use:
unsigned int version;
Is anyone against?
Best Regards,
Petr
Hi,
first, I am sorry that I answer this non-trivial mail so late.
I know that it might be hard to remember the context.
On Mon 2019-06-24 12:26:07, Nicolai Stange wrote:
> Petr Mladek <[email protected]> writes:
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 24c4a13bd26c..614642719825 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -1003,6 +1004,13 @@ int klp_enable_patch(struct klp_patch *patch)
> >
> > mutex_lock(&klp_mutex);
> >
> > + if(!klp_is_patch_compatible(patch)) {
> > + pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> > + patch->mod->name);
> > + mutex_unlock(&klp_mutex);
> > + return -EINVAL;
> > + }
> > +
> > ret = klp_init_patch_early(patch);
> > if (ret) {
> > mutex_unlock(&klp_mutex);
>
>
> Just as a remark: klp_reverse_transition() could still transition back
> to a !klp_is_patch_compatible() patch.
I am slightly confused. The new livepatch is enabled only when the new
states have the same or higher version. And only callbacks from
the new livepatch are used, including post_unpatch() when
the transition gets reverted.
The "compatible" livepatch should be able to handle all situations:
+ Modify the system state when it was not modified before.
+ Take over the system state when it has already been modified
by the previous livepatch.
+ Restore the previous state when the transition is reverted.
> I don't think it's much of a problem, because for live patches
> introducing completely new states to the system, it is reasonable
> to assume that they'll start applying incompatible changes only from
> their ->post_patch(), I guess.
>
> For state "upgrades" to higher versions, it's not so clear though and
> some care will be needed. But I think these could still be handled
> safely at the cost of some complexity in the new live patch's
> ->post_patch().
Just to be sure. The post_unpatch() from the new livepatch
will get called when the transitions is reverted. It should
be able to revert any changes made by its own pre_patch().
You are right that it will need some care. Especially because
the transition revert is not easy to test.
I think that this is the main reason why Joe would like
to introduce the sticky flag. It might be used to block
the transition revert and livepatch disabling when it would
be to complicated, error-prone, or even impossible.
> Another detail is that ->post_unpatch() will be called for the new live
> patch which has been unpatched due to transition reversal and one would
> have to be careful not to free shared state from under the older, still
> active live patch. How would ->post_unpatch() distinguish between
> transition reversal and "normal" live patch disabling? By
> klp_get_prev_state() != NULL?
Exactly. klp_get_prev_state() != NULL can be used in the
post_unpatch() to restore the original state when
the transition gets reverted.
See restore_console_loglevel() in lib/livepatch/test_klp_state2.c
> Perhaps transition reversal should be mentioned in the documentation?
Good point. I'll mention it in the documentation.
Best Regards,
Petr