Hi,
this is another piece in the puzzle that helps to maintain more
livepatches.
Especially pre/post (un)patch callbacks might change a system state.
Any newly installed livepatch has to somehow deal with system state
modifications done be already installed livepatches.
This patchset provides, hopefully, a simple and generic API that
helps to keep and pass information between the livepatches.
It is also usable to prevent loading incompatible livepatches.
There was also a related idea to add a sticky flag. It should be
easy to add it later. It would perfectly fit into the new struct
klp_state.
Petr Mladek (5):
livepatch: Keep replaced patches until post_patch callback is called
livepatch: Basic API to track system state changes
livepatch: Allow to distinguish different version of system state
changes
livepatch: Documentation of the new API for tracking system state
changes
livepatch: Selftests of the API for tracking system state changes
Documentation/livepatch/index.rst | 1 +
Documentation/livepatch/system-state.rst | 80 ++++++++++
include/linux/livepatch.h | 17 +++
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 44 ++++--
kernel/livepatch/core.h | 5 +-
kernel/livepatch/state.c | 121 +++++++++++++++
kernel/livepatch/state.h | 9 ++
kernel/livepatch/transition.c | 12 +-
lib/livepatch/Makefile | 5 +-
lib/livepatch/test_klp_state.c | 161 ++++++++++++++++++++
lib/livepatch/test_klp_state2.c | 190 ++++++++++++++++++++++++
lib/livepatch/test_klp_state3.c | 5 +
tools/testing/selftests/livepatch/Makefile | 3 +-
tools/testing/selftests/livepatch/test-state.sh | 180 ++++++++++++++++++++++
15 files changed, 814 insertions(+), 21 deletions(-)
create mode 100644 Documentation/livepatch/system-state.rst
create mode 100644 kernel/livepatch/state.c
create mode 100644 kernel/livepatch/state.h
create mode 100644 lib/livepatch/test_klp_state.c
create mode 100644 lib/livepatch/test_klp_state2.c
create mode 100644 lib/livepatch/test_klp_state3.c
create mode 100755 tools/testing/selftests/livepatch/test-state.sh
--
2.16.4
This is another step how to help maintaining more livepatches.
One big help was the atomic replace and cumulative livepatches. These
livepatches replaces the already installed ones. Therefore it should
be enough when each cumulative livepatch is consistent.
The problems might come with shadow variables and callbacks. They might
change the system behavior or state so that it is not longer safe to
go back and use an older livepatch or the original kernel code. Also
any new livepatch must be able to detect what changes have already been
done by the already installed livepatches.
This is where the livepatch system state tracking gets useful. It
allows to:
- find whether a system state has already been modified by
previous livepatches
- store data needed to manipulate and restore the system state
The information about the manipulated system states is stored in an
array of struct klp_state. There are two functions that allow
to find this structure for a given struct klp_patch or for
already installed (replaced) livepatches.
The dependencies are going to be solved by a version field added later.
The only important information is that it will be allowed to modify
the same state by more non-cumulative livepatches. It is the same logic
as that it is allowed to modify the same function several times.
The livepatch author is responsible for preventing incompatible
changes.
Signed-off-by: Petr Mladek <[email protected]>
---
include/linux/livepatch.h | 15 +++++++++
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/state.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 99 insertions(+), 1 deletion(-)
create mode 100644 kernel/livepatch/state.c
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index eeba421cc671..591abdee30d7 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -132,10 +132,21 @@ struct klp_object {
bool patched;
};
+/**
+ * struct klp_state - state of the system modified by the livepatch
+ * @id: system state identifier (non zero)
+ * @data: custom data
+ */
+struct klp_state {
+ int id;
+ void *data;
+};
+
/**
* struct klp_patch - patch structure for live patching
* @mod: reference to the live patch module
* @objs: object entries for kernel objects to be patched
+ * @states: system states that can get modified
* @replace: replace all actively used patches
* @list: list node for global list of actively used patches
* @kobj: kobject for sysfs resources
@@ -150,6 +161,7 @@ struct klp_patch {
/* external */
struct module *mod;
struct klp_object *objs;
+ struct klp_state *states;
bool replace;
/* internal */
@@ -220,6 +232,9 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
+struct klp_state *klp_get_state(struct klp_patch *patch, int id);
+struct klp_state *klp_get_prev_state(int id);
+
#else /* !CONFIG_LIVEPATCH */
static inline int klp_module_coming(struct module *mod) { return 0; }
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index cf9b5bcdb952..cf03d4bdfc66 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs := core.o patch.o shadow.o transition.o
+livepatch-objs := core.o patch.o shadow.o state.o transition.o
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
new file mode 100644
index 000000000000..f8822b71f96e
--- /dev/null
+++ b/kernel/livepatch/state.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * system_state.c - State of the system modified by livepatches
+ *
+ * Copyright (C) 2019 SUSE
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/livepatch.h>
+#include "core.h"
+#include "transition.h"
+
+#define klp_for_each_state(patch, state) \
+ for (state = patch->states; state && state->id; state++)
+
+/**
+ * klp_get_state() - get information about system state modified by
+ * the given patch
+ * @patch: livepatch that modifies the given system state
+ * @id: custom identifier of the modified system state
+ *
+ * Checks whether the given patch modifies to given system state.
+ *
+ * The function can be called either from pre/post (un)patch
+ * callbacks or from the kernel code added by the livepatch.
+ *
+ * Return: pointer to struct klp_state when found, otherwise NULL.
+ */
+struct klp_state *klp_get_state(struct klp_patch *patch, int id)
+{
+ struct klp_state *state;
+
+ klp_for_each_state(patch, state) {
+ if (state->id == id)
+ return state;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(klp_get_state);
+
+/**
+ * klp_get_prev_state() - get information about system state modified by
+ * the already installed livepatches
+ * @id: custom identifier of the modified system state
+ *
+ * Checks whether already installed livepatches modify the given
+ * system state.
+ *
+ * The same system state can be modified by more non-cumulative
+ * livepatches. It is expected that the latest livepatch has
+ * the most up-to-date information.
+ *
+ * The function can be called only during transition when a new
+ * livepatch is being enabled or when such a transition is reverted.
+ * It is typically called only from from pre/post (un)patch
+ * callbacks.
+ *
+ * Return: pointer to the latest struct klp_state from already
+ * installed livepatches, NULL when not found.
+ */
+struct klp_state *klp_get_prev_state(int id)
+{
+ struct klp_patch *patch;
+ struct klp_state *state, *last_state = NULL;
+
+ if (WARN_ON_ONCE(!klp_transition_patch))
+ return NULL;
+
+ klp_for_each_patch(patch) {
+ if (patch == klp_transition_patch)
+ goto out;
+
+ state = klp_get_state(patch, id);
+ if (state)
+ last_state = state;
+ }
+
+out:
+ return last_state;
+}
+EXPORT_SYMBOL_GPL(klp_get_prev_state);
--
2.16.4
On Tue, Jun 11, 2019 at 03:56:22PM +0200, Petr Mladek wrote:
> Hi,
>
> this is another piece in the puzzle that helps to maintain more
> livepatches.
>
> Especially pre/post (un)patch callbacks might change a system state.
> Any newly installed livepatch has to somehow deal with system state
> modifications done be already installed livepatches.
>
> This patchset provides, hopefully, a simple and generic API that
> helps to keep and pass information between the livepatches.
> It is also usable to prevent loading incompatible livepatches.
>
> There was also a related idea to add a sticky flag. It should be
> easy to add it later. It would perfectly fit into the new struct
> klp_state.
>
> Petr Mladek (5):
> livepatch: Keep replaced patches until post_patch callback is called
> livepatch: Basic API to track system state changes
> livepatch: Allow to distinguish different version of system state
> changes
> livepatch: Documentation of the new API for tracking system state
> changes
> livepatch: Selftests of the API for tracking system state changes
I confess I missed most of the previous discussion, but from a first
read-through this seems reasonable, and the code looks simple and
self-contained.
--
Josh
On Tue, Jun 11, 2019 at 03:56:22PM +0200, Petr Mladek wrote:
> Hi,
>
> this is another piece in the puzzle that helps to maintain more
> livepatches.
>
> Especially pre/post (un)patch callbacks might change a system state.
> Any newly installed livepatch has to somehow deal with system state
> modifications done be already installed livepatches.
>
> This patchset provides, hopefully, a simple and generic API that
> helps to keep and pass information between the livepatches.
> It is also usable to prevent loading incompatible livepatches.
>
Thanks for posting, Petr and aplogies for not getting to this RFC
earlier. I think this strikes a reasonable balance between the (too)
"simplified" versioning scheme that I posted a few weeks back, and what
I was afraid might have been too complicated callback-state-version
concept.
This RFC reads fairly straightforward and especially easy to review
given the included documentation and self-tests. I'll add a few
comments per patch, but again, I like how this came out.
> There was also a related idea to add a sticky flag. It should be
> easy to add it later. It would perfectly fit into the new struct
> klp_state.
I think so, too. It would indicate that the patch is introducing a
state which cannot be safely unloaded. But we can talk about that at a
later time if/when we want to add that wrinkle to klp_state.
-- Joe
On Tue, Jun 11, 2019 at 03:56:24PM +0200, Petr Mladek wrote:
> This is another step how to help maintaining more livepatches.
>
> One big help was the atomic replace and cumulative livepatches. These
> livepatches replaces the already installed ones. Therefore it should
nit: s/replaces/replaces
> be enough when each cumulative livepatch is consistent.
>
> The problems might come with shadow variables and callbacks. They might
> change the system behavior or state so that it is not longer safe to
nit: s/not longer safe/no longer safe
> go back and use an older livepatch or the original kernel code. Also
> any new livepatch must be able to detect what changes have already been
> done by the already installed livepatches.
>
> This is where the livepatch system state tracking gets useful. It
> allows to:
>
> - find whether a system state has already been modified by
> previous livepatches
>
> - store data needed to manipulate and restore the system state
>
> The information about the manipulated system states is stored in an
> array of struct klp_state. There are two functions that allow
> to find this structure for a given struct klp_patch or for
> already installed (replaced) livepatches.
>
suggestion: "Two new functions, klp_get_state() and
klp_get_prev_state(), can find this structure ..." or perhaps drop this
part altogether and let the future reader do a 'git show' or 'git log
-p' to see the code changes and the exact function names.
-- Joe
> The dependencies are going to be solved by a version field added later.
> The only important information is that it will be allowed to modify
> the same state by more non-cumulative livepatches. It is the same logic
> as that it is allowed to modify the same function several times.
> The livepatch author is responsible for preventing incompatible
> changes.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> include/linux/livepatch.h | 15 +++++++++
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/state.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 99 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.c
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index eeba421cc671..591abdee30d7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -132,10 +132,21 @@ struct klp_object {
> bool patched;
> };
>
> +/**
> + * struct klp_state - state of the system modified by the livepatch
> + * @id: system state identifier (non zero)
> + * @data: custom data
> + */
> +struct klp_state {
> + int id;
> + void *data;
> +};
> +
> /**
> * struct klp_patch - patch structure for live patching
> * @mod: reference to the live patch module
> * @objs: object entries for kernel objects to be patched
> + * @states: system states that can get modified
> * @replace: replace all actively used patches
> * @list: list node for global list of actively used patches
> * @kobj: kobject for sysfs resources
> @@ -150,6 +161,7 @@ struct klp_patch {
> /* external */
> struct module *mod;
> struct klp_object *objs;
> + struct klp_state *states;
> bool replace;
>
> /* internal */
> @@ -220,6 +232,9 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
>
> +struct klp_state *klp_get_state(struct klp_patch *patch, int id);
> +struct klp_state *klp_get_prev_state(int id);
> +
> #else /* !CONFIG_LIVEPATCH */
>
> static inline int klp_module_coming(struct module *mod) { return 0; }
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index cf9b5bcdb952..cf03d4bdfc66 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> -livepatch-objs := core.o patch.o shadow.o transition.o
> +livepatch-objs := core.o patch.o shadow.o state.o transition.o
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> new file mode 100644
> index 000000000000..f8822b71f96e
> --- /dev/null
> +++ b/kernel/livepatch/state.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * system_state.c - State of the system modified by livepatches
> + *
> + * Copyright (C) 2019 SUSE
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/livepatch.h>
> +#include "core.h"
> +#include "transition.h"
> +
> +#define klp_for_each_state(patch, state) \
> + for (state = patch->states; state && state->id; state++)
> +
> +/**
> + * klp_get_state() - get information about system state modified by
> + * the given patch
> + * @patch: livepatch that modifies the given system state
> + * @id: custom identifier of the modified system state
> + *
> + * Checks whether the given patch modifies to given system state.
> + *
> + * The function can be called either from pre/post (un)patch
> + * callbacks or from the kernel code added by the livepatch.
> + *
> + * Return: pointer to struct klp_state when found, otherwise NULL.
> + */
> +struct klp_state *klp_get_state(struct klp_patch *patch, int id)
> +{
> + struct klp_state *state;
> +
> + klp_for_each_state(patch, state) {
> + if (state->id == id)
> + return state;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_get_state);
> +
> +/**
> + * klp_get_prev_state() - get information about system state modified by
> + * the already installed livepatches
> + * @id: custom identifier of the modified system state
> + *
> + * Checks whether already installed livepatches modify the given
> + * system state.
> + *
> + * The same system state can be modified by more non-cumulative
> + * livepatches. It is expected that the latest livepatch has
> + * the most up-to-date information.
> + *
> + * The function can be called only during transition when a new
> + * livepatch is being enabled or when such a transition is reverted.
> + * It is typically called only from from pre/post (un)patch
> + * callbacks.
> + *
> + * Return: pointer to the latest struct klp_state from already
> + * installed livepatches, NULL when not found.
> + */
> +struct klp_state *klp_get_prev_state(int id)
> +{
> + struct klp_patch *patch;
> + struct klp_state *state, *last_state = NULL;
> +
> + if (WARN_ON_ONCE(!klp_transition_patch))
> + return NULL;
> +
> + klp_for_each_patch(patch) {
> + if (patch == klp_transition_patch)
> + goto out;
> +
> + state = klp_get_state(patch, id);
> + if (state)
> + last_state = state;
> + }
> +
> +out:
> + return last_state;
> +}
> +EXPORT_SYMBOL_GPL(klp_get_prev_state);
> --
> 2.16.4
>
Hi Petr,
> this is another piece in the puzzle that helps to maintain more
> livepatches.
>
> Especially pre/post (un)patch callbacks might change a system state.
> Any newly installed livepatch has to somehow deal with system state
> modifications done be already installed livepatches.
>
> This patchset provides, hopefully, a simple and generic API that
> helps to keep and pass information between the livepatches.
> It is also usable to prevent loading incompatible livepatches.
I like it a lot, many thanks for doing this!
Minor remarks/questions will follow inline.
Nicolai
Petr Mladek <[email protected]> writes:
> ---
> include/linux/livepatch.h | 15 +++++++++
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/state.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 99 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.c
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index eeba421cc671..591abdee30d7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -132,10 +132,21 @@ struct klp_object {
> bool patched;
> };
>
> +/**
> + * struct klp_state - state of the system modified by the livepatch
> + * @id: system state identifier (non zero)
> + * @data: custom data
> + */
> +struct klp_state {
> + int id;
Can we make this an unsigned long please? It would be consistent with
shadow variable ids and would give more room for encoding bugzilla or
CVE numbers or so.
Nicolai
> + void *data;
> +};
> +