* [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
@ 2019-09-28 15:12 Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 01/12] livepatch: Always check hypervisor build ID upon livepatch upload Pawel Wieczorkiewicz
` (12 more replies)
0 siblings, 13 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, wipawel, Stefano Stabellini,
Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Ross Lagerwall, Ian Jackson, mpohlack, Tim Deegan,
Pawel Wieczorkiewicz, Julien Grall, Jan Beulich
This series introduces new features to the livepatch functionality as
briefly discussed during Xen Developer Summit 2019: [a] and [b].
It also provides a few fixes and some small improvements.
Main changes in v4:
- Fix various typos and minor issues
- Simplify arch_livepatch_{apply,revert} by using
common_livepatch_{apply,revert}
- Improve python bindings and fix few issues
Main changes in v3:
- Fix expectation test to work on Arm
- Add test for metadata (Konrad)
- Minor fixes to documentation
Main changes in v2:
- added new features to livepatch documentation
- added livepatch tests
- enabled Arm support for [5]
- make .modinfo optional for [11]
- fixed typos
FEATURES:
1. independent modules (patches: [1], [2])
* livepatch-build-tools repo dependency [A]
Livepatch enforces the following buildid-based dependency chain
between hotpatch modules:
1) first module depends on given hypervisor buildid
2) every consecutive module depends on previous module's buildid
This way proper hotpatch stack order is maintained and enforced.
While it is important for production hotpatches it limits agility and
blocks usage of testing or debug hotpatches. These kinds of hotpatch
modules are typically expected to be loaded at any time irrespective
of current state of the modules stack.
[A] livepatch-build: Embed hypervisor build id into every hotpatch
2. pre- and post- apply|revert actions hooks (patches: [3], [4])
* livepatch-build-tools repo dependency [B]
This is an implementation of 4 new livepatch module vetoing hooks,
that can be optionally supplied along with modules.
Hooks that currently exists in the livepatch mechanism aren't agile
enough and have various limitations:
* run only from within a quiescing zone
* cannot conditionally prevent applying or reverting
* do not have access to the module context
To address these limitations the following has been implemented:
1) pre-apply hook
2) post-apply hook
3) pre-revert hook
4) post-revert hook
[B] create-diff-object: Handle extra pre-|post- hooks
3. apply|revert actions replacement hooks (patches: [5], [6], [7])
* livepatch-build-tools repo dependency: [C], [D], [E]
To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.
[C] create-diff-object: Do not create empty .livepatch.funcs section
[D] create-diff-object: Handle optional apply|revert hooks
[E] create-diff-object: Add support for applied/reverted marker
4. inline asm hotpatching expectations (patches: [8])
* livepatch-build-tools repo dependency: [F]
Expectations are designed as optional feature, since the main use of
them is planned for inline asm hotpatching.
The payload structure is modified as each expectation structure is
part of the livepatch_func structure and hence extends the payload.
The payload version is bumped to 3 with this change to highlight the
ABI modification and enforce proper support.
The expectation is manually enabled during inline asm module
construction. If enabled, expectation ensures that the expected
content of memory is to be found at a given patching (old_addr)
location.
[F] create-diff-object: Add support for expectations
5. runtime hotpatch metadata support (patches: [9], [10], [11])
Having detailed hotpatch metadata helps to properly identify module's
origin and version. It also allows to keep track of the history of
hotpatch loads in the system (at least within dmesg buffer size
limits).
Extend the livepatch list operation to fetch also payloads' metadata.
This is achieved by extending the sysctl list interface with 2 extra
guest handles:
* metadata - an array of arbitrary size strings
* metadata_len - an array of metadata strings' lengths (uin32_t each)
To unify and simplify the interface, handle the modules' name strings
of arbitrary size by copying them in adhering chunks to the userland.
6. python bindings for livepatch operations (patches: [12])
Extend the XC python bindings library to support all common livepatch
operations and actions:
- status (pyxc_livepatch_status):
- action (pyxc_livepatch_action):
- upload (pyxc_livepatch_upload):
- list (pyxc_livepatch_list):
[a] https://wiki.xenproject.org/wiki/Design_Sessions_2019#LivePatch_improvements_and_features
[b] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00846.html
Merged in v1:
python: Add XC binding for Xen build ID
livepatch: always print XENLOG_ERR information
Pawel Wieczorkiewicz (12):
[1] livepatch: Always check hypervisor build ID upon hotpatch upload
[2] livepatch: Allow to override inter-modules buildid dependency
[3] livepatch: Export payload structure via livepatch_payload.h
[4] livepatch: Implement pre-|post- apply|revert hooks
[5] livepatch: Add support for apply|revert action replacement hooks
[6] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence
[7] livepatch: Add per-function applied/reverted state tracking marker
[8] livepatch: Add support for inline asm hotpatching expectations
[9] livepatch: Add support for modules .modinfo section metadata
[10] livepatch: Handle arbitrary size names with the list operation
[11] livepatch: Add metadata runtime retrieval mechanism
[12] livepatch: Add python bindings for livepatch operations
.gitignore | 6 +-
docs/misc/livepatch.pandoc | 240 ++++++++-
tools/libxc/include/xenctrl.h | 68 ++-
tools/libxc/xc_misc.c | 162 ++++--
tools/misc/xen-livepatch.c | 257 +++++++---
tools/python/xen/lowlevel/xc/xc.c | 267 ++++++++++
xen/common/livepatch.c | 653 +++++++++++++++++++++----
xen/include/public/sysctl.h | 62 ++-
xen/include/xen/livepatch.h | 43 +-
xen/include/xen/livepatch_payload.h | 83 ++++
xen/test/livepatch/Makefile | 121 ++++-
xen/test/livepatch/xen_action_hooks.c | 102 ++++
xen/test/livepatch/xen_action_hooks_marker.c | 112 +++++
xen/test/livepatch/xen_action_hooks_noapply.c | 136 +++++
xen/test/livepatch/xen_action_hooks_nofunc.c | 86 ++++
xen/test/livepatch/xen_action_hooks_norevert.c | 143 ++++++
xen/test/livepatch/xen_expectations.c | 41 ++
xen/test/livepatch/xen_expectations_fail.c | 42 ++
xen/test/livepatch/xen_prepost_hooks.c | 122 +++++
xen/test/livepatch/xen_prepost_hooks_fail.c | 75 +++
20 files changed, 2542 insertions(+), 279 deletions(-)
create mode 100644 xen/test/livepatch/xen_action_hooks.c
create mode 100644 xen/test/livepatch/xen_action_hooks_marker.c
create mode 100644 xen/test/livepatch/xen_action_hooks_noapply.c
create mode 100644 xen/test/livepatch/xen_action_hooks_nofunc.c
create mode 100644 xen/test/livepatch/xen_action_hooks_norevert.c
create mode 100644 xen/test/livepatch/xen_expectations.c
create mode 100644 xen/test/livepatch/xen_expectations_fail.c
create mode 100644 xen/test/livepatch/xen_prepost_hooks.c
create mode 100644 xen/test/livepatch/xen_prepost_hooks_fail.c
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 01/12] livepatch: Always check hypervisor build ID upon livepatch upload
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency Pawel Wieczorkiewicz
` (11 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
This change is part of a independant stacked livepatch modules
feature. This feature allows to bypass dependencies between modules
upon loading, but still verifies Xen build ID matching.
In order to prevent (up)loading any livepatches built for different
hypervisor version as indicated by the Xen Build ID, add checking for
the payload's vs Xen's build id match.
To achieve that embed into every livepatch another section with a
dedicated hypervisor build id in it. After the payload is loaded and
the .livepatch.xen_depends section becomes available, perform the
check and reject the payload if there is no match.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v3:
* renamed check_xen_build_id() to xen_build_id_dep()
Changed since v1:
* always print XENLOG_ERR message from check_xen_build_id()
* fix typo in test/livepatch/Makefile
.gitignore | 1 +
docs/misc/livepatch.pandoc | 28 +++++++++++++++++++--------
xen/common/livepatch.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
xen/include/xen/livepatch.h | 7 ++++---
xen/test/livepatch/Makefile | 31 +++++++++++++++++++++++++-----
5 files changed, 98 insertions(+), 16 deletions(-)
diff --git a/.gitignore b/.gitignore
index 3c947ac948..6f83fc8728 100644
--- a/.gitignore
+++ b/.gitignore
@@ -312,6 +312,7 @@ xen/test/livepatch/xen_bye_world.livepatch
xen/test/livepatch/xen_hello_world.livepatch
xen/test/livepatch/xen_nop.livepatch
xen/test/livepatch/xen_replace_world.livepatch
+xen/test/livepatch/xen_no_xen_buildid.livepatch
xen/tools/kconfig/.tmp_gtkcheck
xen/tools/kconfig/.tmp_qtcheck
xen/tools/symbols
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index 6d9f72f49b..fd1f5d0126 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -270,6 +270,8 @@ like what the Linux kernel module loader does.
The payload contains at least three sections:
* `.livepatch.funcs` - which is an array of livepatch_func structures.
+ * `.livepatch.xen_depends` - which is an ELF Note that describes what Xen
+ build-id the payload depends on. **MUST** have one.
* `.livepatch.depends` - which is an ELF Note that describes what the payload
depends on. **MUST** have one.
* `.note.gnu.build-id` - the build-id of this payload. **MUST** have one.
@@ -383,16 +385,16 @@ The type definition of the function are as follow:
typedef void (*livepatch_loadcall_t)(void);
typedef void (*livepatch_unloadcall_t)(void);
-### .livepatch.depends and .note.gnu.build-id
+### .livepatch.xen_depends, .livepatch.depends and .note.gnu.build-id
To support dependencies checking and safe loading (to load the
appropiate payload against the right hypervisor) there is a need
to embbed an build-id dependency.
-This is done by the payload containing an section `.livepatch.depends`
-which follows the format of an ELF Note. The contents of this
-(name, and description) are specific to the linker utilized to
-build the hypevisor and payload.
+This is done by the payload containing sections `.livepatch.xen_depends`
+and `.livepatch.depends` which follow the format of an ELF Note.
+The contents of these (name, and description) are specific to the linker
+utilized to build the hypevisor and payload.
If GNU linker is used then the name is `GNU` and the description
is a NT_GNU_BUILD_ID type ID. The description can be an SHA1
@@ -400,6 +402,13 @@ checksum, MD5 checksum or any unique value.
The size of these structures varies with the `--build-id` linker option.
+There are two kinds of build-id dependencies:
+
+ * Xen build-id dependency (.livepatch.xen_depends section)
+ * previous payload build-id dependency (.livepatch.depends section)
+
+See "Live patch interdependencies" for more information.
+
## Hypercalls
We will employ the sub operations of the system management hypercall (sysctl).
@@ -894,13 +903,16 @@ but is more complex to implement.
The second option which requires an build-id of the hypervisor
is implemented in the Xen hypervisor.
-Specifically each payload has two build-id ELF notes:
+Specifically each payload has three build-id ELF notes:
* The build-id of the payload itself (generated via --build-id).
+ * The build-id of the Xen hypervisor it depends on (extracted from the
+ hypervisor during build time).
* The build-id of the payload it depends on (extracted from the
the previous payload or hypervisor during build time).
-This means that the very first payload depends on the hypervisor
-build-id.
+This means that every payload depends on the hypervisor build-id and on
+the build-id of the previous payload in the stack.
+The very first payload depends on the hypervisor build-id only.
# Not Yet Done
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 7caa30c202..163c9c79ea 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -74,6 +74,7 @@ struct payload {
unsigned int nsyms; /* Nr of entries in .strtab and symbols. */
struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */
+ struct livepatch_build_id xen_dep; /* ELFNOTE_DESC(.livepatch.xen_depends). */
livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
@@ -476,11 +477,34 @@ static bool section_ok(const struct livepatch_elf *elf,
return true;
}
+static int xen_build_id_dep(const struct payload *payload)
+{
+ const void *id = NULL;
+ unsigned int len = 0;
+ int rc;
+
+ ASSERT(payload->xen_dep.len);
+ ASSERT(payload->xen_dep.p);
+
+ rc = xen_build_id(&id, &len);
+ if ( rc )
+ return rc;
+
+ if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len) ) {
+ printk(XENLOG_ERR LIVEPATCH "%s: check against hypervisor build-id failed\n",
+ payload->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int check_special_sections(const struct livepatch_elf *elf)
{
unsigned int i;
static const char *const names[] = { ELF_LIVEPATCH_FUNC,
ELF_LIVEPATCH_DEPENDS,
+ ELF_LIVEPATCH_XEN_DEPENDS,
ELF_BUILD_ID_NOTE};
DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
@@ -632,6 +656,22 @@ static int prepare_payload(struct payload *payload,
return -EINVAL;
}
+ sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
+ if ( sec )
+ {
+ n = sec->load_addr;
+
+ if ( sec->sec->sh_size <= sizeof(*n) )
+ return -EINVAL;
+
+ if ( xen_build_id_check(n, sec->sec->sh_size,
+ &payload->xen_dep.p, &payload->xen_dep.len) )
+ return -EINVAL;
+
+ if ( !payload->xen_dep.len || !payload->xen_dep.p )
+ return -EINVAL;
+ }
+
/* Setup the virtual region with proper data. */
region = &payload->region;
@@ -882,6 +922,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
if ( rc )
goto out;
+ rc = xen_build_id_dep(payload);
+ if ( rc )
+ goto out;
+
rc = build_symbol_table(payload, &elf);
if ( rc )
goto out;
@@ -1655,6 +1699,9 @@ static void livepatch_printall(unsigned char key)
if ( data->dep.len )
printk("depend-on=%*phN\n", data->dep.len, data->dep.p);
+
+ if ( data->xen_dep.len )
+ printk("depend-on-xen=%*phN\n", data->xen_dep.len, data->xen_dep.p);
}
spin_unlock(&payload_lock);
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 1b1817ca0d..ed997aa4cc 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -29,9 +29,10 @@ struct xen_sysctl_livepatch_op;
/* Convenience define for printk. */
#define LIVEPATCH "livepatch: "
/* ELF payload special section names. */
-#define ELF_LIVEPATCH_FUNC ".livepatch.funcs"
-#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
-#define ELF_BUILD_ID_NOTE ".note.gnu.build-id"
+#define ELF_LIVEPATCH_FUNC ".livepatch.funcs"
+#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
+#define ELF_LIVEPATCH_XEN_DEPENDS ".livepatch.xen_depends"
+#define ELF_BUILD_ID_NOTE ".note.gnu.build-id"
/* Arbitrary limit for payload size and .bss section size. */
#define LIVEPATCH_MAX_SIZE MB(2)
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 6831383db1..938aee17ec 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -19,11 +19,13 @@ LIVEPATCH := xen_hello_world.livepatch
LIVEPATCH_BYE := xen_bye_world.livepatch
LIVEPATCH_REPLACE := xen_replace_world.livepatch
LIVEPATCH_NOP := xen_nop.livepatch
+LIVEPATCH_NO_XEN_BUILDID := xen_no_xen_buildid.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
LIVEPATCHES += $(LIVEPATCH_REPLACE)
LIVEPATCHES += $(LIVEPATCH_NOP)
+LIVEPATCHES += $(LIVEPATCH_NO_XEN_BUILDID)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -59,7 +61,7 @@ config.h: xen_hello_world_func.o
xen_hello_world.o: config.h
.PHONY: $(LIVEPATCH)
-$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
+$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
#
@@ -78,6 +80,17 @@ note.o:
--rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
rm -f $@.bin
+#
+# Append .livepatch.xen_depends section
+# with Xen build-id derived from xen-syms.
+#
+.PHONY: xen_note.o
+xen_note.o:
+ $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
+ $(OBJCOPY) $(OBJCOPY_MAGIC) \
+ --rename-section=.data=.livepatch.xen_depends,alloc,load,readonly,data,contents -S $@.bin $@
+ rm -f $@.bin
+
#
# Extract the build-id of the xen_hello_world.livepatch
# (which xen_bye_world will depend on).
@@ -92,20 +105,28 @@ hello_world_note.o: $(LIVEPATCH)
xen_bye_world.o: config.h
.PHONY: $(LIVEPATCH_BYE)
-$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o
+$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
xen_replace_world.o: config.h
.PHONY: $(LIVEPATCH_REPLACE)
-$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o
+$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
xen_nop.o: config.h
.PHONY: $(LIVEPATCH_NOP)
-$(LIVEPATCH_NOP): xen_nop.o note.o
+$(LIVEPATCH_NOP): xen_nop.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+# This one always fails upon upload, because it deliberately
+# does not have a .livepatch.xen_depends (xen_note.o) section.
+xen_no_xen_buildid.o: config.h
+
+.PHONY: $(LIVEPATCH_NO_XEN_BUILDID)
+$(LIVEPATCH_NO_XEN_BUILDID): xen_nop.o note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NO_XEN_BUILDID) $^
+
.PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID)
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 01/12] livepatch: Always check hypervisor build ID upon livepatch upload Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-09-30 11:05 ` Jan Beulich
2019-11-08 10:36 ` Ross Lagerwall
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 03/12] livepatch: Export payload structure via livepatch_payload.h Pawel Wieczorkiewicz
` (10 subsequent siblings)
12 siblings, 2 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
By default Livepatch enforces the following buildid-based dependency
chain between livepatch modules:
1) first module depends on given hypervisor buildid
2) every consecutive module depends on previous module's buildid
This way proper livepatch stack order is maintained and enforced.
While it is important for production livepatches it limits agility and
blocks usage of testing or debug livepatches. These kinds of livepatch
modules are typically expected to be loaded at any time irrespective
of current state of the modules stack.
To enable testing and debug livepatches allow user dynamically ignore
the inter-modules dependency. In this case only hypervisor buildid
match is verified and enforced.
To allow userland pass additional paremeters for livepatch actions
add support for action flags.
Each of the apply, revert, unload and revert action gets additional
64-bit parameter 'flags' where extra flags can be applied in a mask
form.
Initially only one flag '--nodeps' is added for the apply action.
This flag modifies the default buildid dependency check as described
above.
The global sysctl interface input flag parameter is defined with a
single corresponding flag macro:
LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
The userland xen-livepatch tool is modified to support the '--nodeps'
flag for apply and load commands. A general mechanism for specifying
more flags in the future for apply and other action is however added.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Changed since v3:
* simplified loop in xen-livepatch.c
tools/libxc/include/xenctrl.h | 9 ++--
tools/libxc/xc_misc.c | 20 +++----
tools/misc/xen-livepatch.c | 121 +++++++++++++++++++++++++++++++++++-------
xen/common/livepatch.c | 14 +++--
xen/include/public/sysctl.h | 11 +++-
5 files changed, 139 insertions(+), 36 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c92386aab8..2fc62422f5 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2598,11 +2598,12 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
* to complete them. The `timeout` offers an option to expire the
* operation if it could not be completed within the specified time
* (in ns). Value of 0 means let hypervisor decide the best timeout.
+ * The `flags` allows to pass extra parameters to the actions.
*/
-int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout);
-int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout);
-int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout);
-int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout);
+int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
+int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
+int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
+int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
/*
* Ensure cache coherency after memory modifications. A call to this function
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 8e60b6e9f0..a8e9e7d1e2 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -854,7 +854,8 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
static int _xc_livepatch_action(xc_interface *xch,
char *name,
unsigned int action,
- uint32_t timeout)
+ uint32_t timeout,
+ uint64_t flags)
{
int rc;
DECLARE_SYSCTL;
@@ -880,6 +881,7 @@ static int _xc_livepatch_action(xc_interface *xch,
sysctl.u.livepatch.pad = 0;
sysctl.u.livepatch.u.action.cmd = action;
sysctl.u.livepatch.u.action.timeout = timeout;
+ sysctl.u.livepatch.u.action.flags = flags;
sysctl.u.livepatch.u.action.name = def_name;
set_xen_guest_handle(sysctl.u.livepatch.u.action.name.name, name);
@@ -891,24 +893,24 @@ static int _xc_livepatch_action(xc_interface *xch,
return rc;
}
-int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout)
+int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags)
{
- return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout);
+ return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout, flags);
}
-int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout)
+int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags)
{
- return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT, timeout);
+ return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT, timeout, flags);
}
-int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout)
+int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags)
{
- return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD, timeout);
+ return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD, timeout, flags);
}
-int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout)
+int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags)
{
- return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout);
+ return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout, flags);
}
/*
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 3233472157..0eee94fd91 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -23,18 +23,23 @@ void show_help(void)
{
fprintf(stderr,
"xen-livepatch: live patching tool\n"
- "Usage: xen-livepatch <command> [args]\n"
+ "Usage: xen-livepatch <command> [args] [command-flags]\n"
" <name> An unique name of payload. Up to %d characters.\n"
"Commands:\n"
" help display this help\n"
" upload <name> <file> upload file <file> with <name> name\n"
" list list payloads uploaded.\n"
- " apply <name> apply <name> patch.\n"
+ " apply <name> [flags] apply <name> patch.\n"
+ " Supported flags:\n"
+ " --nodeps Disable inter-module buildid dependency check.\n"
+ " Check only against hypervisor buildid.\n"
" revert <name> revert name <name> patch.\n"
" replace <name> apply <name> patch and revert all others.\n"
" unload <name> unload name <name> patch.\n"
- " load <file> upload and apply <file>.\n"
- " name is the <file> name\n",
+ " load <file> [flags] upload and apply <file> with name as the <file> name\n"
+ " Supported flags:\n"
+ " --nodeps Disable inter-module buildid dependency check.\n"
+ " Check only against hypervisor buildid.\n",
XEN_LIVEPATCH_NAME_SIZE);
}
@@ -225,12 +230,13 @@ static int upload_func(int argc, char *argv[])
return rc;
}
-/* These MUST match to the 'action_options[]' array slots. */
+/* These MUST match to the 'action_options[]' and 'flag_options[]' array slots. */
enum {
ACTION_APPLY = 0,
ACTION_REVERT = 1,
ACTION_UNLOAD = 2,
ACTION_REPLACE = 3,
+ ACTION_NUM
};
struct {
@@ -238,7 +244,7 @@ struct {
int expected; /* The state to be in after the function. */
const char *name;
const char *verb;
- int (*function)(xc_interface *xch, char *name, uint32_t timeout);
+ int (*function)(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
} action_options[] = {
{ .allow = LIVEPATCH_STATE_CHECKED,
.expected = LIVEPATCH_STATE_APPLIED,
@@ -266,6 +272,66 @@ struct {
},
};
+/*
+ * This structure defines supported flag options for actions.
+ * It defines entries for each action and supports up to 64
+ * flags per action.
+ */
+struct {
+ const char *name;
+ const uint64_t flag;
+} flag_options[ACTION_NUM][8 * sizeof(uint64_t)] = {
+ { /* ACTION_APPLY */
+ { .name = "--nodeps",
+ .flag = LIVEPATCH_ACTION_APPLY_NODEPS,
+ },
+ },
+ { /* ACTION_REVERT */
+ },
+ { /* ACTION_UNLOAD */
+ },
+ { /* ACTION_REPLACE */
+ }
+};
+
+/*
+ * Parse user provided action flags.
+ * This function expects to only receive an array of input parameters being flags.
+ * Expected action is specified via idx paramater (index of flag_options[]).
+ */
+static int get_flags(int argc, char *argv[], unsigned int idx, uint64_t *flags)
+{
+ int i, j;
+
+ if ( !flags || idx >= ARRAY_SIZE(flag_options) )
+ return -1;
+
+ *flags = 0;
+ for ( i = 0; i < argc; i++ )
+ {
+ for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ )
+ {
+ if ( !flag_options[idx][j].name )
+ goto error;
+
+ if ( !strcmp(flag_options[idx][j].name, argv[i]) )
+ {
+ *flags |= flag_options[idx][j].flag;
+ break;
+ }
+ }
+
+ if ( j == ARRAY_SIZE(flag_options[idx]) )
+ goto error;
+ }
+
+ return 0;
+error:
+ fprintf(stderr, "Unsupported flag: %s.\n", argv[i]);
+ errno = EINVAL;
+ return errno;
+}
+
/* The hypervisor timeout for the live patching operation is 30 msec,
* but it could take some time for the operation to start, so wait twice
* that period. */
@@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx)
char name[XEN_LIVEPATCH_NAME_SIZE];
int rc;
xen_livepatch_status_t status;
+ uint64_t flags;
- if ( argc != 1 )
+ if ( argc < 1 )
{
show_help();
return -1;
@@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx)
if ( idx >= ARRAY_SIZE(action_options) )
return -1;
- if ( get_name(argc, argv, name) )
+ if ( get_name(argc--, argv++, name) )
+ return EINVAL;
+
+ if ( get_flags(argc, argv, idx, &flags) )
return EINVAL;
/* Check initial status. */
@@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
if ( action_options[idx].allow & status.state )
{
printf("%s %s... ", action_options[idx].verb, name);
- rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
+ rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, flags);
if ( rc )
{
int saved_errno = errno;
@@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx)
static int load_func(int argc, char *argv[])
{
- int rc;
- char *new_argv[2];
- char *path, *name, *lastdot;
+ int i, rc = ENOMEM;
+ char *upload_argv[2];
+ char **apply_argv, *path, *name, *lastdot;
- if ( argc != 1 )
+ if ( argc < 1 )
{
show_help();
return -1;
}
+
+ /* apply action has <id> [flags] input requirement, which must be constructed */
+ apply_argv = (char **) malloc(argc * sizeof(*apply_argv));
+ if ( !apply_argv )
+ return rc;
+
/* <file> */
- new_argv[1] = argv[0];
+ upload_argv[1] = argv[0];
/* Synthesize the <id> */
path = strdup(argv[0]);
@@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[])
lastdot = strrchr(name, '.');
if ( lastdot != NULL )
*lastdot = '\0';
- new_argv[0] = name;
+ upload_argv[0] = name;
+ apply_argv[0] = name;
- rc = upload_func(2 /* <id> <file> */, new_argv);
+ /* Fill in all user provided flags */
+ for ( i = 1; i < argc; i++ )
+ apply_argv[i] = argv[i];
+
+ rc = upload_func(2 /* <id> <file> */, upload_argv);
if ( rc )
- return rc;
+ goto error;
- rc = action_func(1 /* only <id> */, new_argv, ACTION_APPLY);
+ rc = action_func(argc, apply_argv, ACTION_APPLY);
if ( rc )
- action_func(1, new_argv, ACTION_UNLOAD);
+ action_func(1 /* only <id> */, upload_argv, ACTION_UNLOAD);
+error:
+ free(apply_argv);
free(path);
return rc;
}
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 163c9c79ea..d8ab3374c8 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1575,9 +1575,17 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
break;
}
- rc = build_id_dep(data, !!list_empty(&applied_list));
- if ( rc )
- break;
+ /*
+ * Check if action is issued with nodeps flags to ignore module
+ * stack dependencies.
+ */
+ if ( !(action->flags & LIVEPATCH_ACTION_APPLY_NODEPS) )
+ {
+ rc = build_id_dep(data, !!list_empty(&applied_list));
+ if ( rc )
+ break;
+ }
+
data->rc = -EAGAIN;
rc = schedule_work(data, action->cmd, action->timeout);
}
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 91c48dcae0..1b2b165a6d 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@
#include "domctl.h"
#include "physdev.h"
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
/*
* Read console content from Xen buffer ring.
@@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action {
/* hypervisor default. */
/* Or upper bound of time (ns) */
/* for operation to take. */
+
+/*
+ * Overwrite default inter-module buildid dependency chain enforcement.
+ * Check only if module is built for given hypervisor by comparing buildid.
+ */
+#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
+ uint64_t flags; /* IN: action flags. */
+ /* Provide additional parameters */
+ /* for an action. */
};
struct xen_sysctl_livepatch_op {
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 03/12] livepatch: Export payload structure via livepatch_payload.h
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 01/12] livepatch: Always check hypervisor build ID upon livepatch upload Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks Pawel Wieczorkiewicz
` (9 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: Pawel Wieczorkiewicz, wipawel, Ross Lagerwall, mpohlack,
Konrad Rzeszutek Wilk
The payload structure will be used by the new hooks implementation and
therefore its definition has to be exported via the livepatch_payload
header.
The new hooks will make use of the payload structure fields and the
hooks' pointers will also be defined in the payload structure, so
the structure along with all field definitions needs to be available
to the code being patched in.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/common/livepatch.c | 37 ----------------------------------
xen/include/xen/livepatch_payload.h | 40 +++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d8ab3374c8..df2315e7c7 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -45,43 +45,6 @@ static LIST_HEAD(applied_list);
static unsigned int payload_cnt;
static unsigned int payload_version = 1;
-/* To contain the ELF Note header. */
-struct livepatch_build_id {
- const void *p;
- unsigned int len;
-};
-
-struct payload {
- uint32_t state; /* One of the LIVEPATCH_STATE_*. */
- int32_t rc; /* 0 or -XEN_EXX. */
- bool reverted; /* Whether it was reverted. */
- bool safe_to_reapply; /* Can apply safely after revert. */
- struct list_head list; /* Linked to 'payload_list'. */
- const void *text_addr; /* Virtual address of .text. */
- size_t text_size; /* .. and its size. */
- const void *rw_addr; /* Virtual address of .data. */
- size_t rw_size; /* .. and its size (if any). */
- const void *ro_addr; /* Virtual address of .rodata. */
- size_t ro_size; /* .. and its size (if any). */
- unsigned int pages; /* Total pages for [text,rw,ro]_addr */
- struct list_head applied_list; /* Linked to 'applied_list'. */
- struct livepatch_func *funcs; /* The array of functions to patch. */
- unsigned int nfuncs; /* Nr of functions to patch. */
- const struct livepatch_symbol *symtab; /* All symbols. */
- const char *strtab; /* Pointer to .strtab. */
- struct virtual_region region; /* symbol, bug.frame patching and
- exception table (x86). */
- unsigned int nsyms; /* Nr of entries in .strtab and symbols. */
- struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
- struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */
- struct livepatch_build_id xen_dep; /* ELFNOTE_DESC(.livepatch.xen_depends). */
- livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
- livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
- unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
- unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */
- char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
-};
-
/* Defines an outstanding patching action. */
struct livepatch_work
{
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 4a1a96d054..99613af2db 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -4,6 +4,15 @@
#ifndef __XEN_LIVEPATCH_PAYLOAD_H__
#define __XEN_LIVEPATCH_PAYLOAD_H__
+#include <xen/virtual_region.h>
+
+/* To contain the ELF Note header. */
+struct livepatch_build_id {
+ const void *p;
+ unsigned int len;
+};
+
+typedef struct payload livepatch_payload_t;
/*
* The following definitions are to be used in patches. They are taken
@@ -12,6 +21,37 @@
typedef void livepatch_loadcall_t(void);
typedef void livepatch_unloadcall_t(void);
+struct payload {
+ uint32_t state; /* One of the LIVEPATCH_STATE_*. */
+ int32_t rc; /* 0 or -XEN_EXX. */
+ bool reverted; /* Whether it was reverted. */
+ bool safe_to_reapply; /* Can apply safely after revert. */
+ struct list_head list; /* Linked to 'payload_list'. */
+ const void *text_addr; /* Virtual address of .text. */
+ size_t text_size; /* .. and its size. */
+ const void *rw_addr; /* Virtual address of .data. */
+ size_t rw_size; /* .. and its size (if any). */
+ const void *ro_addr; /* Virtual address of .rodata. */
+ size_t ro_size; /* .. and its size (if any). */
+ unsigned int pages; /* Total pages for [text,rw,ro]_addr */
+ struct list_head applied_list; /* Linked to 'applied_list'. */
+ struct livepatch_func *funcs; /* The array of functions to patch. */
+ unsigned int nfuncs; /* Nr of functions to patch. */
+ const struct livepatch_symbol *symtab; /* All symbols. */
+ const char *strtab; /* Pointer to .strtab. */
+ struct virtual_region region; /* symbol, bug.frame patching and
+ exception table (x86). */
+ unsigned int nsyms; /* Nr of entries in .strtab and symbols. */
+ struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
+ struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */
+ struct livepatch_build_id xen_dep; /* ELFNOTE_DESC(.livepatch.xen_depends). */
+ livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
+ livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
+ unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
+ unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */
+ char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
+};
+
/*
* LIVEPATCH_LOAD_HOOK macro
*
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (2 preceding siblings ...)
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 03/12] livepatch: Export payload structure via livepatch_payload.h Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-11-08 10:43 ` Ross Lagerwall
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 05/12] livepatch: Add support for apply|revert action replacement hooks Pawel Wieczorkiewicz
` (8 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
This is an implementation of 4 new livepatch module vetoing hooks,
that can be optionally supplied along with modules.
Hooks that currently exists in the livepatch mechanism aren't agile
enough and have various limitations:
* run only from within a quiescing zone
* cannot conditionally prevent applying or reverting
* do not have access to the module context
To address these limitations the following has been implemented:
1) pre-apply hook
runs before the apply action is scheduled for execution. Its main
purpose is to prevent from applying a livepatch when certain
expected conditions aren't met or when mutating actions implemented
in the hook fail or cannot be executed.
2) post-apply hook
runs after the apply action has been executed and quiescing zone
exited. Its main purpose is to provide an ability to follow-up on
actions performed by the pre- hook, when module application was
successful or undo certain preparation steps of the pre- hook in
case of a failure. The success/failure error code is provided to
the post- hooks via the rc field of the payload structure.
3) pre-revert hook
runs before the revert action is scheduled for execution. Its main
purpose is to prevent from reverting a livepatch when certain
expected conditions aren't met or when mutating actions implemented
in the hook fail or cannot be executed.
4) post-revert hook
runs after the revert action has been executed and quiescing zone
exited. Its main purpose is to perform cleanup of all previously
executed mutating actions in order to restore the original system
state from before the current module application.
The success/failure error code is provided to the post- hooks via
the rc field of the payload structure.
The replace action performs atomically the following actions:
- revert all applied modules
- apply a single replacement module.
With the vetoing hooks in place various inter-hook dependencies may
arise. Also, during the revert part of the operation certain vetoing
hooks may detect failing conditions that previously were satisfied.
That could in turn lead to situation when the revert part must be
rolled back with all the pre- and post- hooks re-applied, which again
can't be guaranteed to always succeed.
The simplest response to this complication is to disallow the replace
action completely on modules with vetoing hooks.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Petre Eftime <epetre@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Changed since v3:
* s/hotpatch/livepatch/g
* s/bool_t/bool/g
Changed since v1:
* added corresponding documentation
* added tests
.gitignore | 6 +-
docs/misc/livepatch.pandoc | 90 +++++++++++++-
xen/common/livepatch.c | 179 +++++++++++++++++++++++++---
xen/include/xen/livepatch_payload.h | 27 +++++
xen/test/livepatch/Makefile | 19 ++-
xen/test/livepatch/xen_prepost_hooks.c | 122 +++++++++++++++++++
xen/test/livepatch/xen_prepost_hooks_fail.c | 75 ++++++++++++
7 files changed, 492 insertions(+), 26 deletions(-)
create mode 100644 xen/test/livepatch/xen_prepost_hooks.c
create mode 100644 xen/test/livepatch/xen_prepost_hooks_fail.c
diff --git a/.gitignore b/.gitignore
index 6f83fc8728..5b80e2bbd5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -308,11 +308,7 @@ xen/include/xen/acm_policy.h
xen/include/xen/compile.h
xen/include/xen/lib/x86/cpuid-autogen.h
xen/test/livepatch/config.h
-xen/test/livepatch/xen_bye_world.livepatch
-xen/test/livepatch/xen_hello_world.livepatch
-xen/test/livepatch/xen_nop.livepatch
-xen/test/livepatch/xen_replace_world.livepatch
-xen/test/livepatch/xen_no_xen_buildid.livepatch
+xen/test/livepatch/*.livepatch
xen/tools/kconfig/.tmp_gtkcheck
xen/tools/kconfig/.tmp_qtcheck
xen/tools/symbols
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index fd1f5d0126..5abddd0d27 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -23,6 +23,9 @@ The document is split in four sections:
* payload - telemetries of the old code along with binary blob of the new
function (if needed).
* reloc - telemetries contained in the payload to construct proper trampoline.
+ * hook - an auxiliary function being called before, during or after payload
+ application or revert.
+ * quiescing zone - period when all CPUs are lock-step with each other.
## History
@@ -270,6 +273,10 @@ like what the Linux kernel module loader does.
The payload contains at least three sections:
* `.livepatch.funcs` - which is an array of livepatch_func structures.
+ and/or any of:
+ * `.livepatch.hooks.{preapply,postapply,prerevert,postrevert}'
+ - which are a pointer to a hook function pointer.
+
* `.livepatch.xen_depends` - which is an ELF Note that describes what Xen
build-id the payload depends on. **MUST** have one.
* `.livepatch.depends` - which is an ELF Note that describes what the payload
@@ -330,12 +337,24 @@ When reverting a patch, the hypervisor iterates over each `livepatch_func`
and the core code copies the data from the undo buffer (private internal copy)
to `old_addr`.
-It optionally may contain the address of functions to be called right before
-being applied and after being reverted:
+It optionally may contain the address of hooks to be called right before
+being applied and after being reverted (while all CPUs are still in quiescing
+zone). These hooks do not have access to payload structure.
* `.livepatch.hooks.load` - an array of function pointers.
* `.livepatch.hooks.unload` - an array of function pointers.
+It optionally may also contain the address of pre- and post- vetoing hooks to
+be called before (pre) or after (post) apply and revert payload actions (while
+all CPUs are already released from quiescing zone). These hooks do have
+access to payload structure. The pre-apply hook can prevent from loading the
+payload if encoded in it condition is not met. Accordingly, the pre-revert
+hook can prevent from unloading the livepatch if encoded in it condition is not
+met.
+
+ * `.livepatch.hooks.{preapply,postapply}`
+ * `.livepatch.hooks.{prerevert,postrevert}`
+ - which are a pointer to a single hook function pointer.
### Example of .livepatch.funcs
@@ -371,7 +390,9 @@ A simple example of what a payload file can be:
Code must be compiled with `-fPIC`.
-### .livepatch.hooks.load and .livepatch.hooks.unload
+### Hooks
+
+#### .livepatch.hooks.load and .livepatch.hooks.unload
This section contains an array of function pointers to be executed
before payload is being applied (.livepatch.funcs) or after reverting
@@ -385,6 +406,69 @@ The type definition of the function are as follow:
typedef void (*livepatch_loadcall_t)(void);
typedef void (*livepatch_unloadcall_t)(void);
+#### .livepatch.hooks.preapply
+
+This section contains a pointer to a single function pointer to be executed
+before apply action is scheduled (and thereby before CPUs are put into
+quiescing zone). This is useful to prevent from applying a payload when
+certain expected conditions aren't met or when mutating actions implemented
+in the hook fail or cannot be executed.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+ typedef int livepatch_precall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.postapply
+
+This section contains a pointer to a single function pointer to be executed
+after apply action has finished and after all CPUs left the quiescing zone.
+This is useful to provide an ability to follow up on actions performed by
+the preapply hook. Especially, when module application was successful or to
+be able to undo certain preparation steps of the preapply hook in case of a
+failure. The success/failure error code is provided to the postapply hooks
+via the `rc` field of the payload structure.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+ typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.prerevert
+
+This section contains a pointer to a single function pointer to be executed
+before revert action is scheduled (and thereby before CPUs are put into
+quiescing zone). This is useful to prevent from reverting a payload when
+certain expected conditions aren't met or when mutating actions implemented
+in the hook fail or cannot be executed.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+ typedef int livepatch_precall_t(livepatch_payload_t *arg);
+
+#### .livepatch.hooks.postrevert
+
+This section contains a pointer to a single function pointer to be executed
+after revert action has finished and after all CPUs left the quiescing zone.
+This is useful to provide an ability to perform cleanup of all previously
+executed mutating actions in order to restore the original system state from
+before the current payload application. The success/failure error code is
+provided to the postrevert hook via the `rc` field of the payload structure.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+ typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
### .livepatch.xen_depends, .livepatch.depends and .note.gnu.build-id
To support dependencies checking and safe loading (to load the
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index df2315e7c7..e8ba9317b7 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -28,6 +28,8 @@
#include <asm/alternative.h>
#include <asm/event.h>
+#define is_hook_enabled(hook) ({ (hook) && *(hook); })
+
/*
* Protects against payload_list operations and also allows only one
* caller in schedule_work.
@@ -501,6 +503,35 @@ static int check_special_sections(const struct livepatch_elf *elf)
return 0;
}
+/*
+ * Lookup specified section and when exists assign its address to a specified hook.
+ * Perform section pointer and size validation: single hook sections must contain a
+ * single pointer only.
+ */
+#define LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, hook, section_name) do { \
+ const struct livepatch_elf_sec *__sec = livepatch_elf_sec_by_name(elf, section_name); \
+ if ( !__sec ) \
+ break; \
+ if ( !section_ok(elf, __sec, sizeof(*hook)) || __sec->sec->sh_size != sizeof(*hook) ) \
+ return -EINVAL; \
+ hook = __sec->load_addr; \
+} while (0)
+
+/*
+ * Lookup specified section and when exists assign its address to a specified hook.
+ * Perform section pointer and size validation: multi hook sections must contain an
+ * array whose size must be a multiple of the array's items size.
+ */
+#define LIVEPATCH_ASSIGN_MULTI_HOOK(elf, hook, nhooks, section_name) do { \
+ const struct livepatch_elf_sec *__sec = livepatch_elf_sec_by_name(elf, section_name); \
+ if ( !__sec ) \
+ break; \
+ if ( !section_ok(elf, __sec, sizeof(*hook)) ) \
+ return -EINVAL; \
+ hook = __sec->load_addr; \
+ nhooks = __sec->sec->sh_size / sizeof(*hook); \
+} while (0)
+
static int prepare_payload(struct payload *payload,
struct livepatch_elf *elf)
{
@@ -552,25 +583,14 @@ static int prepare_payload(struct payload *payload,
return rc;
}
- sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
- if ( sec )
- {
- if ( !section_ok(elf, sec, sizeof(*payload->load_funcs)) )
- return -EINVAL;
+ LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->load_funcs, payload->n_load_funcs, ".livepatch.hooks.load");
+ LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ".livepatch.hooks.unload");
- payload->load_funcs = sec->load_addr;
- payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
- }
-
- sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
- if ( sec )
- {
- if ( !section_ok(elf, sec, sizeof(*payload->unload_funcs)) )
- return -EINVAL;
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ".livepatch.hooks.preapply");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.post, ".livepatch.hooks.postapply");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.pre, ".livepatch.hooks.prerevert");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ".livepatch.hooks.postrevert");
- payload->unload_funcs = sec->load_addr;
- payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
- }
sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
if ( sec )
{
@@ -1217,6 +1237,39 @@ static bool_t is_work_scheduled(const struct payload *data)
return livepatch_work.do_work && livepatch_work.data == data;
}
+/*
+ * Check if payload has any of the vetoing, non-atomic hooks assigned.
+ * A vetoing, non-atmic hook may perform an operation that changes the
+ * hypervisor state and may not be guaranteed to succeed. Result of
+ * such operation may be returned and may change the livepatch workflow.
+ * Such hooks may require additional cleanup actions performed by other
+ * hooks. Thus they are not suitable for replace action.
+ */
+static inline bool has_payload_any_vetoing_hooks(const struct payload *payload)
+{
+ return is_hook_enabled(payload->hooks.apply.pre) ||
+ is_hook_enabled(payload->hooks.apply.post) ||
+ is_hook_enabled(payload->hooks.revert.pre) ||
+ is_hook_enabled(payload->hooks.revert.post);
+}
+
+/*
+ * Checks if any of the already applied livepatches has any vetoing,
+ * non-atomic hooks assigned.
+ */
+static inline bool livepatch_applied_have_vetoing_hooks(void)
+{
+ struct payload *p;
+
+ list_for_each_entry ( p, &applied_list, applied_list )
+ {
+ if ( has_payload_any_vetoing_hooks(p) )
+ return true;
+ }
+
+ return false;
+}
+
static int schedule_work(struct payload *data, uint32_t cmd, uint32_t timeout)
{
ASSERT(spin_is_locked(&payload_lock));
@@ -1317,6 +1370,7 @@ void check_for_livepatch_work(void)
{
struct payload *p;
unsigned int cpus;
+ bool action_done = false;
p = livepatch_work.data;
if ( !get_cpu_maps() )
@@ -1369,6 +1423,7 @@ void check_for_livepatch_work(void)
livepatch_do_action();
/* Serialize and flush out the CPU via CPUID instruction (on x86). */
arch_livepatch_post_action();
+ action_done = true;
local_irq_restore(flags);
}
@@ -1381,6 +1436,43 @@ void check_for_livepatch_work(void)
/* put_cpu_maps has an barrier(). */
put_cpu_maps();
+ if ( action_done )
+ {
+ switch ( livepatch_work.cmd )
+ {
+ case LIVEPATCH_ACTION_REVERT:
+ if ( is_hook_enabled(p->hooks.revert.post) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling post-revert hook function with rc=%d\n",
+ p->name, p->rc);
+
+ (*p->hooks.revert.post)(p);
+ }
+ break;
+
+ case LIVEPATCH_ACTION_APPLY:
+ if ( is_hook_enabled(p->hooks.apply.post) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling post-apply hook function with rc=%d\n",
+ p->name, p->rc);
+
+ (*p->hooks.apply.post)(p);
+ }
+ break;
+
+ case LIVEPATCH_ACTION_REPLACE:
+ if ( has_payload_any_vetoing_hooks(p) )
+ {
+ /* It should be impossible to get here since livepatch_action() guards against that. */
+ panic(LIVEPATCH "%s: REPLACE action is not supported on livepatches with vetoing hooks!\n",
+ p->name);
+ ASSERT_UNREACHABLE();
+ }
+ default:
+ break;
+ }
+ }
+
printk(XENLOG_INFO LIVEPATCH "%s finished %s with rc=%d\n",
p->name, names[livepatch_work.cmd], p->rc);
}
@@ -1516,6 +1608,21 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
rc = -EBUSY;
break;
}
+
+ if ( is_hook_enabled(data->hooks.revert.pre) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-revert hook function\n", data->name);
+
+ rc = (*data->hooks.revert.pre)(data);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: pre-revert hook failed (rc=%d), aborting!\n",
+ data->name, rc);
+ data->rc = rc;
+ break;
+ }
+ }
+
data->rc = -EAGAIN;
rc = schedule_work(data, action->cmd, action->timeout);
}
@@ -1549,6 +1656,20 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
break;
}
+ if ( is_hook_enabled(data->hooks.apply.pre) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-apply hook function\n", data->name);
+
+ rc = (*data->hooks.apply.pre)(data);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: pre-apply hook failed (rc=%d), aborting!\n",
+ data->name, rc);
+ data->rc = rc;
+ break;
+ }
+ }
+
data->rc = -EAGAIN;
rc = schedule_work(data, action->cmd, action->timeout);
}
@@ -1560,6 +1681,30 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
rc = build_id_dep(data, 1 /* against hypervisor. */);
if ( rc )
break;
+
+ /*
+ * REPLACE action is not supported on livepatches with vetoing hooks.
+ * Vetoing hooks usually perform mutating actions on the system and
+ * typically exist in pairs (pre- hook doing an action and post- hook
+ * undoing the action). Coalescing all hooks from all applied modules
+ * cannot be performed without inspecting potential dependencies between
+ * the mutating hooks and hence cannot be performed automatically by
+ * the replace action. Also, the replace action cannot safely assume a
+ * successful revert of all the module with vetoing hooks. When one
+ * of the hooks fails due to not meeting certain conditions the whole
+ * replace operation must have been reverted with all previous pre- and
+ * post- hooks re-executed (which cannot be guaranteed to succeed).
+ * The simplest response to this complication is disallow replace
+ * action on modules with vetoing hooks.
+ */
+ if ( has_payload_any_vetoing_hooks(data) || livepatch_applied_have_vetoing_hooks() )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: REPLACE action is not supported on livepatches with vetoing hooks!\n",
+ data->name);
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
data->rc = -EAGAIN;
rc = schedule_work(data, action->cmd, action->timeout);
}
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 99613af2db..cd20944cc4 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -21,6 +21,16 @@ typedef struct payload livepatch_payload_t;
typedef void livepatch_loadcall_t(void);
typedef void livepatch_unloadcall_t(void);
+typedef int livepatch_precall_t(livepatch_payload_t *arg);
+typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+
+struct livepatch_hooks {
+ struct {
+ livepatch_precall_t *const *pre;
+ livepatch_postcall_t *const *post;
+ } apply, revert;
+};
+
struct payload {
uint32_t state; /* One of the LIVEPATCH_STATE_*. */
int32_t rc; /* 0 or -XEN_EXX. */
@@ -47,6 +57,7 @@ struct payload {
struct livepatch_build_id xen_dep; /* ELFNOTE_DESC(.livepatch.xen_depends). */
livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
+ struct livepatch_hooks hooks; /* Pre and post hooks for apply and revert */
unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */
char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
@@ -76,6 +87,22 @@ struct payload {
livepatch_unloadcall_t *__weak \
const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
+#define LIVEPATCH_PREAPPLY_HOOK(_fn) \
+ livepatch_precall_t *__attribute__((weak, used)) \
+ const livepatch_preapply_data_##_fn __section(".livepatch.hooks.preapply") = _fn;
+
+#define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
+ livepatch_postcall_t *__attribute__((weak, used)) \
+ const livepatch_postapply_data_##_fn __section(".livepatch.hooks.postapply") = _fn;
+
+#define LIVEPATCH_PREREVERT_HOOK(_fn) \
+ livepatch_precall_t *__attribute__((weak, used)) \
+ const livepatch_prerevert_data_##_fn __section(".livepatch.hooks.prerevert") = _fn;
+
+#define LIVEPATCH_POSTREVERT_HOOK(_fn) \
+ livepatch_postcall_t *__attribute__((weak, used)) \
+ const livepatch_postrevert_data_##_fn __section(".livepatch.hooks.postrevert") = _fn;
+
#endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
/*
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 938aee17ec..a94bc48536 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -20,12 +20,16 @@ LIVEPATCH_BYE := xen_bye_world.livepatch
LIVEPATCH_REPLACE := xen_replace_world.livepatch
LIVEPATCH_NOP := xen_nop.livepatch
LIVEPATCH_NO_XEN_BUILDID := xen_no_xen_buildid.livepatch
+LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
+LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
LIVEPATCHES += $(LIVEPATCH_REPLACE)
LIVEPATCHES += $(LIVEPATCH_NOP)
LIVEPATCHES += $(LIVEPATCH_NO_XEN_BUILDID)
+LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
+LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -128,5 +132,18 @@ xen_no_xen_buildid.o: config.h
$(LIVEPATCH_NO_XEN_BUILDID): xen_nop.o note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NO_XEN_BUILDID) $^
+xen_prepost_hooks.o: config.h
+
+.PHONY: $(LIVEPATCH_PREPOST_HOOKS)
+$(LIVEPATCH_PREPOST_HOOKS): xen_prepost_hooks.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_PREPOST_HOOKS) $^
+
+xen_prepost_hooks_fail.o: config.h
+
+.PHONY: $(LIVEPATCH_PREPOST_HOOKS_FAIL)
+$(LIVEPATCH_PREPOST_HOOKS_FAIL): xen_prepost_hooks_fail.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_PREPOST_HOOKS_FAIL) $^
+
.PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
+ $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL)
diff --git a/xen/test/livepatch/xen_prepost_hooks.c b/xen/test/livepatch/xen_prepost_hooks.c
new file mode 100644
index 0000000000..889377d6eb
--- /dev/null
+++ b/xen/test/livepatch/xen_prepost_hooks.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int pre_apply_cnt;
+static unsigned int post_apply_cnt;
+static unsigned int pre_revert_cnt;
+static unsigned int post_revert_cnt;
+
+static unsigned int pre_revert_retry = 1;
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ pre_apply_cnt++;
+ printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ post_apply_cnt++;
+ printk(KERN_DEBUG "%s: applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ pre_revert_cnt++;
+ printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ /* First revert attempt always fails. Second attempt succeeds. */
+ return -(pre_revert_retry--);
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ post_revert_cnt++;
+ printk(KERN_DEBUG "%s: reverted: %s\n", __func__, func->name);
+ }
+
+ BUG_ON(pre_apply_cnt != 1 || post_apply_cnt != 1);
+ BUG_ON(pre_revert_cnt != 2 || post_revert_cnt != 1);
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_prepost_hooks_fail.c b/xen/test/livepatch/xen_prepost_hooks_fail.c
new file mode 100644
index 0000000000..c6feb5d32d
--- /dev/null
+++ b/xen/test/livepatch/xen_prepost_hooks_fail.c
@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+/* This hook always fail and should prevent from loading the livepatch. */
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ printk(KERN_DEBUG "%s: pre applying: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return -EINVAL;
+}
+
+static int unreachable_pre_hook(livepatch_payload_t *payload)
+{
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+ BUG();
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return -EINVAL;
+}
+
+static void unreachable_post_hook(livepatch_payload_t *payload)
+{
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+ BUG();
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(unreachable_post_hook);
+LIVEPATCH_PREREVERT_HOOK(unreachable_pre_hook);
+LIVEPATCH_POSTREVERT_HOOK(unreachable_post_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 05/12] livepatch: Add support for apply|revert action replacement hooks
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (3 preceding siblings ...)
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 06/12] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence Pawel Wieczorkiewicz
` (7 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
By default, in the quiescing zone, a livepatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.
To increase livepatching system's agility and provide more flexible
long-term livepatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.
Since the alternative functions have direct access to the livepatch
payload structure, they can better control context of the 'load' and
'unload' hooks execution as well as exact instructions replacement
workflows. They can be also easily extended to support extra features
in the future.
To simplify the alternative function generation move code responsible
for payload and livepatch region registration outside of the function.
That way it is guaranteed that the registration step occurs even for
newly supplied functions.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.com>
Reviewed-by: Norbert Manthey <nmanthey@amazon.com>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v3:
* s/hotpatch/livepatch/g
* remove extra newline
Changed since v1:
* added corresponding documentation
* added tests
docs/misc/livepatch.pandoc | 23 ++++++++
xen/common/livepatch.c | 65 ++++++++++++++++++----
xen/include/xen/livepatch_payload.h | 10 ++++
xen/test/livepatch/Makefile | 10 +++-
xen/test/livepatch/xen_action_hooks.c | 100 ++++++++++++++++++++++++++++++++++
5 files changed, 197 insertions(+), 11 deletions(-)
create mode 100644 xen/test/livepatch/xen_action_hooks.c
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index 5abddd0d27..e6c218ccf7 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -275,6 +275,7 @@ The payload contains at least three sections:
* `.livepatch.funcs` - which is an array of livepatch_func structures.
and/or any of:
* `.livepatch.hooks.{preapply,postapply,prerevert,postrevert}'
+ * `.livepatch.hooks.{apply,revert}`
- which are a pointer to a hook function pointer.
* `.livepatch.xen_depends` - which is an ELF Note that describes what Xen
@@ -356,6 +357,14 @@ met.
* `.livepatch.hooks.{prerevert,postrevert}`
- which are a pointer to a single hook function pointer.
+Finally, it optionally may also contain the address of apply or revert action
+hooks to be called instead of the default apply and revert payload actions
+(while all CPUs are kept in quiescing zone). These hooks do have access to
+payload structure.
+
+ * `.livepatch.hooks.{apply,revert}`
+ - which are a pointer to a single hook function pointer.
+
### Example of .livepatch.funcs
A simple example of what a payload file can be:
@@ -469,6 +478,20 @@ The type definition of the function are as follow:
typedef void livepatch_postcall_t(livepatch_payload_t *arg);
+#### .livepatch.hooks.apply and .livepatch.hooks.revert
+
+This section contains a pointer to a single function pointer to be executed
+instead of a default apply (or revert) action function. This is useful to
+replace or augment default behavior of the apply (or revert) action that
+requires all CPUs to be in the quiescing zone.
+This type of hooks do have access to payload structure.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+ typedef int livepatch_actioncall_t(livepatch_payload_t *arg);
+
### .livepatch.xen_depends, .livepatch.depends and .note.gnu.build-id
To support dependencies checking and safe loading (to load the
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index e8ba9317b7..38b1e3519d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -587,8 +587,11 @@ static int prepare_payload(struct payload *payload,
LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ".livepatch.hooks.unload");
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ".livepatch.hooks.preapply");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.action, ".livepatch.hooks.apply");
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.post, ".livepatch.hooks.postapply");
+
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.pre, ".livepatch.hooks.prerevert");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.action, ".livepatch.hooks.revert");
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ".livepatch.hooks.postrevert");
sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
@@ -1114,6 +1117,11 @@ static int apply_payload(struct payload *data)
arch_livepatch_revive();
+ return 0;
+}
+
+static inline void apply_payload_tail(struct payload *data)
+{
/*
* We need RCU variant (which has barriers) in case we crash here.
* The applied_list is iterated by the trap code.
@@ -1121,7 +1129,7 @@ static int apply_payload(struct payload *data)
list_add_tail_rcu(&data->applied_list, &applied_list);
register_virtual_region(&data->region);
- return 0;
+ data->state = LIVEPATCH_STATE_APPLIED;
}
static int revert_payload(struct payload *data)
@@ -1154,6 +1162,11 @@ static int revert_payload(struct payload *data)
ASSERT(!local_irq_is_enabled());
arch_livepatch_revive();
+ return 0;
+}
+
+static inline void revert_payload_tail(struct payload *data)
+{
/*
* We need RCU variant (which has barriers) in case we crash here.
@@ -1163,7 +1176,7 @@ static int revert_payload(struct payload *data)
unregister_virtual_region(&data->region);
data->reverted = true;
- return 0;
+ data->state = LIVEPATCH_STATE_CHECKED;
}
/*
@@ -1183,15 +1196,31 @@ static void livepatch_do_action(void)
switch ( livepatch_work.cmd )
{
case LIVEPATCH_ACTION_APPLY:
- rc = apply_payload(data);
+ if ( is_hook_enabled(data->hooks.apply.action) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook function\n", data->name);
+
+ rc = (*data->hooks.apply.action)(data);
+ }
+ else
+ rc = apply_payload(data);
+
if ( rc == 0 )
- data->state = LIVEPATCH_STATE_APPLIED;
+ apply_payload_tail(data);
break;
case LIVEPATCH_ACTION_REVERT:
- rc = revert_payload(data);
+ if ( is_hook_enabled(data->hooks.revert.action) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling revert action hook function\n", data->name);
+
+ rc = (*data->hooks.revert.action)(data);
+ }
+ else
+ rc = revert_payload(data);
+
if ( rc == 0 )
- data->state = LIVEPATCH_STATE_CHECKED;
+ revert_payload_tail(data);
break;
case LIVEPATCH_ACTION_REPLACE:
@@ -1202,9 +1231,17 @@ static void livepatch_do_action(void)
*/
list_for_each_entry_safe_reverse ( other, tmp, &applied_list, applied_list )
{
- other->rc = revert_payload(other);
+ if ( is_hook_enabled(other->hooks.revert.action) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling revert action hook function\n", other->name);
+
+ other->rc = (*other->hooks.revert.action)(other);
+ }
+ else
+ other->rc = revert_payload(other);
+
if ( other->rc == 0 )
- other->state = LIVEPATCH_STATE_CHECKED;
+ revert_payload_tail(other);
else
{
rc = -EINVAL;
@@ -1214,9 +1251,17 @@ static void livepatch_do_action(void)
if ( rc == 0 )
{
- rc = apply_payload(data);
+ if ( is_hook_enabled(data->hooks.apply.action) )
+ {
+ printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook function\n", data->name);
+
+ rc = (*data->hooks.apply.action)(data);
+ }
+ else
+ rc = apply_payload(data);
+
if ( rc == 0 )
- data->state = LIVEPATCH_STATE_APPLIED;
+ apply_payload_tail(data);
}
break;
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index cd20944cc4..ff16af0dd6 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -22,11 +22,13 @@ typedef void livepatch_loadcall_t(void);
typedef void livepatch_unloadcall_t(void);
typedef int livepatch_precall_t(livepatch_payload_t *arg);
+typedef int livepatch_actioncall_t(livepatch_payload_t *arg);
typedef void livepatch_postcall_t(livepatch_payload_t *arg);
struct livepatch_hooks {
struct {
livepatch_precall_t *const *pre;
+ livepatch_actioncall_t *const *action;
livepatch_postcall_t *const *post;
} apply, revert;
};
@@ -91,6 +93,10 @@ struct payload {
livepatch_precall_t *__attribute__((weak, used)) \
const livepatch_preapply_data_##_fn __section(".livepatch.hooks.preapply") = _fn;
+#define LIVEPATCH_APPLY_HOOK(_fn) \
+ livepatch_actioncall_t *__attribute__((weak, used)) \
+ const livepatch_apply_data_##_fn __section(".livepatch.hooks.apply") = _fn;
+
#define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
livepatch_postcall_t *__attribute__((weak, used)) \
const livepatch_postapply_data_##_fn __section(".livepatch.hooks.postapply") = _fn;
@@ -99,6 +105,10 @@ struct payload {
livepatch_precall_t *__attribute__((weak, used)) \
const livepatch_prerevert_data_##_fn __section(".livepatch.hooks.prerevert") = _fn;
+#define LIVEPATCH_REVERT_HOOK(_fn) \
+ livepatch_actioncall_t *__attribute__((weak, used)) \
+ const livepatch_revert_data_##_fn __section(".livepatch.hooks.revert") = _fn;
+
#define LIVEPATCH_POSTREVERT_HOOK(_fn) \
livepatch_postcall_t *__attribute__((weak, used)) \
const livepatch_postrevert_data_##_fn __section(".livepatch.hooks.postrevert") = _fn;
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index a94bc48536..116e52e774 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -22,6 +22,7 @@ LIVEPATCH_NOP := xen_nop.livepatch
LIVEPATCH_NO_XEN_BUILDID := xen_no_xen_buildid.livepatch
LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
+LIVEPATCH_ACTION_HOOKS := xen_action_hooks.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
@@ -30,6 +31,7 @@ LIVEPATCHES += $(LIVEPATCH_NOP)
LIVEPATCHES += $(LIVEPATCH_NO_XEN_BUILDID)
LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -144,6 +146,12 @@ xen_prepost_hooks_fail.o: config.h
$(LIVEPATCH_PREPOST_HOOKS_FAIL): xen_prepost_hooks_fail.o xen_hello_world_func.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_PREPOST_HOOKS_FAIL) $^
+xen_actions_hooks.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS)
+$(LIVEPATCH_ACTION_HOOKS): xen_action_hooks.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS) $^
+
.PHONY: livepatch
livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
- $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL)
+ $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS)
diff --git a/xen/test/livepatch/xen_action_hooks.c b/xen/test/livepatch/xen_action_hooks.c
new file mode 100644
index 0000000000..a947afc41f
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int apply_cnt;
+static unsigned int revert_cnt;
+
+static int apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ apply_cnt++;
+
+ printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static int revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ revert_cnt++;
+
+ printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ printk(KERN_DEBUG "%s: reverted: %s\n", __func__, func->name);
+ }
+
+ BUG_ON(apply_cnt != 1 || revert_cnt != 1);
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(apply_hook);
+LIVEPATCH_REVERT_HOOK(revert_hook);
+
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 06/12] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (4 preceding siblings ...)
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 05/12] livepatch: Add support for apply|revert action replacement hooks Pawel Wieczorkiewicz
@ 2019-09-28 15:12 ` Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker Pawel Wieczorkiewicz
` (6 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:12 UTC (permalink / raw)
To: xen-devel
Cc: Pawel Wieczorkiewicz, wipawel, Ross Lagerwall, mpohlack,
Konrad Rzeszutek Wilk
With default implementation the ELF_LIVEPATCH_FUNC section containing
all functions to be replaced or added must be part of the livepatch
payload, otherwise the payload is rejected (with -EINVAL).
However, with the extended hooks implementation, a livepatch may be
constructed of only hooks to perform certain actions without any code
to be added or replaced.
Therefore, do not always expect the functions section and allow it to
be missing, provided there is at least one section containing hooks
present. The functions section, when present in a payload, must be a
single, non-empty section.
Check also all extended hooks sections if they are a single, non-empty
sections each.
At least one of the functions or hooks section must be present in a
valid payload.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v3:
* fix indent
Changed since v1:
* always print XENLOG_ERR messages
* remove ASSERT from build_symbol_table()
* added corresponding documentation
* added tests
xen/common/livepatch.c | 147 +++++++++++++++++++--------
xen/include/xen/livepatch.h | 8 ++
xen/test/livepatch/Makefile | 9 +-
xen/test/livepatch/xen_action_hooks_nofunc.c | 86 ++++++++++++++++
4 files changed, 206 insertions(+), 44 deletions(-)
create mode 100644 xen/test/livepatch/xen_action_hooks_nofunc.c
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 38b1e3519d..c70fd5f600 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -467,8 +467,7 @@ static int xen_build_id_dep(const struct payload *payload)
static int check_special_sections(const struct livepatch_elf *elf)
{
unsigned int i;
- static const char *const names[] = { ELF_LIVEPATCH_FUNC,
- ELF_LIVEPATCH_DEPENDS,
+ static const char *const names[] = { ELF_LIVEPATCH_DEPENDS,
ELF_LIVEPATCH_XEN_DEPENDS,
ELF_BUILD_ID_NOTE};
DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
@@ -503,6 +502,64 @@ static int check_special_sections(const struct livepatch_elf *elf)
return 0;
}
+static int check_patching_sections(const struct livepatch_elf *elf)
+{
+ unsigned int i;
+ static const char *const names[] = { ELF_LIVEPATCH_FUNC,
+ ELF_LIVEPATCH_LOAD_HOOKS,
+ ELF_LIVEPATCH_UNLOAD_HOOKS,
+ ELF_LIVEPATCH_PREAPPLY_HOOK,
+ ELF_LIVEPATCH_APPLY_HOOK,
+ ELF_LIVEPATCH_POSTAPPLY_HOOK,
+ ELF_LIVEPATCH_PREREVERT_HOOK,
+ ELF_LIVEPATCH_REVERT_HOOK,
+ ELF_LIVEPATCH_POSTREVERT_HOOK};
+ DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
+
+ /*
+ * The patching sections are optional, but at least one
+ * must be present. Otherwise, there is nothing to do.
+ * All the existing sections must not be empty and must
+ * be present at most once.
+ */
+ for ( i = 0; i < ARRAY_SIZE(names); i++ )
+ {
+ const struct livepatch_elf_sec *sec;
+
+ sec = livepatch_elf_sec_by_name(elf, names[i]);
+ if ( !sec )
+ {
+ dprintk(XENLOG_DEBUG, LIVEPATCH "%s: %s is missing\n",
+ elf->name, names[i]);
+ continue; /* This section is optional */
+ }
+
+ if ( !sec->sec->sh_size )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: %s is empty\n",
+ elf->name, names[i]);
+ return -EINVAL;
+ }
+
+ if ( test_and_set_bit(i, found) )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n",
+ elf->name, names[i]);
+ return -EINVAL;
+ }
+ }
+
+ /* Checking if at least one section is present. */
+ if ( bitmap_empty(found, ARRAY_SIZE(names)) )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: Nothing to patch. Aborting...\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/*
* Lookup specified section and when exists assign its address to a specified hook.
* Perform section pointer and size validation: single hook sections must contain a
@@ -542,57 +599,59 @@ static int prepare_payload(struct payload *payload,
const Elf_Note *n;
sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
- ASSERT(sec);
- if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
- return -EINVAL;
-
- payload->funcs = sec->load_addr;
- payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
-
- for ( i = 0; i < payload->nfuncs; i++ )
+ if ( sec )
{
- int rc;
+ if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
+ return -EINVAL;
- f = &(payload->funcs[i]);
+ payload->funcs = sec->load_addr;
+ payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
- if ( f->version != LIVEPATCH_PAYLOAD_VERSION )
+ for ( i = 0; i < payload->nfuncs; i++ )
{
- printk(XENLOG_ERR LIVEPATCH "%s: Wrong version (%u). Expected %d\n",
- elf->name, f->version, LIVEPATCH_PAYLOAD_VERSION);
- return -EOPNOTSUPP;
- }
+ int rc;
- /* 'old_addr', 'new_addr', 'new_size' can all be zero. */
- if ( !f->old_size )
- {
- printk(XENLOG_ERR LIVEPATCH "%s: Address or size fields are zero\n",
- elf->name);
- return -EINVAL;
- }
+ f = &(payload->funcs[i]);
- rc = arch_livepatch_verify_func(f);
- if ( rc )
- return rc;
+ if ( f->version != LIVEPATCH_PAYLOAD_VERSION )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: Wrong version (%u). Expected %d\n",
+ elf->name, f->version, LIVEPATCH_PAYLOAD_VERSION);
+ return -EOPNOTSUPP;
+ }
- rc = resolve_old_address(f, elf);
- if ( rc )
- return rc;
+ /* 'old_addr', 'new_addr', 'new_size' can all be zero. */
+ if ( !f->old_size )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: Address or size fields are zero\n",
+ elf->name);
+ return -EINVAL;
+ }
- rc = livepatch_verify_distance(f);
- if ( rc )
- return rc;
+ rc = arch_livepatch_verify_func(f);
+ if ( rc )
+ return rc;
+
+ rc = resolve_old_address(f, elf);
+ if ( rc )
+ return rc;
+
+ rc = livepatch_verify_distance(f);
+ if ( rc )
+ return rc;
+ }
}
- LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->load_funcs, payload->n_load_funcs, ".livepatch.hooks.load");
- LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ".livepatch.hooks.unload");
+ LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->load_funcs, payload->n_load_funcs, ELF_LIVEPATCH_LOAD_HOOKS);
+ LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->unload_funcs, payload->n_unload_funcs, ELF_LIVEPATCH_UNLOAD_HOOKS);
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ".livepatch.hooks.preapply");
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.action, ".livepatch.hooks.apply");
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.post, ".livepatch.hooks.postapply");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.pre, ELF_LIVEPATCH_PREAPPLY_HOOK);
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.action, ELF_LIVEPATCH_APPLY_HOOK);
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.apply.post, ELF_LIVEPATCH_POSTAPPLY_HOOK);
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.pre, ".livepatch.hooks.prerevert");
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.action, ".livepatch.hooks.revert");
- LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ".livepatch.hooks.postrevert");
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.pre, ELF_LIVEPATCH_PREREVERT_HOOK);
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.action, ELF_LIVEPATCH_REVERT_HOOK);
+ LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ELF_LIVEPATCH_POSTREVERT_HOOK);
sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
if ( sec )
@@ -786,8 +845,6 @@ static int build_symbol_table(struct payload *payload,
struct livepatch_symbol *symtab;
char *strtab;
- ASSERT(payload->nfuncs);
-
/* Recall that section @0 is always NULL. */
for ( i = 1; i < elf->nsym; i++ )
{
@@ -904,6 +961,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
if ( rc )
goto out;
+ rc = check_patching_sections(&elf);
+ if ( rc )
+ goto out;
+
rc = prepare_payload(payload, &elf);
if ( rc )
goto out;
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index ed997aa4cc..2aec532ee2 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -33,6 +33,14 @@ struct xen_sysctl_livepatch_op;
#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
#define ELF_LIVEPATCH_XEN_DEPENDS ".livepatch.xen_depends"
#define ELF_BUILD_ID_NOTE ".note.gnu.build-id"
+#define ELF_LIVEPATCH_LOAD_HOOKS ".livepatch.hooks.load"
+#define ELF_LIVEPATCH_UNLOAD_HOOKS ".livepatch.hooks.unload"
+#define ELF_LIVEPATCH_PREAPPLY_HOOK ".livepatch.hooks.preapply"
+#define ELF_LIVEPATCH_APPLY_HOOK ".livepatch.hooks.apply"
+#define ELF_LIVEPATCH_POSTAPPLY_HOOK ".livepatch.hooks.postapply"
+#define ELF_LIVEPATCH_PREREVERT_HOOK ".livepatch.hooks.prerevert"
+#define ELF_LIVEPATCH_REVERT_HOOK ".livepatch.hooks.revert"
+#define ELF_LIVEPATCH_POSTREVERT_HOOK ".livepatch.hooks.postrevert"
/* Arbitrary limit for payload size and .bss section size. */
#define LIVEPATCH_MAX_SIZE MB(2)
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 116e52e774..bbc6bdaf64 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -23,6 +23,7 @@ LIVEPATCH_NO_XEN_BUILDID := xen_no_xen_buildid.livepatch
LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
LIVEPATCH_ACTION_HOOKS := xen_action_hooks.livepatch
+LIVEPATCH_ACTION_HOOKS_NOFUNC := xen_action_hooks_nofunc.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
@@ -32,6 +33,7 @@ LIVEPATCHES += $(LIVEPATCH_NO_XEN_BUILDID)
LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -152,6 +154,11 @@ xen_actions_hooks.o: config.h
$(LIVEPATCH_ACTION_HOOKS): xen_action_hooks.o xen_hello_world_func.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS) $^
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
+$(LIVEPATCH_ACTION_HOOKS_NOFUNC): xen_action_hooks_nofunc.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $^
+
.PHONY: livepatch
livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
- $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS)
+ $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS) \
+ $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
diff --git a/xen/test/livepatch/xen_action_hooks_nofunc.c b/xen/test/livepatch/xen_action_hooks_nofunc.c
new file mode 100644
index 0000000000..2b4e90436f
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_nofunc.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static unsigned int apply_cnt;
+static unsigned int revert_cnt;
+
+static int apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ apply_cnt++;
+ printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static int revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ revert_cnt++;
+ printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ printk(KERN_DEBUG "%s: reverted: %s\n", __func__, func->name);
+ }
+
+ BUG_ON(apply_cnt > 0 || revert_cnt > 0);
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(apply_hook);
+LIVEPATCH_REVERT_HOOK(revert_hook);
+
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (5 preceding siblings ...)
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 06/12] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-11-08 10:52 ` Ross Lagerwall
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 08/12] livepatch: Add support for inline asm livepatching expectations Pawel Wieczorkiewicz
` (5 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
Livepatch only tracks an entire payload applied/reverted state. But,
with an option to supply the apply_payload() and/or revert_payload()
functions as optional hooks, it becomes possible to intermix the
execution of the original apply_payload()/revert_payload() functions
with their dynamically supplied counterparts.
It is important then to track the current state of every function
being patched and prevent situations of unintentional double-apply
or unapplied revert.
To support that, it is necessary to extend public interface of the
livepatch. The struct livepatch_func gets additional field holding
the applied/reverted state marker.
To reflect the livepatch payload ABI change, bump the version flag
LIVEPATCH_PAYLOAD_VERSION up to 2.
[And also update the top of the design document]
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changed since v3:
* Use common_livepatch_{apply,revert} instead of modifying
arch_livepatch_{apply,revert}.
* s/bool_t/bool/g
Changed since v2:
* Documentation fixes
Changed since v1:
* support the feature for all arch (add handling for Arm)
* add common is_func_applied() and is_func_reverted() to be
used by all arch
* remove explicit enum values from enum livepatch_func_state
* added corresponding documentation
* added tests
docs/misc/livepatch.pandoc | 17 ++-
xen/common/livepatch.c | 39 ++++++-
xen/include/public/sysctl.h | 9 +-
xen/include/xen/livepatch.h | 28 +++++
xen/test/livepatch/Makefile | 27 ++++-
xen/test/livepatch/xen_action_hooks.c | 2 +
xen/test/livepatch/xen_action_hooks_marker.c | 112 +++++++++++++++++++
xen/test/livepatch/xen_action_hooks_noapply.c | 136 +++++++++++++++++++++++
xen/test/livepatch/xen_action_hooks_norevert.c | 143 +++++++++++++++++++++++++
9 files changed, 507 insertions(+), 6 deletions(-)
create mode 100644 xen/test/livepatch/xen_action_hooks_marker.c
create mode 100644 xen/test/livepatch/xen_action_hooks_noapply.c
create mode 100644 xen/test/livepatch/xen_action_hooks_norevert.c
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index e6c218ccf7..b9a37ca717 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -1,4 +1,4 @@
-# Xen Live Patching Design v1
+# Xen Live Patching Design v2
## Rationale
@@ -297,10 +297,14 @@ which describe the functions to be patched:
uint32_t old_size;
uint8_t version;
uint8_t opaque[31];
+ /* Added to livepatch payload version 2: */
+ uint8_t applied;
+ uint8_t _pad[7];
};
The size of the structure is 64 bytes on 64-bit hypervisors. It will be
52 on 32-bit hypervisors.
+The version 2 of the payload adds additional 8 bytes to the structure size.
* `name` is the symbol name of the old function. Only used if `old_addr` is
zero, otherwise will be used during dynamic linking (when hypervisor loads
@@ -324,9 +328,15 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
* If the value of `new_addr` is zero then `new_size` determines how many
instruction bytes to NOP (up to opaque size modulo smallest platform
instruction - 1 byte x86 and 4 bytes on ARM).
- * `version` is to be one.
+ * `version` indicates version of the generated payload.
* `opaque` **MUST** be zero.
+The version 2 of the payload adds the following fields to the structure:
+
+ * `applied` tracks function's applied/reverted state. It has a boolean type
+ either LIVEPATCH_FUNC_NOT_APPLIED or LIVEPATCH_FUNC_APPLIED.
+ * `_pad[7]` adds padding to align to 8 bytes.
+
The size of the `livepatch_func` array is determined from the ELF section
size.
@@ -378,6 +388,9 @@ A simple example of what a payload file can be:
uint32_t old_size;
uint8_t version;
uint8_t pad[31];
+ /* Added to livepatch payload version 2: */
+ uint8_t applied;
+ uint8_t _pad[7];
};
/* Our replacement function for xen_extra_version. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index c70fd5f600..d201479b42 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1174,7 +1174,7 @@ static int apply_payload(struct payload *data)
ASSERT(!local_irq_is_enabled());
for ( i = 0; i < data->nfuncs; i++ )
- arch_livepatch_apply(&data->funcs[i]);
+ common_livepatch_apply(&data->funcs[i]);
arch_livepatch_revive();
@@ -1208,7 +1208,7 @@ static int revert_payload(struct payload *data)
}
for ( i = 0; i < data->nfuncs; i++ )
- arch_livepatch_revert(&data->funcs[i]);
+ common_livepatch_revert(&data->funcs[i]);
/*
* Since we are running with IRQs disabled and the hooks may call common
@@ -1240,6 +1240,29 @@ static inline void revert_payload_tail(struct payload *data)
data->state = LIVEPATCH_STATE_CHECKED;
}
+/*
+ * Check if an action has applied the same state to all payload's functions consistently.
+ */
+static inline bool was_action_consistent(const struct payload *data, livepatch_func_state_t expected_state)
+{
+ int i;
+
+ for ( i = 0; i < data->nfuncs; i++ )
+ {
+ struct livepatch_func *f = &(data->funcs[i]);
+
+ if ( f->applied != expected_state )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n",
+ data->name, f->name ?: "noname");
+
+ return false;
+ }
+ }
+
+ return true;
+}
+
/*
* This function is executed having all other CPUs with no deep stack (we may
* have cpu_idle on it) and IRQs disabled.
@@ -1266,6 +1289,9 @@ static void livepatch_do_action(void)
else
rc = apply_payload(data);
+ if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
+ panic("livepatch: partially applied payload '%s'!\n", data->name);
+
if ( rc == 0 )
apply_payload_tail(data);
break;
@@ -1280,6 +1306,9 @@ static void livepatch_do_action(void)
else
rc = revert_payload(data);
+ if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
+ panic("livepatch: partially reverted payload '%s'!\n", data->name);
+
if ( rc == 0 )
revert_payload_tail(data);
break;
@@ -1301,6 +1330,9 @@ static void livepatch_do_action(void)
else
other->rc = revert_payload(other);
+ if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) )
+ panic("livepatch: partially reverted payload '%s'!\n", other->name);
+
if ( other->rc == 0 )
revert_payload_tail(other);
else
@@ -1321,6 +1353,9 @@ static void livepatch_do_action(void)
else
rc = apply_payload(data);
+ if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) )
+ panic("livepatch: partially applied payload '%s'!\n", data->name);
+
if ( rc == 0 )
apply_payload_tail(data);
}
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 1b2b165a6d..3bcb892ce1 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -818,7 +818,7 @@ struct xen_sysctl_cpu_featureset {
* If zero exit with success.
*/
-#define LIVEPATCH_PAYLOAD_VERSION 1
+#define LIVEPATCH_PAYLOAD_VERSION 2
/*
* .livepatch.funcs structure layout defined in the `Payload format`
* section in the Live Patch design document.
@@ -826,6 +826,11 @@ struct xen_sysctl_cpu_featureset {
* We guard this with __XEN__ as toolstacks SHOULD not use it.
*/
#ifdef __XEN__
+typedef enum livepatch_func_state {
+ LIVEPATCH_FUNC_NOT_APPLIED,
+ LIVEPATCH_FUNC_APPLIED
+} livepatch_func_state_t;
+
struct livepatch_func {
const char *name; /* Name of function to be patched. */
void *new_addr;
@@ -834,6 +839,8 @@ struct livepatch_func {
uint32_t old_size;
uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
uint8_t opaque[31];
+ uint8_t applied;
+ uint8_t _pad[7];
};
typedef struct livepatch_func livepatch_func_t;
#endif
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2aec532ee2..64d1fc2cae 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -122,6 +122,34 @@ void arch_livepatch_post_action(void);
void arch_livepatch_mask(void);
void arch_livepatch_unmask(void);
+
+static inline void common_livepatch_apply(struct livepatch_func *func)
+{
+ /* If the action has been already executed on this function, do nothing. */
+ if ( func->applied == LIVEPATCH_FUNC_APPLIED )
+ {
+ printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n",
+ __func__, func->name);
+ return;
+ }
+
+ arch_livepatch_apply(func);
+ func->applied = LIVEPATCH_FUNC_APPLIED;
+}
+
+static inline void common_livepatch_revert(struct livepatch_func *func)
+{
+ /* If the apply action hasn't been executed on this function, do nothing. */
+ if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED )
+ {
+ printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n",
+ __func__, func->name);
+ return;
+ }
+
+ arch_livepatch_revert(func);
+ func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
+}
#else
/*
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index bbc6bdaf64..23113d3418 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -24,6 +24,9 @@ LIVEPATCH_PREPOST_HOOKS := xen_prepost_hooks.livepatch
LIVEPATCH_PREPOST_HOOKS_FAIL := xen_prepost_hooks_fail.livepatch
LIVEPATCH_ACTION_HOOKS := xen_action_hooks.livepatch
LIVEPATCH_ACTION_HOOKS_NOFUNC := xen_action_hooks_nofunc.livepatch
+LIVEPATCH_ACTION_HOOKS_MARKER:= xen_action_hooks_marker.livepatch
+LIVEPATCH_ACTION_HOOKS_NOAPPLY:= xen_action_hooks_noapply.livepatch
+LIVEPATCH_ACTION_HOOKS_NOREVERT:= xen_action_hooks_norevert.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
@@ -34,6 +37,9 @@ LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS)
LIVEPATCHES += $(LIVEPATCH_PREPOST_HOOKS_FAIL)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_MARKER)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
+LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -158,7 +164,26 @@ $(LIVEPATCH_ACTION_HOOKS): xen_action_hooks.o xen_hello_world_func.o note.o xen_
$(LIVEPATCH_ACTION_HOOKS_NOFUNC): xen_action_hooks_nofunc.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $^
+xen_actions_hooks_marker.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_MARKER)
+$(LIVEPATCH_ACTION_HOOKS_MARKER): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_MARKER) $^
+
+xen_actions_hooks_noapply.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
+$(LIVEPATCH_ACTION_HOOKS_NOAPPLY): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) $^
+
+xen_actions_hooks_norevert.o: config.h
+
+.PHONY: $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
+$(LIVEPATCH_ACTION_HOOKS_NOREVERT): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $^
+
.PHONY: livepatch
livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
$(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS) \
- $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
+ $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $(LIVEPATCH_ACTION_HOOKS_MARKER) $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) \
+ $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
diff --git a/xen/test/livepatch/xen_action_hooks.c b/xen/test/livepatch/xen_action_hooks.c
index a947afc41f..39b5313027 100644
--- a/xen/test/livepatch/xen_action_hooks.c
+++ b/xen/test/livepatch/xen_action_hooks.c
@@ -28,6 +28,7 @@ static int apply_hook(livepatch_payload_t *payload)
{
struct livepatch_func *func = &payload->funcs[i];
+ func->applied = LIVEPATCH_FUNC_APPLIED;
apply_cnt++;
printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
@@ -48,6 +49,7 @@ static int revert_hook(livepatch_payload_t *payload)
{
struct livepatch_func *func = &payload->funcs[i];
+ func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
revert_cnt++;
printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
diff --git a/xen/test/livepatch/xen_action_hooks_marker.c b/xen/test/livepatch/xen_action_hooks_marker.c
new file mode 100644
index 0000000000..4f807a577f
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_marker.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_action_hooks_noapply.c b/xen/test/livepatch/xen_action_hooks_noapply.c
new file mode 100644
index 0000000000..4c55c156a6
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_noapply.c
@@ -0,0 +1,136 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int apply_cnt;
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static int apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ apply_cnt++;
+ printk(KERN_DEBUG "%s: applying: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return -EINVAL; /* Mark action as inconsistent */
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(apply_cnt != 1);
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(apply_hook);
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_action_hooks_norevert.c b/xen/test/livepatch/xen_action_hooks_norevert.c
new file mode 100644
index 0000000000..ef77e72071
--- /dev/null
+++ b/xen/test/livepatch/xen_action_hooks_norevert.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+static unsigned int revert_cnt;
+
+static int pre_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static void post_apply_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: post applied: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+static int pre_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+ printk(KERN_DEBUG "%s: pre reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return 0;
+}
+
+static int revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ revert_cnt++;
+ printk(KERN_DEBUG "%s: reverting: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+
+ return -EINVAL; /* Mark action as inconsistent */
+}
+
+static void post_revert_hook(livepatch_payload_t *payload)
+{
+ int i;
+
+ printk(KERN_DEBUG "%s: Hook starting.\n", __func__);
+
+ for (i = 0; i < payload->nfuncs; i++)
+ {
+ struct livepatch_func *func = &payload->funcs[i];
+
+ BUG_ON(revert_cnt != 1);
+ BUG_ON(func->applied != LIVEPATCH_FUNC_APPLIED);
+
+ /* Outside of quiesce zone: MAY TRIGGER HOST CRASH/UNDEFINED BEHAVIOR */
+ arch_livepatch_quiesce();
+ common_livepatch_revert(payload);
+ arch_livepatch_revive();
+ BUG_ON(func->applied == LIVEPATCH_FUNC_APPLIED);
+
+ printk(KERN_DEBUG "%s: post reverted: %s\n", __func__, func->name);
+ }
+
+ printk(KERN_DEBUG "%s: Hook done.\n", __func__);
+}
+
+LIVEPATCH_APPLY_HOOK(revert_hook);
+
+LIVEPATCH_PREAPPLY_HOOK(pre_apply_hook);
+LIVEPATCH_POSTAPPLY_HOOK(post_apply_hook);
+LIVEPATCH_PREREVERT_HOOK(pre_revert_hook);
+LIVEPATCH_POSTREVERT_HOOK(post_revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 08/12] livepatch: Add support for inline asm livepatching expectations
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (6 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 09/12] livepatch: Add support for modules .modinfo section metadata Pawel Wieczorkiewicz
` (4 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
This is the initial implementation of the expectations enhancement
to improve inline asm livepatching.
Expectations are designed as optional feature, since the main use of
them is planned for inline asm livepatching. The flag enabled allows
to control the expectation state.
Each expectation has data and len fields that describe the data
that is expected to be found at a given patching (old_addr) location.
The len must not exceed the data array size. The data array size
follows the size of the opaque array, since the opaque array holds
the original data and therefore must match what is specified in the
expectation (if enabled).
The payload structure is modified as each expectation structure is
part of the livepatch_func structure and hence extends the payload.
Each expectation is checked prior to the apply action (i.e. as late
as possible to check against the most current state of the code).
For the replace action a new payload's expectations are checked AFTER
all applied payloads are successfully reverted, but BEFORE new payload
is applied. That breaks the replace action's atomicity and in case of
an expectation check failure would leave a system with all payloads
reverted. That is obviously insecure. Use it with caution and act
upon replace errors!
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v3:
* Fix for test's automatic expectation generation
Changed since v2:
* Add rsv in the field. Update the size of the structure. (Konrad)
* Fix expectation test to work also on Arm
Changed since v1:
* added corresponding documentation
* added tests
.gitignore | 1 +
docs/misc/livepatch.pandoc | 35 +++++++++++++-
xen/common/livepatch.c | 74 ++++++++++++++++++++++++++++++
xen/include/public/sysctl.h | 16 ++++++-
xen/test/livepatch/Makefile | 27 ++++++++++-
xen/test/livepatch/xen_expectations.c | 41 +++++++++++++++++
xen/test/livepatch/xen_expectations_fail.c | 42 +++++++++++++++++
7 files changed, 231 insertions(+), 5 deletions(-)
create mode 100644 xen/test/livepatch/xen_expectations.c
create mode 100644 xen/test/livepatch/xen_expectations_fail.c
diff --git a/.gitignore b/.gitignore
index 5b80e2bbd5..cb770cc8c7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -308,6 +308,7 @@ xen/include/xen/acm_policy.h
xen/include/xen/compile.h
xen/include/xen/lib/x86/cpuid-autogen.h
xen/test/livepatch/config.h
+xen/test/livepatch/expect_config.h
xen/test/livepatch/*.livepatch
xen/tools/kconfig/.tmp_gtkcheck
xen/tools/kconfig/.tmp_qtcheck
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index b9a37ca717..3713c0530f 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -300,10 +300,11 @@ which describe the functions to be patched:
/* Added to livepatch payload version 2: */
uint8_t applied;
uint8_t _pad[7];
+ livepatch_expectation_t expect;
};
-The size of the structure is 64 bytes on 64-bit hypervisors. It will be
-52 on 32-bit hypervisors.
+The size of the structure is 104 bytes on 64-bit hypervisors. It will be
+92 on 32-bit hypervisors.
The version 2 of the payload adds additional 8 bytes to the structure size.
* `name` is the symbol name of the old function. Only used if `old_addr` is
@@ -336,6 +337,28 @@ The version 2 of the payload adds the following fields to the structure:
* `applied` tracks function's applied/reverted state. It has a boolean type
either LIVEPATCH_FUNC_NOT_APPLIED or LIVEPATCH_FUNC_APPLIED.
* `_pad[7]` adds padding to align to 8 bytes.
+ * `expect` is an optional structure containing expected to-be-replaced data
+ (mostly for inline asm patching). The `expect` structure format is:
+
+ struct livepatch_expectation {
+ uint8_t enabled : 1;
+ uint8_t len : 5;
+ uint8_t rsv: 2;
+ uint8_t data[LIVEPATCH_OPAQUE_SIZE]; /* Same size as opaque[] buffer of
+ struct livepatch_func. This is the
+ max number of bytes to be patched */
+ };
+ typedef struct livepatch_expectation livepatch_expectation_t;
+
+ * `enabled` allows to enable the expectation check for given function.
+ Default state is disabled.
+ * `len` specifies the number of valid bytes in `data` array. 5 bits is
+ enough to specify values up to 32 (of bytes), which is above the array
+ size.
+ * `rsv` reserved bitfields. **MUST** be zero.
+ * `data` contains expected bytes of content to be replaced. Same size as
+ `opaque` buffer of `struct livepatch_func` (max number of bytes to be
+ patched).
The size of the `livepatch_func` array is determined from the ELF section
size.
@@ -391,6 +414,7 @@ A simple example of what a payload file can be:
/* Added to livepatch payload version 2: */
uint8_t applied;
uint8_t _pad[7];
+ livepatch_expectation_t expect;
};
/* Our replacement function for xen_extra_version. */
@@ -408,6 +432,13 @@ A simple example of what a payload file can be:
.old_addr = (void *)0xffff82d08013963c, /* Extracted from xen-syms. */
.new_size = 13, /* To be be computed by scripts. */
.old_size = 13, /* -----------""--------------- */
+ /* Added to livepatch payload version 2: */
+ .expect = { /* All fields to be filled manually */
+ .enabled = 1,
+ .len = 5,
+ .rsv = 0,
+ .data = { 0x48, 0x8d, 0x05, 0x33, 0x1C }
+ },
} __attribute__((__section__(".livepatch.funcs")));
Code must be compiled with `-fPIC`.
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d201479b42..a1d551eba3 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -560,6 +560,61 @@ static int check_patching_sections(const struct livepatch_elf *elf)
return 0;
}
+static inline int livepatch_verify_expectation_fn(const struct livepatch_func *func)
+{
+ const livepatch_expectation_t *exp = &func->expect;
+
+ /* Ignore disabled expectations. */
+ if ( !exp->enabled )
+ return 0;
+
+ /* There is nothing to expect */
+ if ( !func->old_addr )
+ return -EFAULT;
+
+ if ( exp->len > sizeof(exp->data))
+ return -EOVERFLOW;
+
+ if ( exp->rsv )
+ return -EINVAL;
+
+ /* Incorrect expectation */
+ if ( func->old_size < exp->len )
+ return -ERANGE;
+
+ if ( memcmp(func->old_addr, exp->data, exp->len) )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: expectation failed: expected:%*phN, actual:%*phN\n",
+ func->name, exp->len, exp->data, exp->len, func->old_addr);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static inline int livepatch_check_expectations(const struct payload *payload)
+{
+ int i, rc;
+
+ printk(XENLOG_INFO LIVEPATCH "%s: Verifying enabled expectations for all functions\n",
+ payload->name);
+
+ for ( i = 0; i < payload->nfuncs; i++ )
+ {
+ const struct livepatch_func *func = &(payload->funcs[i]);
+
+ rc = livepatch_verify_expectation_fn(func);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: expectations of %s failed (rc=%d), aborting!\n",
+ payload->name, func->name ?: "unknown", rc);
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
/*
* Lookup specified section and when exists assign its address to a specified hook.
* Perform section pointer and size validation: single hook sections must contain a
@@ -1344,6 +1399,20 @@ static void livepatch_do_action(void)
if ( rc == 0 )
{
+ /*
+ * Make sure all expectation requirements are met.
+ * Beware all the payloads are reverted at this point.
+ * If expectations are not met the system is left in a
+ * completely UNPATCHED state!
+ */
+ rc = livepatch_check_expectations(data);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: SYSTEM MIGHT BE INSECURE: "
+ "Replace action has been aborted after reverting ALL payloads!\n", data->name);
+ break;
+ }
+
if ( is_hook_enabled(data->hooks.apply.action) )
{
printk(XENLOG_INFO LIVEPATCH "%s: Calling apply action hook function\n", data->name);
@@ -1797,6 +1866,11 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action)
break;
}
+ /* Make sure all expectation requirements are met. */
+ rc = livepatch_check_expectations(data);
+ if ( rc )
+ break;
+
if ( is_hook_enabled(data->hooks.apply.pre) )
{
printk(XENLOG_INFO LIVEPATCH "%s: Calling pre-apply hook function\n", data->name);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3bcb892ce1..bcdfc1fafe 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -826,6 +826,19 @@ struct xen_sysctl_cpu_featureset {
* We guard this with __XEN__ as toolstacks SHOULD not use it.
*/
#ifdef __XEN__
+#define LIVEPATCH_OPAQUE_SIZE 31
+
+struct livepatch_expectation {
+ uint8_t enabled : 1;
+ uint8_t len : 5; /* Length of data up to LIVEPATCH_OPAQUE_SIZE
+ (5 bits is enough for now) */
+ uint8_t rsv : 2; /* Reserved. Zero value */
+ uint8_t data[LIVEPATCH_OPAQUE_SIZE]; /* Same size as opaque[] buffer of
+ struct livepatch_func. This is the
+ max number of bytes to be patched */
+};
+typedef struct livepatch_expectation livepatch_expectation_t;
+
typedef enum livepatch_func_state {
LIVEPATCH_FUNC_NOT_APPLIED,
LIVEPATCH_FUNC_APPLIED
@@ -838,9 +851,10 @@ struct livepatch_func {
uint32_t new_size;
uint32_t old_size;
uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
- uint8_t opaque[31];
+ uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
uint8_t applied;
uint8_t _pad[7];
+ livepatch_expectation_t expect;
};
typedef struct livepatch_func livepatch_func_t;
#endif
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 23113d3418..cebd3eb49c 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -27,6 +27,8 @@ LIVEPATCH_ACTION_HOOKS_NOFUNC := xen_action_hooks_nofunc.livepatch
LIVEPATCH_ACTION_HOOKS_MARKER:= xen_action_hooks_marker.livepatch
LIVEPATCH_ACTION_HOOKS_NOAPPLY:= xen_action_hooks_noapply.livepatch
LIVEPATCH_ACTION_HOOKS_NOREVERT:= xen_action_hooks_norevert.livepatch
+LIVEPATCH_EXPECTATIONS:= xen_expectations.livepatch
+LIVEPATCH_EXPECTATIONS_FAIL:= xen_expectations_fail.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
@@ -40,6 +42,8 @@ LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_MARKER)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
+LIVEPATCHES += $(LIVEPATCH_EXPECTATIONS)
+LIVEPATCHES += $(LIVEPATCH_EXPECTATIONS_FAIL)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -54,7 +58,7 @@ uninstall:
.PHONY: clean
clean::
- rm -f *.o .*.o.d *.livepatch config.h
+ rm -f *.o .*.o.d *.livepatch config.h expect_config.h
#
# To compute these values we need the binary files: xen-syms
@@ -182,8 +186,27 @@ xen_actions_hooks_norevert.o: config.h
$(LIVEPATCH_ACTION_HOOKS_NOREVERT): xen_action_hooks_marker.o xen_hello_world_func.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $^
+EXPECT_BYTES_COUNT := 8
+CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=1 $(1) | sed -n -e '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(EXPECT_BYTES_COUNT) | awk '{$$0=$$2; printf "%s", substr($$0,length-1)}' | sed 's/.\{2\}/0x&,/g' | sed 's/^/{/;s/,$$/}/g')
+.PHONY: expect_config.h
+expect_config.h: EXPECT_BYTES=$(call CODE_GET_EXPECT,$(BASEDIR)/xen-syms,xen_extra_version)
+expect_config.h: xen_expectations.o
+ (set -e; \
+ echo "#define EXPECT_BYTES $(EXPECT_BYTES)"; \
+ echo "#define EXPECT_BYTES_COUNT $(EXPECT_BYTES_COUNT)") > $@
+
+xen_expectations.o: expect_config.h
+
+.PHONY: $(LIVEPATCH_EXPECTATIONS)
+$(LIVEPATCH_EXPECTATIONS): xen_expectations.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_EXPECTATIONS) $^
+
+.PHONY: $(LIVEPATCH_EXPECTATIONS_FAIL)
+$(LIVEPATCH_EXPECTATIONS_FAIL): xen_expectations_fail.o xen_hello_world_func.o note.o xen_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_EXPECTATIONS_FAIL) $^
+
.PHONY: livepatch
livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
$(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL) $(LIVEPATCH_ACTION_HOOKS) \
$(LIVEPATCH_ACTION_HOOKS_NOFUNC) $(LIVEPATCH_ACTION_HOOKS_MARKER) $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) \
- $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
+ $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $(LIVEPATCH_EXPECTATIONS) $(LIVEPATCH_EXPECTATIONS_FAIL)
diff --git a/xen/test/livepatch/xen_expectations.c b/xen/test/livepatch/xen_expectations.c
new file mode 100644
index 0000000000..c8175a458b
--- /dev/null
+++ b/xen/test/livepatch/xen_expectations.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include "expect_config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char livepatch_exceptions_str[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_exceptions = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = livepatch_exceptions_str,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = EXPECT_BYTES_COUNT,
+ .old_size = EXPECT_BYTES_COUNT,
+ .expect = {
+ .enabled = 1,
+ .len = EXPECT_BYTES_COUNT,
+ .data = EXPECT_BYTES
+ },
+
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/test/livepatch/xen_expectations_fail.c b/xen/test/livepatch/xen_expectations_fail.c
new file mode 100644
index 0000000000..36a110286f
--- /dev/null
+++ b/xen/test/livepatch/xen_expectations_fail.c
@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ *
+ */
+
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static const char livepatch_exceptions_str[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+#define EXPECT_BYTES_COUNT 6
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_exceptions = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = livepatch_exceptions_str,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = EXPECT_BYTES_COUNT,
+ .old_size = EXPECT_BYTES_COUNT,
+ .expect = {
+ .enabled = 1,
+ .len = EXPECT_BYTES_COUNT,
+ .data = { 0xDE, 0xAD, 0xC0, 0xDE, 0xBA, 0xBE }
+ },
+
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 09/12] livepatch: Add support for modules .modinfo section metadata
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (7 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 08/12] livepatch: Add support for inline asm livepatching expectations Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation Pawel Wieczorkiewicz
` (3 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: Pawel Wieczorkiewicz, wipawel, Ross Lagerwall, mpohlack,
Konrad Rzeszutek Wilk
Having detailed livepatch metadata helps to properly identify module's
origin and version. It also allows to keep track of the history of
livepatch loads in the system (at least within dmesg buffer size
limits).
The livepatch metadata are embedded in a form of .modinfo section.
Each such section contains data of the following format:
key=value\0key=value\0...key=value\0
The .modinfo section may be generated and appended to the resulting
livepatch ELF file optionally as an extra step of a higher level
livepatch build system.
The metadata section pointer and the section length is stored in the
livepatch payload structure and is used to display the content upon
livepatch apply operation.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed since v2:
* Added .modinfo tests (Konrad)
xen/common/livepatch.c | 34 ++++++++++++++++++++++++++++++++++
xen/include/xen/livepatch_payload.h | 6 ++++++
xen/test/livepatch/Makefile | 10 +++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index a1d551eba3..46685f3aa8 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -853,6 +853,23 @@ static int prepare_payload(struct payload *payload,
#endif
}
+ sec = livepatch_elf_sec_by_name(elf, ".modinfo");
+ if ( sec )
+ {
+ if ( !section_ok(elf, sec, sizeof(*payload->metadata.data)) )
+ return -EINVAL;
+
+ payload->metadata.data = sec->load_addr;
+ payload->metadata.len = sec->sec->sh_size;
+
+ /* The metadata is required to consists of null terminated strings. */
+ if ( payload->metadata.data[payload->metadata.len - 1] != '\0' )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: Incorrect metadata format detected\n", payload->name);
+ return -EINVAL;
+ }
+ }
+
return 0;
}
@@ -1201,6 +1218,19 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
* for XEN_SYSCTL_LIVEPATCH_ACTION operation (see livepatch_action).
*/
+static inline void livepatch_display_metadata(const struct livepatch_metadata *metadata)
+{
+ const char *str;
+
+ if ( metadata && metadata->data && metadata->len > 0 )
+ {
+ printk(XENLOG_INFO LIVEPATCH "module metadata:\n");
+ for ( str = metadata->data; str < (metadata->data + metadata->len); str += (strlen(str) + 1) )
+ printk(XENLOG_INFO LIVEPATCH " %s\n", str);
+ }
+
+}
+
static int apply_payload(struct payload *data)
{
unsigned int i;
@@ -1233,6 +1263,8 @@ static int apply_payload(struct payload *data)
arch_livepatch_revive();
+ livepatch_display_metadata(&data->metadata);
+
return 0;
}
@@ -2008,6 +2040,8 @@ static void livepatch_printall(unsigned char key)
data->name, state2str(data->state), data->state, data->text_addr,
data->rw_addr, data->ro_addr, data->pages);
+ livepatch_display_metadata(&data->metadata);
+
for ( i = 0; i < data->nfuncs; i++ )
{
struct livepatch_func *f = &(data->funcs[i]);
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index ff16af0dd6..9f5f064205 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -33,6 +33,11 @@ struct livepatch_hooks {
} apply, revert;
};
+struct livepatch_metadata {
+ const char *data; /* Ptr to .modinfo section with ASCII data. */
+ uint32_t len; /* Length of the metadata section. */
+};
+
struct payload {
uint32_t state; /* One of the LIVEPATCH_STATE_*. */
int32_t rc; /* 0 or -XEN_EXX. */
@@ -63,6 +68,7 @@ struct payload {
unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */
char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
+ struct livepatch_metadata metadata; /* Module meta data record */
};
/*
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index cebd3eb49c..ebb343c314 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -79,9 +79,17 @@ config.h: xen_hello_world_func.o
xen_hello_world.o: config.h
.PHONY: $(LIVEPATCH)
-$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o xen_note.o
+$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o xen_note.o modinfo.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
+.PHONY: modinfo.o
+modinfo.o:
+ (set -e; \
+ printf "LIVEPATCH_RULEZ\0") > $@.bin
+ $(OBJCOPY) $(OBJCOPY_MAGIC) \
+ --rename-section=.data=.modinfo,alloc,load,readonly,data,contents -S $@.bin $@
+ #rm -f $@.bin
+
#
# This target is only accessible if CONFIG_LIVEPATCH is defined, which
# depends on $(build_id_linker) being available. Hence we do not
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (8 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 09/12] livepatch: Add support for modules .modinfo section metadata Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-09-30 8:50 ` Jan Beulich
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 11/12] livepatch: Add metadata runtime retrieval mechanism Pawel Wieczorkiewicz
` (2 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
The payloads' name strings can be of arbitrary size (typically small
with an upper bound of XEN_LIVEPATCH_NAME_SIZE).
Current implementation of the list operation interface allows to copy
names in the XEN_LIVEPATCH_NAME_SIZE chunks regardless of its actual
size and enforces space allocation requirements on userland tools.
To unify and simplify the interface, handle the name strings of
arbitrary size by copying them in adhering chunks to the userland.
In order to let the userland allocate enough space for the incoming
data add an auxiliary interface xc_livepatch_list_get_sizes() that
provides the current number of payload entries and the total size of
all name strings. This is achieved by extending the sysctl list
interface with an extra fields: name_total_size.
The xc_livepatch_list_get_sizes() issues the livepatch sysctl list
operation with the nr field set to 0. In this mode the operation
returns the number of payload entries and calculates the total sizes
for all payloads' names.
When the sysctl operation is issued with a non-zero nr field (for
instance with a value obtained earlier with the prior call to the
xc_livepatch_list_get_sizes()) the new field name_total_size provides
the total size of actually copied data.
Extend the libxc to handle the name back-to-back data transfers.
The xen-livepatch tool is modified to start the list operation with a
call to the xc_livepatch_list_get_sizes() to obtain the actual number
of payloads as well as the necessary space for names.
The tool now always requests the actual number of entries and leaves
the preemption handling to the libxc routine. The libxc still returns
'done' and 'left' parameters with the same semantic allowing the tool
to detect anomalies and react to them. At the moment it is expected
that the tool receives the exact number of entries as requested.
The xen-livepatch tool has been also modified to handle the name
back-to-back transfers correctly.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Changed since v3:
* use uint32_t instead of uint64_t and off_t for name_total_size
and related variables
Changed since v1:
* added corresponding documentation
docs/misc/livepatch.pandoc | 24 +++++----
tools/libxc/include/xenctrl.h | 49 +++++++++++++------
tools/libxc/xc_misc.c | 100 ++++++++++++++++++++++++++++---------
tools/misc/xen-livepatch.c | 111 ++++++++++++++++++++++--------------------
xen/common/livepatch.c | 31 +++++++++---
xen/include/public/sysctl.h | 15 +++---
6 files changed, 218 insertions(+), 112 deletions(-)
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index 3713c0530f..eebd04e8df 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -717,17 +717,20 @@ The caller provides:
* `idx` Index iterator. The index into the hypervisor's payload count. It is
recommended that on first invocation zero be used so that `nr` (which the
hypervisor will update with the remaining payload count) be provided.
- Also the hypervisor will provide `version` with the most current value.
+ Also the hypervisor will provide `version` with the most current value and
+ calculated total size for all payloads' names.
* `nr` The max number of entries to populate. Can be zero which will result
in the hypercall being a probing one and return the number of payloads
(and update the `version`).
* `pad` - *MUST* be zero.
* `status` Virtual address of where to write `struct xen_livepatch_status`
structures. Caller *MUST* allocate up to `nr` of them.
- * `name` - Virtual address of where to write the unique name of the payload.
- Caller *MUST* allocate up to `nr` of them. Each *MUST* be of
- **XEN_LIVEPATCH_NAME_SIZE** size. Note that **XEN_LIVEPATCH_NAME_SIZE** includes
- the NUL terminator.
+ * `name` - Virtual address of where to write the unique name of the payloads.
+ Caller *MUST* allocate enough space to be able to store all received data
+ (i.e. total allocated space *MUST* match the `name_total_size` value
+ provided by the hypervisor). Individual payload name cannot be longer than
+ **XEN_LIVEPATCH_NAME_SIZE** bytes. Note that **XEN_LIVEPATCH_NAME_SIZE**
+ includes the NUL terminator.
* `len` - Virtual address of where to write the length of each unique name
of the payload. Caller *MUST* allocate up to `nr` of them. Each *MUST* be
of sizeof(uint32_t) (4 bytes).
@@ -736,7 +739,8 @@ If the hypercall returns an positive number, it is the number (upto `nr`
provided to the hypercall) of the payloads returned, along with `nr` updated
with the number of remaining payloads, `version` updated (it may be the same
across hypercalls - if it varies the data is stale and further calls could
-fail). The `status`, `name`, and `len` are updated at their designed index
+fail) and the `name_total_size` containing total size of transferred data for
+the array. The `status`, `name`, and `len` are updated at their designed index
value (`idx`) with the returned value of data.
If the hypercall returns -XEN_E2BIG the `nr` is too big and should be
@@ -775,11 +779,13 @@ The structure is as follow:
amount of payloads and version.
OUT: How many payloads left. */
uint32_t pad; /* IN: Must be zero. */
+ uint32_t name_total_size; /* OUT: Total size of all transfer names */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
space allocate for nr of them. */
- XEN_GUEST_HANDLE_64(char) id; /* OUT: Array of names. Each member
- MUST XEN_LIVEPATCH_NAME_SIZE in size.
- Must have nr of them. */
+ XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
+ may have an arbitrary length up to
+ XEN_LIVEPATCH_NAME_SIZE bytes. Must have
+ nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
Must have nr of them. */
};
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2fc62422f5..937cb47c8b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2551,7 +2551,25 @@ int xc_livepatch_get(xc_interface *xch,
xen_livepatch_status_t *status);
/*
- * The heart of this function is to get an array of xen_livepatch_status_t.
+ * Get a number of available payloads and get actual total size of
+ * the payloads' name array.
+ *
+ * This functions is typically executed first before the xc_livepatch_list()
+ * to obtain the sizes and correctly allocate all necessary data resources.
+ *
+ * The return value is zero if the hypercall completed successfully.
+ *
+ * If there was an error performing the sysctl operation, the return value
+ * will contain the hypercall error code value.
+ */
+int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
+ uint32_t *name_total_size);
+
+/*
+ * The heart of this function is to get an array of the following objects:
+ * - xen_livepatch_status_t: states and return codes of payloads
+ * - name: names of payloads
+ * - len: lengths of corresponding payloads' names
*
* However it is complex because it has to deal with the hypervisor
* returning some of the requested data or data being stale
@@ -2562,21 +2580,20 @@ int xc_livepatch_get(xc_interface *xch,
* 'left' are also updated with the number of entries filled out
* and respectively the number of entries left to get from hypervisor.
*
- * It is expected that the caller of this function will take the
- * 'left' and use the value for 'start'. This way we have an
- * cursor in the array. Note that the 'info','name', and 'len' will
- * be updated at the subsequent calls.
+ * It is expected that the caller of this function will first issue the
+ * xc_livepatch_list_get_sizes() in order to obtain total sizes of names
+ * as well as the current number of payload entries.
+ * The total sizes are required and supplied via the 'name_total_size'
+ * parameter.
*
- * The 'max' is to be provided by the caller with the maximum
- * number of entries that 'info', 'name', and 'len' arrays can
- * be filled up with.
- *
- * Each entry in the 'name' array is expected to be of XEN_LIVEPATCH_NAME_SIZE
- * length.
+ * The 'max' is to be provided by the caller with the maximum number of
+ * entries that 'info', 'name', 'len' arrays can be filled up with.
*
* Each entry in the 'info' array is expected to be of xen_livepatch_status_t
* structure size.
*
+ * Each entry in the 'name' array may have an arbitrary size.
+ *
* Each entry in the 'len' array is expected to be of uint32_t size.
*
* The return value is zero if the hypercall completed successfully.
@@ -2588,10 +2605,12 @@ int xc_livepatch_get(xc_interface *xch,
* will contain the number of entries that had been succesfully
* retrieved (if any).
*/
-int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
- xen_livepatch_status_t *info, char *name,
- uint32_t *len, unsigned int *done,
- unsigned int *left);
+int xc_livepatch_list(xc_interface *xch, const unsigned int max,
+ const unsigned int start,
+ struct xen_livepatch_status *info,
+ char *name, uint32_t *len,
+ const uint32_t name_total_size,
+ unsigned int *done, unsigned int *left);
/*
* The operations are asynchronous and the hypervisor may take a while
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index a8e9e7d1e2..20e0e6673b 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -662,7 +662,48 @@ int xc_livepatch_get(xc_interface *xch,
}
/*
- * The heart of this function is to get an array of xen_livepatch_status_t.
+ * Get a number of available payloads and get actual total size of
+ * the payloads' name array.
+ *
+ * This functions is typically executed first before the xc_livepatch_list()
+ * to obtain the sizes and correctly allocate all necessary data resources.
+ *
+ * The return value is zero if the hypercall completed successfully.
+ *
+ * If there was an error performing the sysctl operation, the return value
+ * will contain the hypercall error code value.
+ */
+int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
+ uint32_t *name_total_size)
+{
+ DECLARE_SYSCTL;
+ int rc;
+
+ if ( !nr || !name_total_size )
+ {
+ errno = EINVAL;
+ return -1;
+ }
+
+ memset(&sysctl, 0, sizeof(sysctl));
+ sysctl.cmd = XEN_SYSCTL_livepatch_op;
+ sysctl.u.livepatch.cmd = XEN_SYSCTL_LIVEPATCH_LIST;
+
+ rc = do_sysctl(xch, &sysctl);
+ if ( rc )
+ return rc;
+
+ *nr = sysctl.u.livepatch.u.list.nr;
+ *name_total_size = sysctl.u.livepatch.u.list.name_total_size;
+
+ return 0;
+}
+
+/*
+ * The heart of this function is to get an array of the following objects:
+ * - xen_livepatch_status_t: states and return codes of payloads
+ * - name: names of payloads
+ * - len: lengths of corresponding payloads' names
*
* However it is complex because it has to deal with the hypervisor
* returning some of the requested data or data being stale
@@ -673,21 +714,20 @@ int xc_livepatch_get(xc_interface *xch,
* 'left' are also updated with the number of entries filled out
* and respectively the number of entries left to get from hypervisor.
*
- * It is expected that the caller of this function will take the
- * 'left' and use the value for 'start'. This way we have an
- * cursor in the array. Note that the 'info','name', and 'len' will
- * be updated at the subsequent calls.
+ * It is expected that the caller of this function will first issue the
+ * xc_livepatch_list_get_sizes() in order to obtain total sizes of names
+ * as well as the current number of payload entries.
+ * The total sizes are required and supplied via the 'name_total_size'
+ * parameter.
*
- * The 'max' is to be provided by the caller with the maximum
- * number of entries that 'info', 'name', and 'len' arrays can
- * be filled up with.
- *
- * Each entry in the 'name' array is expected to be of XEN_LIVEPATCH_NAME_SIZE
- * length.
+ * The 'max' is to be provided by the caller with the maximum number of
+ * entries that 'info', 'name', 'len' arrays can be filled up with.
*
* Each entry in the 'info' array is expected to be of xen_livepatch_status_t
* structure size.
*
+ * Each entry in the 'name' array may have an arbitrary size.
+ *
* Each entry in the 'len' array is expected to be of uint32_t size.
*
* The return value is zero if the hypercall completed successfully.
@@ -699,11 +739,12 @@ int xc_livepatch_get(xc_interface *xch,
* will contain the number of entries that had been succesfully
* retrieved (if any).
*/
-int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
+int xc_livepatch_list(xc_interface *xch, const unsigned int max,
+ const unsigned int start,
struct xen_livepatch_status *info,
char *name, uint32_t *len,
- unsigned int *done,
- unsigned int *left)
+ const uint32_t name_total_size,
+ unsigned int *done, unsigned int *left)
{
int rc;
DECLARE_SYSCTL;
@@ -714,27 +755,33 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
uint32_t max_batch_sz, nr;
uint32_t version = 0, retries = 0;
uint32_t adjust = 0;
- ssize_t sz;
+ uint32_t name_off = 0;
+ uint32_t name_sz;
- if ( !max || !info || !name || !len )
+ if ( !max || !info || !name || !len || !done || !left )
{
errno = EINVAL;
return -1;
}
+ if ( name_total_size == 0 )
+ {
+ errno = ENOENT;
+ return -1;
+ }
+
+ memset(&sysctl, 0, sizeof(sysctl));
sysctl.cmd = XEN_SYSCTL_livepatch_op;
sysctl.u.livepatch.cmd = XEN_SYSCTL_LIVEPATCH_LIST;
- sysctl.u.livepatch.pad = 0;
- sysctl.u.livepatch.u.list.version = 0;
sysctl.u.livepatch.u.list.idx = start;
- sysctl.u.livepatch.u.list.pad = 0;
max_batch_sz = max;
- /* Convience value. */
- sz = sizeof(*name) * XEN_LIVEPATCH_NAME_SIZE;
+ name_sz = name_total_size;
*done = 0;
*left = 0;
do {
+ uint32_t _name_sz;
+
/*
* The first time we go in this loop our 'max' may be bigger
* than what the hypervisor is comfortable with - hence the first
@@ -754,11 +801,11 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
sysctl.u.livepatch.u.list.nr = nr;
/* Fix the size (may vary between hypercalls). */
HYPERCALL_BOUNCE_SET_SIZE(info, nr * sizeof(*info));
- HYPERCALL_BOUNCE_SET_SIZE(name, nr * nr);
+ HYPERCALL_BOUNCE_SET_SIZE(name, name_sz);
HYPERCALL_BOUNCE_SET_SIZE(len, nr * sizeof(*len));
/* Move the pointer to proper offset into 'info'. */
(HYPERCALL_BUFFER(info))->ubuf = info + *done;
- (HYPERCALL_BUFFER(name))->ubuf = name + (sz * *done);
+ (HYPERCALL_BUFFER(name))->ubuf = name + name_off;
(HYPERCALL_BUFFER(len))->ubuf = len + *done;
/* Allocate memory. */
rc = xc_hypercall_bounce_pre(xch, info);
@@ -827,14 +874,19 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
break;
}
*left = sysctl.u.livepatch.u.list.nr; /* Total remaining count. */
+ _name_sz = sysctl.u.livepatch.u.list.name_total_size; /* Total received name size. */
/* Copy only up 'rc' of data' - we could add 'min(rc,nr) if desired. */
HYPERCALL_BOUNCE_SET_SIZE(info, (rc * sizeof(*info)));
- HYPERCALL_BOUNCE_SET_SIZE(name, (rc * sz));
+ HYPERCALL_BOUNCE_SET_SIZE(name, _name_sz);
HYPERCALL_BOUNCE_SET_SIZE(len, (rc * sizeof(*len)));
/* Bounce the data and free the bounce buffer. */
xc_hypercall_bounce_post(xch, info);
xc_hypercall_bounce_post(xch, name);
xc_hypercall_bounce_post(xch, len);
+
+ name_sz -= _name_sz;
+ name_off += _name_sz;
+
/* And update how many elements of info we have copied into. */
*done += rc;
/* Update idx. */
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 0eee94fd91..77c04d23c1 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -64,14 +64,13 @@ static const char *state2str(unsigned int state)
return names[state];
}
-/* This value was choosen adhoc. It could be 42 too. */
-#define MAX_LEN 11
static int list_func(int argc, char *argv[])
{
- unsigned int idx, done, left, i;
+ unsigned int nr, done, left, i;
xen_livepatch_status_t *info = NULL;
char *name = NULL;
uint32_t *len = NULL;
+ uint32_t name_total_size, name_off;
int rc = ENOMEM;
if ( argc )
@@ -79,65 +78,73 @@ static int list_func(int argc, char *argv[])
show_help();
return -1;
}
- idx = left = 0;
- info = malloc(sizeof(*info) * MAX_LEN);
- if ( !info )
- return rc;
- name = malloc(sizeof(*name) * XEN_LIVEPATCH_NAME_SIZE * MAX_LEN);
- if ( !name )
+ done = left = 0;
+
+ rc = xc_livepatch_list_get_sizes(xch, &nr, &name_total_size);
+ if ( rc )
{
- free(info);
+ rc = errno;
+ fprintf(stderr, "Failed to get list sizes.\n"
+ "Error %d: %s\n",
+ rc, strerror(rc));
return rc;
}
- len = malloc(sizeof(*len) * MAX_LEN);
- if ( !len ) {
- free(name);
- free(info);
+
+ if ( nr == 0 )
+ {
+ fprintf(stdout, "Nothing to list\n");
+ return 0;
+ }
+
+ info = malloc(nr * sizeof(*info));
+ if ( !info )
return rc;
+
+ name = malloc(name_total_size * sizeof(*name));
+ if ( !name )
+ goto error_name;
+
+ len = malloc(nr * sizeof(*len));
+ if ( !len )
+ goto error_len;
+
+ memset(info, 'A', nr * sizeof(*info));
+ memset(name, 'B', name_total_size * sizeof(*name));
+ memset(len, 'C', nr * sizeof(*len));
+ name_off = 0;
+
+ rc = xc_livepatch_list(xch, nr, 0, info, name, len, name_total_size, &done, &left);
+ if ( rc || done != nr || left > 0)
+ {
+ rc = errno;
+ fprintf(stderr, "Failed to list %d/%d.\n"
+ "Error %d: %s\n",
+ left, nr, rc, strerror(rc));
+ goto error;
}
- do {
- done = 0;
- /* The memset is done to catch errors. */
- memset(info, 'A', sizeof(*info) * MAX_LEN);
- memset(name, 'B', sizeof(*name) * MAX_LEN * XEN_LIVEPATCH_NAME_SIZE);
- memset(len, 'C', sizeof(*len) * MAX_LEN);
- rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
- if ( rc )
- {
- rc = errno;
- fprintf(stderr, "Failed to list %d/%d.\n"
- "Error %d: %s\n",
- idx, left, rc, strerror(rc));
- break;
- }
- if ( !idx )
- fprintf(stdout," ID | status\n"
- "----------------------------------------+------------\n");
+ fprintf(stdout," ID | status\n"
+ "----------------------------------------+------------\n");
- for ( i = 0; i < done; i++ )
- {
- unsigned int j;
- uint32_t sz;
- char *str;
-
- sz = len[i];
- str = name + (i * XEN_LIVEPATCH_NAME_SIZE);
- for ( j = sz; j < XEN_LIVEPATCH_NAME_SIZE; j++ )
- str[j] = '\0';
-
- printf("%-40s| %s", str, state2str(info[i].state));
- if ( info[i].rc )
- printf(" (%d, %s)\n", -info[i].rc, strerror(-info[i].rc));
- else
- puts("");
- }
- idx += done;
- } while ( left );
+ for ( i = 0; i < done; i++ )
+ {
+ char *name_str = name + name_off;
+
+ printf("%-40.*s| %s", len[i], name_str, state2str(info[i].state));
+ if ( info[i].rc )
+ printf(" (%d, %s)\n", -info[i].rc, strerror(-info[i].rc));
+ else
+ puts("");
+
+ name_off += len[i];
+ }
+error:
+ free(len);
+error_len:
free(name);
+error_name:
free(info);
- free(len);
return rc;
}
#undef MAX_LEN
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 46685f3aa8..3c191eb889 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1164,7 +1164,6 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
if ( list->nr &&
(!guest_handle_okay(list->status, list->nr) ||
- !guest_handle_okay(list->name, XEN_LIVEPATCH_NAME_SIZE * list->nr) ||
!guest_handle_okay(list->len, list->nr)) )
return -EINVAL;
@@ -1175,23 +1174,35 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
return -EINVAL;
}
+ list->name_total_size = 0;
if ( list->nr )
{
+ uint64_t name_offset = 0;
+
list_for_each_entry( data, &payload_list, list )
{
- uint32_t len;
+ uint32_t name_len;
if ( list->idx > i++ )
continue;
status.state = data->state;
status.rc = data->rc;
- len = strlen(data->name) + 1;
+
+ name_len = strlen(data->name) + 1;
+ list->name_total_size += name_len;
+
+ if ( !guest_handle_subrange_okay(list->name, name_offset,
+ name_offset + name_len - 1) )
+ {
+ rc = -EINVAL;
+ break;
+ }
/* N.B. 'idx' != 'i'. */
- if ( __copy_to_guest_offset(list->name, idx * XEN_LIVEPATCH_NAME_SIZE,
- data->name, len) ||
- __copy_to_guest_offset(list->len, idx, &len, 1) ||
+ if ( __copy_to_guest_offset(list->name, name_offset,
+ data->name, name_len) ||
+ __copy_to_guest_offset(list->len, idx, &name_len, 1) ||
__copy_to_guest_offset(list->status, idx, &status, 1) )
{
rc = -EFAULT;
@@ -1199,11 +1210,19 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
}
idx++;
+ name_offset += name_len;
if ( (idx >= list->nr) || hypercall_preempt_check() )
break;
}
}
+ else
+ {
+ list_for_each_entry( data, &payload_list, list )
+ {
+ list->name_total_size += strlen(data->name) + 1;
+ }
+ }
list->nr = payload_cnt - i; /* Remaining amount. */
list->version = payload_version;
spin_unlock(&payload_lock);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index bcdfc1fafe..08807c92a5 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -925,10 +925,11 @@ struct xen_sysctl_livepatch_get {
*
* If the hypercall returns an positive number, it is the number (up to `nr`)
* of the payloads returned, along with `nr` updated with the number of remaining
- * payloads, `version` updated (it may be the same across hypercalls. If it
- * varies the data is stale and further calls could fail). The `status`,
- * `name`, and `len`' are updated at their designed index value (`idx`) with
- * the returned value of data.
+ * payloads, `version` updated (it may be the same across hypercalls. If it varies
+ * the data is stale and further calls could fail) and the name_total_size
+ * containing total size of transferred data for the array.
+ * The `status`, `name`, `len` are updated at their designed index value (`idx`)
+ * with the returned value of data.
*
* If the hypercall returns E2BIG the `nr` is too big and should be
* lowered. The upper limit of `nr` is left to the implemention.
@@ -951,11 +952,13 @@ struct xen_sysctl_livepatch_list {
amount of payloads and version.
OUT: How many payloads left. */
uint32_t pad; /* IN: Must be zero. */
+ uint32_t name_total_size; /* OUT: Total size of all transfer names */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
space allocate for nr of them. */
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
- MUST XEN_LIVEPATCH_NAME_SIZE in size.
- Must have nr of them. */
+ may have an arbitrary length up to
+ XEN_LIVEPATCH_NAME_SIZE bytes. Must have
+ nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
Must have nr of them. */
};
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 11/12] livepatch: Add metadata runtime retrieval mechanism
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (9 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations Pawel Wieczorkiewicz
2019-10-18 14:25 ` [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Konrad Rzeszutek Wilk
12 siblings, 0 replies; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ross Lagerwall, Ian Jackson,
mpohlack, Tim Deegan, Pawel Wieczorkiewicz, Julien Grall,
Jan Beulich
Extend the livepatch list operation to fetch also payloads' metadata.
This is achieved by extending the sysctl list interface with 2 extra
guest handles:
* metadata - an array of arbitrary size strings
* metadata_len - an array of metadata strings' lengths (uin32_t each)
Payloads' metadata is a string of arbitrary size and does not have an
upper bound limit. It may also vary in size between payloads.
In order to let the userland allocate enough space for the incoming
data add a metadata total size field to the list sysctl operation and
fill it with total size of all payloads' metadata.
Extend the libxc to handle the metadata back-to-back data transfers
as well as metadata length array data transfers.
The xen-livepatch userland tool is extended to always display the
metadata for each received module. The metadata is received with the
following format: key=value\0key=value\0...key=value\0. The format is
modified to the following one: key=value;key=value;...key=value.
The new format allows to easily parse the metadata for a given module
by a machine.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Changed since v3:
* use uint32_t instead of uint64_t and off_t for
metadata_total_size and related variables
Changed since v1:
* added corresponding documentation
* make metadata optional (do not display it when given payload
does not have it)
docs/misc/livepatch.pandoc | 31 ++++++++++++++++-----
tools/libxc/include/xenctrl.h | 22 +++++++++++----
tools/libxc/xc_misc.c | 64 +++++++++++++++++++++++++++++++++++--------
tools/misc/xen-livepatch.c | 41 +++++++++++++++++++++------
xen/common/livepatch.c | 22 +++++++++++----
xen/include/public/sysctl.h | 19 +++++++++----
6 files changed, 156 insertions(+), 43 deletions(-)
diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
index eebd04e8df..ec73a9314e 100644
--- a/docs/misc/livepatch.pandoc
+++ b/docs/misc/livepatch.pandoc
@@ -707,8 +707,8 @@ The structure is as follow:
### XEN_SYSCTL_LIVEPATCH_LIST (2)
-Retrieve an array of abbreviated status and names of payloads that are loaded in the
-hypervisor.
+Retrieve an array of abbreviated status, names and metadata of payloads that are
+loaded in the hypervisor.
The caller provides:
@@ -717,8 +717,9 @@ The caller provides:
* `idx` Index iterator. The index into the hypervisor's payload count. It is
recommended that on first invocation zero be used so that `nr` (which the
hypervisor will update with the remaining payload count) be provided.
- Also the hypervisor will provide `version` with the most current value and
- calculated total size for all payloads' names.
+ Also the hypervisor will provide `version` with the most current value,
+ calculated total size of all payloads' names and calculated total size of
+ all payload's metadata.
* `nr` The max number of entries to populate. Can be zero which will result
in the hypercall being a probing one and return the number of payloads
(and update the `version`).
@@ -734,14 +735,23 @@ The caller provides:
* `len` - Virtual address of where to write the length of each unique name
of the payload. Caller *MUST* allocate up to `nr` of them. Each *MUST* be
of sizeof(uint32_t) (4 bytes).
+ * `metadata` - Virtual address of where to write the metadata of the payloads.
+ Caller *MUST* allocate enough space to be able to store all received data
+ (i.e. total allocated space *MUST* match the `metadata_total_size` value
+ provided by the hypervisor). Individual payload metadata string can be of
+ arbitrary length. The metadata string format is: key=value\0...key=value\0.
+ * `metadata_len` - Virtual address of where to write the length of each metadata
+ string of the payload. Caller *MUST* allocate up to `nr` of them. Each *MUST*
+ be of sizeof(uint32_t) (4 bytes).
If the hypercall returns an positive number, it is the number (upto `nr`
provided to the hypercall) of the payloads returned, along with `nr` updated
with the number of remaining payloads, `version` updated (it may be the same
across hypercalls - if it varies the data is stale and further calls could
-fail) and the `name_total_size` containing total size of transferred data for
-the array. The `status`, `name`, and `len` are updated at their designed index
-value (`idx`) with the returned value of data.
+fail), `name_total_size` and `metadata_total_size` containing total sizes of
+transferred data for both the arrays.
+The `status`, `name`, `len`, `metadata` and `metadata_len` are updated at their
+designed index value (`idx`) with the returned value of data.
If the hypercall returns -XEN_E2BIG the `nr` is too big and should be
lowered.
@@ -780,6 +790,7 @@ The structure is as follow:
OUT: How many payloads left. */
uint32_t pad; /* IN: Must be zero. */
uint32_t name_total_size; /* OUT: Total size of all transfer names */
+ uint32_t metadata_total_size; /* OUT: Total size of all transfer metadata */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
space allocate for nr of them. */
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
@@ -788,6 +799,12 @@ The structure is as follow:
nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
Must have nr of them. */
+ XEN_GUEST_HANDLE_64(char) metadata; /* OUT: Array of metadata strings. Each
+ member may have an arbitrary length.
+ Must have nr of them. */
+ XEN_GUEST_HANDLE_64(uint32) metadata_len; /* OUT: Array of lengths of metadata's.
+ Must have nr of them. */
+
};
### XEN_SYSCTL_LIVEPATCH_ACTION (3)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 937cb47c8b..66a5c72fd6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2552,7 +2552,7 @@ int xc_livepatch_get(xc_interface *xch,
/*
* Get a number of available payloads and get actual total size of
- * the payloads' name array.
+ * the payloads' name and metadata arrays.
*
* This functions is typically executed first before the xc_livepatch_list()
* to obtain the sizes and correctly allocate all necessary data resources.
@@ -2563,13 +2563,16 @@ int xc_livepatch_get(xc_interface *xch,
* will contain the hypercall error code value.
*/
int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
- uint32_t *name_total_size);
+ uint32_t *name_total_size,
+ uint32_t *metadata_total_size);
/*
* The heart of this function is to get an array of the following objects:
* - xen_livepatch_status_t: states and return codes of payloads
* - name: names of payloads
* - len: lengths of corresponding payloads' names
+ * - metadata: payloads' metadata
+ * - metadata_len: lengths of corresponding payloads' metadata
*
* However it is complex because it has to deal with the hypervisor
* returning some of the requested data or data being stale
@@ -2582,12 +2585,13 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
*
* It is expected that the caller of this function will first issue the
* xc_livepatch_list_get_sizes() in order to obtain total sizes of names
- * as well as the current number of payload entries.
- * The total sizes are required and supplied via the 'name_total_size'
- * parameter.
+ * and all metadata as well as the current number of payload entries.
+ * The total sizes are required and supplied via the 'name_total_size' and
+ * 'metadata_total_size' parameters.
*
* The 'max' is to be provided by the caller with the maximum number of
- * entries that 'info', 'name', 'len' arrays can be filled up with.
+ * entries that 'info', 'name', 'len', 'metadata' and 'metadata_len' arrays
+ * can be filled up with.
*
* Each entry in the 'info' array is expected to be of xen_livepatch_status_t
* structure size.
@@ -2596,6 +2600,10 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
*
* Each entry in the 'len' array is expected to be of uint32_t size.
*
+ * Each entry in the 'metadata' array may have an arbitrary size.
+ *
+ * Each entry in the 'metadata_len' array is expected to be of uint32_t size.
+ *
* The return value is zero if the hypercall completed successfully.
* Note that the return value is _not_ the amount of entries filled
* out - that is saved in 'done'.
@@ -2610,6 +2618,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
struct xen_livepatch_status *info,
char *name, uint32_t *len,
const uint32_t name_total_size,
+ char *metadata, uint32_t *metadata_len,
+ const uint32_t metadata_total_size,
unsigned int *done, unsigned int *left);
/*
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 20e0e6673b..80b097b878 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -663,7 +663,7 @@ int xc_livepatch_get(xc_interface *xch,
/*
* Get a number of available payloads and get actual total size of
- * the payloads' name array.
+ * the payloads' name and metadata arrays.
*
* This functions is typically executed first before the xc_livepatch_list()
* to obtain the sizes and correctly allocate all necessary data resources.
@@ -674,12 +674,13 @@ int xc_livepatch_get(xc_interface *xch,
* will contain the hypercall error code value.
*/
int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
- uint32_t *name_total_size)
+ uint32_t *name_total_size,
+ uint32_t *metadata_total_size)
{
DECLARE_SYSCTL;
int rc;
- if ( !nr || !name_total_size )
+ if ( !nr || !name_total_size || !metadata_total_size )
{
errno = EINVAL;
return -1;
@@ -695,6 +696,7 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
*nr = sysctl.u.livepatch.u.list.nr;
*name_total_size = sysctl.u.livepatch.u.list.name_total_size;
+ *metadata_total_size = sysctl.u.livepatch.u.list.metadata_total_size;
return 0;
}
@@ -704,6 +706,8 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
* - xen_livepatch_status_t: states and return codes of payloads
* - name: names of payloads
* - len: lengths of corresponding payloads' names
+ * - metadata: payloads' metadata
+ * - metadata_len: lengths of corresponding payloads' metadata
*
* However it is complex because it has to deal with the hypervisor
* returning some of the requested data or data being stale
@@ -716,12 +720,13 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
*
* It is expected that the caller of this function will first issue the
* xc_livepatch_list_get_sizes() in order to obtain total sizes of names
- * as well as the current number of payload entries.
- * The total sizes are required and supplied via the 'name_total_size'
- * parameter.
+ * and all metadata as well as the current number of payload entries.
+ * The total sizes are required and supplied via the 'name_total_size' and
+ * 'metadata_total_size' parameters.
*
* The 'max' is to be provided by the caller with the maximum number of
- * entries that 'info', 'name', 'len' arrays can be filled up with.
+ * entries that 'info', 'name', 'len', 'metadata' and 'metadata_len' arrays
+ * can be filled up with.
*
* Each entry in the 'info' array is expected to be of xen_livepatch_status_t
* structure size.
@@ -730,6 +735,10 @@ int xc_livepatch_list_get_sizes(xc_interface *xch, unsigned int *nr,
*
* Each entry in the 'len' array is expected to be of uint32_t size.
*
+ * Each entry in the 'metadata' array may have an arbitrary size.
+ *
+ * Each entry in the 'metadata_len' array is expected to be of uint32_t size.
+ *
* The return value is zero if the hypercall completed successfully.
* Note that the return value is _not_ the amount of entries filled
* out - that is saved in 'done'.
@@ -744,6 +753,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
struct xen_livepatch_status *info,
char *name, uint32_t *len,
const uint32_t name_total_size,
+ char *metadata, uint32_t *metadata_len,
+ const uint32_t metadata_total_size,
unsigned int *done, unsigned int *left)
{
int rc;
@@ -752,13 +763,16 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
DECLARE_HYPERCALL_BOUNCE(info, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(name, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(len, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(metadata, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(metadata_len, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
uint32_t max_batch_sz, nr;
uint32_t version = 0, retries = 0;
uint32_t adjust = 0;
- uint32_t name_off = 0;
- uint32_t name_sz;
+ uint32_t name_off = 0, metadata_off = 0;
+ uint32_t name_sz, metadata_sz;
- if ( !max || !info || !name || !len || !done || !left )
+ if ( !max || !info || !name || !len ||
+ !metadata || !metadata_len || !done || !left )
{
errno = EINVAL;
return -1;
@@ -777,10 +791,11 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
max_batch_sz = max;
name_sz = name_total_size;
+ metadata_sz = metadata_total_size;
*done = 0;
*left = 0;
do {
- uint32_t _name_sz;
+ uint32_t _name_sz, _metadata_sz;
/*
* The first time we go in this loop our 'max' may be bigger
@@ -803,10 +818,14 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
HYPERCALL_BOUNCE_SET_SIZE(info, nr * sizeof(*info));
HYPERCALL_BOUNCE_SET_SIZE(name, name_sz);
HYPERCALL_BOUNCE_SET_SIZE(len, nr * sizeof(*len));
+ HYPERCALL_BOUNCE_SET_SIZE(metadata, metadata_sz);
+ HYPERCALL_BOUNCE_SET_SIZE(metadata_len, nr * sizeof(*metadata_len));
/* Move the pointer to proper offset into 'info'. */
(HYPERCALL_BUFFER(info))->ubuf = info + *done;
(HYPERCALL_BUFFER(name))->ubuf = name + name_off;
(HYPERCALL_BUFFER(len))->ubuf = len + *done;
+ (HYPERCALL_BUFFER(metadata))->ubuf = metadata + metadata_off;
+ (HYPERCALL_BUFFER(metadata_len))->ubuf = metadata_len + *done;
/* Allocate memory. */
rc = xc_hypercall_bounce_pre(xch, info);
if ( rc )
@@ -820,9 +839,19 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
if ( rc )
break;
+ rc = xc_hypercall_bounce_pre(xch, metadata);
+ if ( rc )
+ break;
+
+ rc = xc_hypercall_bounce_pre(xch, metadata_len);
+ if ( rc )
+ break;
+
set_xen_guest_handle(sysctl.u.livepatch.u.list.status, info);
set_xen_guest_handle(sysctl.u.livepatch.u.list.name, name);
set_xen_guest_handle(sysctl.u.livepatch.u.list.len, len);
+ set_xen_guest_handle(sysctl.u.livepatch.u.list.metadata, metadata);
+ set_xen_guest_handle(sysctl.u.livepatch.u.list.metadata_len, metadata_len);
rc = do_sysctl(xch, &sysctl);
/*
@@ -839,6 +868,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
xc_hypercall_bounce_post(xch, info);
xc_hypercall_bounce_post(xch, name);
xc_hypercall_bounce_post(xch, len);
+ xc_hypercall_bounce_post(xch, metadata);
+ xc_hypercall_bounce_post(xch, metadata_len);
continue;
}
else if ( rc < 0 ) /* For all other errors we bail out. */
@@ -863,6 +894,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
xc_hypercall_bounce_post(xch, info);
xc_hypercall_bounce_post(xch, name);
xc_hypercall_bounce_post(xch, len);
+ xc_hypercall_bounce_post(xch, metadata);
+ xc_hypercall_bounce_post(xch, metadata_len);
continue;
}
@@ -875,17 +908,24 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
}
*left = sysctl.u.livepatch.u.list.nr; /* Total remaining count. */
_name_sz = sysctl.u.livepatch.u.list.name_total_size; /* Total received name size. */
+ _metadata_sz = sysctl.u.livepatch.u.list.metadata_total_size; /* Total received metadata size. */
/* Copy only up 'rc' of data' - we could add 'min(rc,nr) if desired. */
HYPERCALL_BOUNCE_SET_SIZE(info, (rc * sizeof(*info)));
HYPERCALL_BOUNCE_SET_SIZE(name, _name_sz);
HYPERCALL_BOUNCE_SET_SIZE(len, (rc * sizeof(*len)));
+ HYPERCALL_BOUNCE_SET_SIZE(metadata, _metadata_sz);
+ HYPERCALL_BOUNCE_SET_SIZE(metadata_len, (rc * sizeof(*metadata_len)));
/* Bounce the data and free the bounce buffer. */
xc_hypercall_bounce_post(xch, info);
xc_hypercall_bounce_post(xch, name);
xc_hypercall_bounce_post(xch, len);
+ xc_hypercall_bounce_post(xch, metadata);
+ xc_hypercall_bounce_post(xch, metadata_len);
name_sz -= _name_sz;
name_off += _name_sz;
+ metadata_sz -= _metadata_sz;
+ metadata_off += _metadata_sz;
/* And update how many elements of info we have copied into. */
*done += rc;
@@ -898,6 +938,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
xc_hypercall_bounce_post(xch, len);
xc_hypercall_bounce_post(xch, name);
xc_hypercall_bounce_post(xch, info);
+ xc_hypercall_bounce_post(xch, metadata);
+ xc_hypercall_bounce_post(xch, metadata_len);
}
return rc > 0 ? 0 : rc;
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 77c04d23c1..75773d299a 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -69,8 +69,10 @@ static int list_func(int argc, char *argv[])
unsigned int nr, done, left, i;
xen_livepatch_status_t *info = NULL;
char *name = NULL;
+ char *metadata = NULL;
uint32_t *len = NULL;
- uint32_t name_total_size, name_off;
+ uint32_t *metadata_len = NULL;
+ uint32_t name_total_size, metadata_total_size, name_off, metadata_off;
int rc = ENOMEM;
if ( argc )
@@ -80,7 +82,7 @@ static int list_func(int argc, char *argv[])
}
done = left = 0;
- rc = xc_livepatch_list_get_sizes(xch, &nr, &name_total_size);
+ rc = xc_livepatch_list_get_sizes(xch, &nr, &name_total_size, &metadata_total_size);
if ( rc )
{
rc = errno;
@@ -108,12 +110,23 @@ static int list_func(int argc, char *argv[])
if ( !len )
goto error_len;
+ metadata = malloc(metadata_total_size * sizeof(*metadata) + 1);
+ if ( !metadata )
+ goto error_metadata;
+
+ metadata_len = malloc(nr * sizeof(*metadata_len));
+ if ( !metadata_len )
+ goto error_metadata_len;
+
memset(info, 'A', nr * sizeof(*info));
memset(name, 'B', name_total_size * sizeof(*name));
memset(len, 'C', nr * sizeof(*len));
- name_off = 0;
+ memset(metadata, 'D', metadata_total_size * sizeof(*metadata) + 1);
+ memset(metadata_len, 'E', nr * sizeof(*metadata_len));
+ name_off = metadata_off = 0;
- rc = xc_livepatch_list(xch, nr, 0, info, name, len, name_total_size, &done, &left);
+ rc = xc_livepatch_list(xch, nr, 0, info, name, len, name_total_size,
+ metadata, metadata_len, metadata_total_size, &done, &left);
if ( rc || done != nr || left > 0)
{
rc = errno;
@@ -123,23 +136,35 @@ static int list_func(int argc, char *argv[])
goto error;
}
- fprintf(stdout," ID | status\n"
- "----------------------------------------+------------\n");
+ fprintf(stdout," ID | status | metadata\n"
+ "----------------------------------------+------------+---------------\n");
for ( i = 0; i < done; i++ )
{
+ unsigned int j;
char *name_str = name + name_off;
+ char *metadata_str = metadata + metadata_off;
printf("%-40.*s| %s", len[i], name_str, state2str(info[i].state));
if ( info[i].rc )
- printf(" (%d, %s)\n", -info[i].rc, strerror(-info[i].rc));
+ printf(" (%d, %s) | ", -info[i].rc, strerror(-info[i].rc));
else
- puts("");
+ printf(" | ");
+
+ /* Replace all '\0' with semi-colons. */
+ for ( j = 0; metadata_len[i] && j < metadata_len[i] - 1; j++ )
+ metadata_str[j] = (metadata_str[j] ?: ';');
+ printf("%.*s\n", metadata_len[i], metadata_str);
name_off += len[i];
+ metadata_off += metadata_len[i];
}
error:
+ free(metadata_len);
+error_metadata_len:
+ free(metadata);
+error_metadata:
free(len);
error_len:
free(name);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 3c191eb889..6c3311f4f5 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1164,7 +1164,8 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
if ( list->nr &&
(!guest_handle_okay(list->status, list->nr) ||
- !guest_handle_okay(list->len, list->nr)) )
+ !guest_handle_okay(list->len, list->nr) ||
+ !guest_handle_okay(list->metadata_len, list->nr)) )
return -EINVAL;
spin_lock(&payload_lock);
@@ -1175,13 +1176,14 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
}
list->name_total_size = 0;
+ list->metadata_total_size = 0;
if ( list->nr )
{
- uint64_t name_offset = 0;
+ uint64_t name_offset = 0, metadata_offset = 0;
list_for_each_entry( data, &payload_list, list )
{
- uint32_t name_len;
+ uint32_t name_len, metadata_len;
if ( list->idx > i++ )
continue;
@@ -1192,8 +1194,13 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
name_len = strlen(data->name) + 1;
list->name_total_size += name_len;
+ metadata_len = data->metadata.len;
+ list->metadata_total_size += metadata_len;
+
if ( !guest_handle_subrange_okay(list->name, name_offset,
- name_offset + name_len - 1) )
+ name_offset + name_len - 1) ||
+ !guest_handle_subrange_okay(list->metadata, metadata_offset,
+ metadata_offset + metadata_len - 1) )
{
rc = -EINVAL;
break;
@@ -1203,7 +1210,10 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
if ( __copy_to_guest_offset(list->name, name_offset,
data->name, name_len) ||
__copy_to_guest_offset(list->len, idx, &name_len, 1) ||
- __copy_to_guest_offset(list->status, idx, &status, 1) )
+ __copy_to_guest_offset(list->status, idx, &status, 1) ||
+ __copy_to_guest_offset(list->metadata, metadata_offset,
+ data->metadata.data, metadata_len) ||
+ __copy_to_guest_offset(list->metadata_len, idx, &metadata_len, 1) )
{
rc = -EFAULT;
break;
@@ -1211,6 +1221,7 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
idx++;
name_offset += name_len;
+ metadata_offset += metadata_len;
if ( (idx >= list->nr) || hypercall_preempt_check() )
break;
@@ -1221,6 +1232,7 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
list_for_each_entry( data, &payload_list, list )
{
list->name_total_size += strlen(data->name) + 1;
+ list->metadata_total_size += data->metadata.len;
}
}
list->nr = payload_cnt - i; /* Remaining amount. */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 08807c92a5..5c36b3471b 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -920,16 +920,17 @@ struct xen_sysctl_livepatch_get {
};
/*
- * Retrieve an array of abbreviated status and names of payloads that are
- * loaded in the hypervisor.
+ * Retrieve an array of abbreviated status, names and metadata of payloads that
+ * are loaded in the hypervisor.
*
* If the hypercall returns an positive number, it is the number (up to `nr`)
* of the payloads returned, along with `nr` updated with the number of remaining
* payloads, `version` updated (it may be the same across hypercalls. If it varies
- * the data is stale and further calls could fail) and the name_total_size
- * containing total size of transferred data for the array.
- * The `status`, `name`, `len` are updated at their designed index value (`idx`)
- * with the returned value of data.
+ * the data is stale and further calls could fail), `name_total_size` and
+ * `metadata_total_size` containing total sizes of transferred data for both the
+ * arrays.
+ * The `status`, `name`, `len`, `metadata` and `metadata_len` are updated at their
+ * designed index value (`idx`) with the returned value of data.
*
* If the hypercall returns E2BIG the `nr` is too big and should be
* lowered. The upper limit of `nr` is left to the implemention.
@@ -953,6 +954,7 @@ struct xen_sysctl_livepatch_list {
OUT: How many payloads left. */
uint32_t pad; /* IN: Must be zero. */
uint32_t name_total_size; /* OUT: Total size of all transfer names */
+ uint32_t metadata_total_size; /* OUT: Total size of all transfer metadata */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
space allocate for nr of them. */
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
@@ -961,6 +963,11 @@ struct xen_sysctl_livepatch_list {
nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
Must have nr of them. */
+ XEN_GUEST_HANDLE_64(char) metadata; /* OUT: Array of metadata strings. Each
+ member may have an arbitrary length.
+ Must have nr of them. */
+ XEN_GUEST_HANDLE_64(uint32) metadata_len; /* OUT: Array of lengths of metadata's.
+ Must have nr of them. */
};
/*
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (10 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 11/12] livepatch: Add metadata runtime retrieval mechanism Pawel Wieczorkiewicz
@ 2019-09-28 15:13 ` Pawel Wieczorkiewicz
2019-09-30 15:13 ` Ross Lagerwall
2019-10-18 14:25 ` [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Konrad Rzeszutek Wilk
12 siblings, 1 reply; 28+ messages in thread
From: Pawel Wieczorkiewicz @ 2019-09-28 15:13 UTC (permalink / raw)
To: xen-devel
Cc: wipawel, Wei Liu, Ian Jackson, mpohlack,
Marek Marczykowski-Górecki, Pawel Wieczorkiewicz
Extend the XC python bindings library to support also all common
livepatch operations and actions.
Add the python bindings for the following operations:
- status (pyxc_livepatch_status):
Requires a payload name as an input.
Returns a status dict containing a state string and a return code
integer.
- action (pyxc_livepatch_action):
Requires a payload name and an action id as an input. Timeout and
flags are optional parameters.
Returns None or throws an exception.
- upload (pyxc_livepatch_upload):
Requires a payload name and a module's filename as an input.
Returns None or throws an exception.
- list (pyxc_livepatch_list):
Takes no parameters.
Returns a list of dicts containing each payload's:
* name as a string
* state as a string
* return code as an integer
* list of metadata key=value strings
Each functions throws an exception error based on the errno value
received from its corresponding libxc function call.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Mazein <amazein@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changed since v3:
* return None instead of integer 0 from pyxc_livepatch_action()
and pyxc_livepatch_upload()
* use fstat() instead of stat()
* simplify error condition handling code for pyxc_livepatch_upload
and also save and restore errno value
* check done and left values to handle errors in
pyxc_livepatch_list()
* use PyList_SET_ITEM() to avoid the need for PyDECREF
Changed since v1:
* changed PyList_Append() with PyList_SetItem() as requested by
Marek
tools/python/xen/lowlevel/xc/xc.c | 267 ++++++++++++++++++++++++++++++++++++++
1 file changed, 267 insertions(+)
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 9d53c4cf37..631249f9a1 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1979,6 +1979,224 @@ static PyObject *pyflask_access(PyObject *self, PyObject *args,
return Py_BuildValue("i",ret);
}
+static PyObject *pyxc_livepatch_status(XcObject *self,
+ PyObject *args,
+ PyObject *kwds)
+{
+ xen_livepatch_status_t status;
+ PyObject *info_dict = NULL;
+ char *name;
+ int rc;
+
+ static char *kwd_list[] = { "name", NULL };
+
+ if ( !PyArg_ParseTupleAndKeywords(args, kwds, "s", kwd_list, &name) )
+ goto error;
+
+ rc = xc_livepatch_get(self->xc_handle, name, &status);
+ if ( rc )
+ goto error;
+
+ info_dict = Py_BuildValue(
+ "{s:i,s:i}",
+ "state", status.state,
+ "rc", status.rc);
+
+error:
+ return info_dict ?: pyxc_error_to_exception(self->xc_handle);
+}
+
+static PyObject *pyxc_livepatch_action(XcObject *self,
+ PyObject *args,
+ PyObject *kwds)
+{
+ int (*action_func)(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags);
+ char *name;
+ unsigned int action;
+ uint32_t timeout;
+ uint64_t flags;
+ int rc = -1;
+
+ static char *kwd_list[] = { "name", "action", "timeout", "flags", NULL };
+
+ if ( !PyArg_ParseTupleAndKeywords(args, kwds, "sI|Ik", kwd_list,
+ &name, &action, &timeout, &flags) )
+ goto error;
+
+ switch (action)
+ {
+ case LIVEPATCH_ACTION_UNLOAD:
+ action_func = xc_livepatch_unload;
+ break;
+ case LIVEPATCH_ACTION_REVERT:
+ action_func = xc_livepatch_revert;
+ break;
+ case LIVEPATCH_ACTION_APPLY:
+ action_func = xc_livepatch_apply;
+ break;
+ case LIVEPATCH_ACTION_REPLACE:
+ action_func = xc_livepatch_replace;
+ break;
+ default:
+ goto error;
+ }
+
+ rc = action_func(self->xc_handle, name, timeout, flags);
+
+error:
+ return rc ? pyxc_error_to_exception(self->xc_handle) : Py_None;
+}
+
+static PyObject *pyxc_livepatch_upload(XcObject *self,
+ PyObject *args,
+ PyObject *kwds)
+{
+ unsigned char *fbuf = MAP_FAILED;
+ char *name, *filename;
+ struct stat buf;
+ int fd = 0, rc = -1, saved_errno;
+ ssize_t len;
+
+ static char *kwd_list[] = { "name", "filename", NULL };
+
+ if ( !PyArg_ParseTupleAndKeywords(args, kwds, "ss", kwd_list,
+ &name, &filename))
+ goto error;
+
+ fd = open(filename, O_RDONLY);
+ if ( fd < 0 )
+ goto error;
+
+ if ( fstat(fd, &buf) != 0 )
+ goto error;
+
+ len = buf.st_size;
+ fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
+ if ( fbuf == MAP_FAILED )
+ goto error;
+
+ rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
+
+ saved_errno = errno;
+ munmap(fbuf, len);
+ close(fd);
+ errno = saved_errno;
+
+error:
+ return rc ? pyxc_error_to_exception(self->xc_handle) : Py_None;
+}
+
+static PyObject *pyxc_livepatch_list(XcObject *self)
+{
+ PyObject *list = Py_None;
+ unsigned int nr, done, left, i;
+ xen_livepatch_status_t *info = NULL;
+ char *name = NULL;
+ char *metadata = NULL;
+ uint32_t *len = NULL;
+ uint32_t *metadata_len = NULL;
+ uint32_t name_total_size, metadata_total_size;
+ uint32_t name_off, metadata_off;
+ int rc;
+
+ done = left = 0;
+ rc = xc_livepatch_list_get_sizes(self->xc_handle, &nr,
+ &name_total_size, &metadata_total_size);
+ if ( rc )
+ goto error;
+
+ if ( nr == 0 )
+ return PyList_New(0);
+
+ rc = ENOMEM;
+ info = malloc(nr * sizeof(*info));
+ if ( !info )
+ goto error;
+
+ name = malloc(name_total_size * sizeof(*name));
+ if ( !name )
+ goto error;
+
+ len = malloc(nr * sizeof(*len));
+ if ( !len )
+ goto error;
+
+ metadata = malloc(metadata_total_size * sizeof(*metadata));
+ if ( !metadata )
+ goto error;
+
+ metadata_len = malloc(nr * sizeof(*metadata_len));
+ if ( !metadata_len )
+ goto error;
+
+ rc = xc_livepatch_list(self->xc_handle, nr, 0, info,
+ name, len, name_total_size,
+ metadata, metadata_len, metadata_total_size,
+ &done, &left);
+ if ( rc )
+ goto error;
+
+ if ( done != nr || left > 0 )
+ {
+ rc = EFAULT;
+ goto error;
+ }
+
+ list = PyList_New(done);
+ name_off = metadata_off = 0;
+ for ( i = 0; i < done; i++ )
+ {
+ PyObject *info_dict, *metadata_list;
+ char *name_str, *metadata_str;
+
+ name_str = name + name_off;
+ metadata_str = metadata + metadata_off;
+
+ metadata_list = PyList_New(0);
+ for ( char *s = metadata_str; s < metadata_str + metadata_len[i]; s += strlen(s) + 1 )
+ {
+ PyObject *field = Py_BuildValue("s", s);
+ if ( field == NULL )
+ {
+ Py_DECREF(list);
+ Py_DECREF(metadata_list);
+ rc = EFAULT;
+ goto error;
+ }
+
+ PyList_Append(metadata_list, field);
+ Py_DECREF(field);
+ }
+
+ info_dict = Py_BuildValue(
+ "{s:s,s:i,s:i,s:N}",
+ "name", name_str,
+ "state", info[i].state,
+ "rc", info[i].rc,
+ "metadata", metadata_list);
+
+ if ( info_dict == NULL )
+ {
+ Py_DECREF(list);
+ Py_DECREF(metadata_list);
+ rc = EFAULT;
+ goto error;
+ }
+ PyList_SET_ITEM(list, i, info_dict);
+
+ name_off += len[i];
+ metadata_off += metadata_len[i];
+ }
+
+error:
+ free(info);
+ free(name);
+ free(len);
+ free(metadata);
+ free(metadata_len);
+ return rc ? pyxc_error_to_exception(self->xc_handle) : list;
+}
+
static PyMethodDef pyxc_methods[] = {
{ "domain_create",
(PyCFunction)pyxc_domain_create,
@@ -2542,6 +2760,44 @@ static PyMethodDef pyxc_methods[] = {
"Returns: [int]: 0 on all permission granted; -1 if any permissions are \
denied\n" },
+ { "livepatch_status",
+ (PyCFunction)pyxc_livepatch_status,
+ METH_KEYWORDS, "\n"
+ "Gets current state and return code for a specified module.\n"
+ " name [str]: Module name to be used\n"
+ "Returns: [dict] on success; throwing an exception on error\n"
+ " state [int]: Module current state: CHECKED or APPLIED\n"
+ " rc [int]: Return code of last module's operation\n" },
+
+ { "livepatch_upload",
+ (PyCFunction)pyxc_livepatch_upload,
+ METH_KEYWORDS, "\n"
+ "Uploads a module with specified name from filename.\n"
+ " name [str]: Module name to be used\n"
+ " filename [str]: Filename of a module to be uploaded\n"
+ "Returns: None on success; throwing an exception on error\n" },
+
+ { "livepatch_action",
+ (PyCFunction)pyxc_livepatch_action,
+ METH_KEYWORDS, "\n"
+ "Performs an action (unload, revert, apply or replace) on a specified \
+ module.\n"
+ " name [str]: Module name to be used\n"
+ " action [uint]: Action enum id\n"
+ " timeout [uint]: Action scheduled execution timeout\n"
+ " flags [ulong]: Flags specifying action's extra parameters\n"
+ "Returns: None on success; throwing an exception on error\n" },
+
+ { "livepatch_list",
+ (PyCFunction)pyxc_livepatch_list,
+ METH_NOARGS, "\n"
+ "List all uploaded livepatch modules with their current state and metadata.\n"
+ "Returns: [list of dicts] on success; throwing an exception on error\n"
+ " name [str]: Module name\n"
+ " state [int]: Module current state: CHECKED or APPLIED\n"
+ " rc [int]: Return code of last module's operation\n"
+ " metadata [list]: List of module's metadata 'key=value' strings\n" },
+
{ NULL, NULL, 0, NULL }
};
@@ -2653,6 +2909,17 @@ PyMODINIT_FUNC initxc(void)
PyModule_AddIntConstant(m, "XEN_SCHEDULER_CREDIT", XEN_SCHEDULER_CREDIT);
PyModule_AddIntConstant(m, "XEN_SCHEDULER_CREDIT2", XEN_SCHEDULER_CREDIT2);
+ /* Expose livepatch constants to Python */
+ PyModule_AddIntConstant(m, "LIVEPATCH_ACTION_UNLOAD", LIVEPATCH_ACTION_UNLOAD);
+ PyModule_AddIntConstant(m, "LIVEPATCH_ACTION_REVERT", LIVEPATCH_ACTION_REVERT);
+ PyModule_AddIntConstant(m, "LIVEPATCH_ACTION_APPLY", LIVEPATCH_ACTION_APPLY);
+ PyModule_AddIntConstant(m, "LIVEPATCH_ACTION_REPLACE", LIVEPATCH_ACTION_REPLACE);
+
+ PyModule_AddIntConstant(m, "LIVEPATCH_ACTION_APPLY_NODEPS", LIVEPATCH_ACTION_APPLY_NODEPS);
+
+ PyModule_AddIntConstant(m, "LIVEPATCH_STATE_APPLIED", LIVEPATCH_STATE_APPLIED);
+ PyModule_AddIntConstant(m, "LIVEPATCH_STATE_CHECKED", LIVEPATCH_STATE_CHECKED);
+
#if PY_MAJOR_VERSION >= 3
return m;
#endif
--
2.16.5
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation Pawel Wieczorkiewicz
@ 2019-09-30 8:50 ` Jan Beulich
2019-09-30 10:58 ` Wieczorkiewicz, Pawel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2019-09-30 8:50 UTC (permalink / raw)
To: Pawel Wieczorkiewicz
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, mpohlack, Tim Deegan,
Ross Lagerwall, Julien Grall, xen-devel
On 28.09.2019 17:13, Pawel Wieczorkiewicz wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -925,10 +925,11 @@ struct xen_sysctl_livepatch_get {
> *
> * If the hypercall returns an positive number, it is the number (up to `nr`)
> * of the payloads returned, along with `nr` updated with the number of remaining
> - * payloads, `version` updated (it may be the same across hypercalls. If it
> - * varies the data is stale and further calls could fail). The `status`,
> - * `name`, and `len`' are updated at their designed index value (`idx`) with
> - * the returned value of data.
> + * payloads, `version` updated (it may be the same across hypercalls. If it varies
> + * the data is stale and further calls could fail) and the name_total_size
> + * containing total size of transferred data for the array.
> + * The `status`, `name`, `len` are updated at their designed index value (`idx`)
> + * with the returned value of data.
> *
> * If the hypercall returns E2BIG the `nr` is too big and should be
> * lowered. The upper limit of `nr` is left to the implemention.
> @@ -951,11 +952,13 @@ struct xen_sysctl_livepatch_list {
> amount of payloads and version.
> OUT: How many payloads left. */
> uint32_t pad; /* IN: Must be zero. */
> + uint32_t name_total_size; /* OUT: Total size of all transfer names */
> XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
> space allocate for nr of them. */
> XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
> - MUST XEN_LIVEPATCH_NAME_SIZE in size.
> - Must have nr of them. */
> + may have an arbitrary length up to
> + XEN_LIVEPATCH_NAME_SIZE bytes. Must have
> + nr of them. */
> XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
> Must have nr of them. */
> };
Non-binary-compatible changes require an interface version bump.
I wonder though why you don't use the "pad" field; in fact the
way you make the change you'd have to introduce a 2nd padding
field, to make padding explicit _and_ check it's zero on input
(for future extensibility _without_ having to bump the interface
version).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation
2019-09-30 8:50 ` Jan Beulich
@ 2019-09-30 10:58 ` Wieczorkiewicz, Pawel
2019-09-30 11:11 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-09-30 10:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ross Lagerwall, George Dunlap,
Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
Pohlack, Martin, Wieczorkiewicz, Pawel, Julien Grall, xen-devel
> On 30. Sep 2019, at 10:50, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.09.2019 17:13, Pawel Wieczorkiewicz wrote:
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -925,10 +925,11 @@ struct xen_sysctl_livepatch_get {
>> *
>>
snip
>> uint32_t pad; /* IN: Must be zero. */
>> + uint32_t name_total_size; /* OUT: Total size of all transfer names */
>> XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
>> space allocate for nr of them. */
>> XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
>> - MUST XEN_LIVEPATCH_NAME_SIZE in size.
>> - Must have nr of them. */
>> + may have an arbitrary length up to
>> + XEN_LIVEPATCH_NAME_SIZE bytes. Must have
>> + nr of them. */
>> XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
>> Must have nr of them. */
>> };
>
> Non-binary-compatible changes require an interface version bump.
The bump happens with this patch of the patchset:
https://patchwork.kernel.org/patch/11165427/
> I wonder though why you don't use the "pad" field; in fact the
> way you make the change you'd have to introduce a 2nd padding
> field, to make padding explicit _and_ check it's zero on input
> (for future extensibility _without_ having to bump the interface
> version).
>
I do not use the pad field because I introduce another field with the
next patch of the patchset: https://patchwork.kernel.org/patch/11165433/
Then I would have to re-add the pad field again I suppose.
Also, I was (false?) impression that the pad field is dedicated to
the future input parameters, so I should not touch it.
> Jan
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency Pawel Wieczorkiewicz
@ 2019-09-30 11:05 ` Jan Beulich
2019-09-30 11:14 ` Wieczorkiewicz, Pawel
2019-11-08 10:36 ` Ross Lagerwall
1 sibling, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2019-09-30 11:05 UTC (permalink / raw)
To: Pawel Wieczorkiewicz
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, mpohlack, Tim Deegan,
RossLagerwall, Julien Grall, xen-devel
On 28.09.2019 17:12, Pawel Wieczorkiewicz wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -35,7 +35,7 @@
> #include "domctl.h"
> #include "physdev.h"
>
> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
Oh, I see.
> @@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action {
> /* hypervisor default. */
> /* Or upper bound of time (ns) */
> /* for operation to take. */
> +
> +/*
> + * Overwrite default inter-module buildid dependency chain enforcement.
> + * Check only if module is built for given hypervisor by comparing buildid.
> + */
> +#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
> + uint64_t flags; /* IN: action flags. */
> + /* Provide additional parameters */
> + /* for an action. */
64 bits seems quite a lot (and unusual) for a flags field. Also,
as a nit, do you perhaps mean "override" instead of "overwrite"
in the comment?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation
2019-09-30 10:58 ` Wieczorkiewicz, Pawel
@ 2019-09-30 11:11 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2019-09-30 11:11 UTC (permalink / raw)
To: Wieczorkiewicz, Pawel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, Tim Deegan,
Martin Pohlack, Ross Lagerwall, Julien Grall, xen-devel
On 30.09.2019 12:58, Wieczorkiewicz, Pawel wrote:
>
>
>> On 30. Sep 2019, at 10:50, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.09.2019 17:13, Pawel Wieczorkiewicz wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -925,10 +925,11 @@ struct xen_sysctl_livepatch_get {
>>> *
>>>
> snip
>>> uint32_t pad; /* IN: Must be zero. */
>>> + uint32_t name_total_size; /* OUT: Total size of all transfer names */
>>> XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have enough
>>> space allocate for nr of them. */
>>> XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
>>> - MUST XEN_LIVEPATCH_NAME_SIZE in size.
>>> - Must have nr of them. */
>>> + may have an arbitrary length up to
>>> + XEN_LIVEPATCH_NAME_SIZE bytes. Must have
>>> + nr of them. */
>>> XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of name's.
>>> Must have nr of them. */
>>> };
>>
>> Non-binary-compatible changes require an interface version bump.
>
> The bump happens with this patch of the patchset:
> https://patchwork.kernel.org/patch/11165427/
>
>> I wonder though why you don't use the "pad" field; in fact the
>> way you make the change you'd have to introduce a 2nd padding
>> field, to make padding explicit _and_ check it's zero on input
>> (for future extensibility _without_ having to bump the interface
>> version).
>>
>
> I do not use the pad field because I introduce another field with the
> next patch of the patchset: https://patchwork.kernel.org/patch/11165433/
> Then I would have to re-add the pad field again I suppose.
Yes indeed; this may seem a little cumbersome, but you want your
series structured that a large time gap between any two parts of
which is not going to result in not well formed code.
> Also, I was (false?) impression that the pad field is dedicated to
> the future input parameters, so I should not touch it.
With its padding function it's IN only (and I believe is being
checked to have been set to zero by the caller). Once assigned
a new purpose, it can as well become OUT, provided the prior
meaning of zero in the field doesn't have a chance of confusing
callers.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency
2019-09-30 11:05 ` Jan Beulich
@ 2019-09-30 11:14 ` Wieczorkiewicz, Pawel
2019-09-30 11:21 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-09-30 11:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, RossLagerwall, George Dunlap,
Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
Pohlack, Martin, Wieczorkiewicz, Pawel, Julien Grall, xen-devel
> On 30. Sep 2019, at 13:05, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.09.2019 17:12, Pawel Wieczorkiewicz wrote:
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -35,7 +35,7 @@
>> #include "domctl.h"
>> #include "physdev.h"
>>
>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
>
> Oh, I see.
>
>> @@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action {
>> /* hypervisor default. */
>> /* Or upper bound of time (ns) */
>> /* for operation to take. */
>> +
>> +/*
>> + * Overwrite default inter-module buildid dependency chain enforcement.
>> + * Check only if module is built for given hypervisor by comparing buildid.
>> + */
>> +#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
>> + uint64_t flags; /* IN: action flags. */
>> + /* Provide additional parameters */
>> + /* for an action. */
>
> 64 bits seems quite a lot (and unusual) for a flags field. Also,
When I use uint32_t for the flags, I would have to also add another
one for padding I suppose.
Why not just use that space for future flags? So, I chose uint64_t.
> as a nit, do you perhaps mean "override" instead of "overwrite"
> in the comment?
>
Indeed I do. Will fix in v5.
> Jan
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency
2019-09-30 11:14 ` Wieczorkiewicz, Pawel
@ 2019-09-30 11:21 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2019-09-30 11:21 UTC (permalink / raw)
To: Wieczorkiewicz, Pawel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, IanJackson, TimDeegan,
Martin Pohlack, RossLagerwall, Julien Grall, xen-devel
On 30.09.2019 13:14, Wieczorkiewicz, Pawel wrote:
>
>
>> On 30. Sep 2019, at 13:05, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.09.2019 17:12, Pawel Wieczorkiewicz wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -35,7 +35,7 @@
>>> #include "domctl.h"
>>> #include "physdev.h"
>>>
>>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
>>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
>>
>> Oh, I see.
>>
>>> @@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action {
>>> /* hypervisor default. */
>>> /* Or upper bound of time (ns) */
>>> /* for operation to take. */
>>> +
>>> +/*
>>> + * Overwrite default inter-module buildid dependency chain enforcement.
>>> + * Check only if module is built for given hypervisor by comparing buildid.
>>> + */
>>> +#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
>>> + uint64_t flags; /* IN: action flags. */
>>> + /* Provide additional parameters */
>>> + /* for an action. */
>>
>> 64 bits seems quite a lot (and unusual) for a flags field. Also,
>
> When I use uint32_t for the flags, I would have to also add another
> one for padding I suppose.
> Why not just use that space for future flags? So, I chose uint64_t.
uint64_t is (on x86 and 32-bit architectures at least) less efficient
to deal with.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations Pawel Wieczorkiewicz
@ 2019-09-30 15:13 ` Ross Lagerwall
2019-10-01 7:44 ` Wieczorkiewicz, Pawel
0 siblings, 1 reply; 28+ messages in thread
From: Ross Lagerwall @ 2019-09-30 15:13 UTC (permalink / raw)
To: Pawel Wieczorkiewicz, xen-devel
Cc: wipawel, Ian Jackson, mpohlack, Wei Liu, Marek Marczykowski-Górecki
On 9/28/19 4:13 PM, Pawel Wieczorkiewicz wrote:
> Extend the XC python bindings library to support also all common
> livepatch operations and actions.
>
> Add the python bindings for the following operations:
> - status (pyxc_livepatch_status):
> Requires a payload name as an input.
> Returns a status dict containing a state string and a return code
> integer.
> - action (pyxc_livepatch_action):
> Requires a payload name and an action id as an input. Timeout and
> flags are optional parameters.
> Returns None or throws an exception.
> - upload (pyxc_livepatch_upload):
> Requires a payload name and a module's filename as an input.
> Returns None or throws an exception.
> - list (pyxc_livepatch_list):
> Takes no parameters.
> Returns a list of dicts containing each payload's:
> * name as a string
> * state as a string
> * return code as an integer
> * list of metadata key=value strings
>
> Each functions throws an exception error based on the errno value
> received from its corresponding libxc function call.
>
snip> +static PyObject *pyxc_livepatch_upload(XcObject *self,
> + PyObject *args,
> + PyObject *kwds)
> +{
> + unsigned char *fbuf = MAP_FAILED;
> + char *name, *filename;
> + struct stat buf;
> + int fd = 0, rc = -1, saved_errno;
> + ssize_t len;
> +
> + static char *kwd_list[] = { "name", "filename", NULL };
> +
> + if ( !PyArg_ParseTupleAndKeywords(args, kwds, "ss", kwd_list,
> + &name, &filename))
> + goto error;
> +
> + fd = open(filename, O_RDONLY);
> + if ( fd < 0 )
> + goto error;
> +
> + if ( fstat(fd, &buf) != 0 )
> + goto error;
> +
> + len = buf.st_size;
> + fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
> + if ( fbuf == MAP_FAILED )
> + goto error;
> +
> + rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
> +
> + saved_errno = errno;
> + munmap(fbuf, len);
> + close(fd);
> + errno = saved_errno;
> +
> +error:
> + return rc ? pyxc_error_to_exception(self->xc_handle) : Py_None;
> +}
> +
The fstat() and mmap() error paths leak fd on error.
Regards,
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations
2019-09-30 15:13 ` Ross Lagerwall
@ 2019-10-01 7:44 ` Wieczorkiewicz, Pawel
0 siblings, 0 replies; 28+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-10-01 7:44 UTC (permalink / raw)
To: Ross Lagerwall
Cc: Wei Liu, Ian Jackson, Marek Marczykowski-Górecki, Pohlack,
Martin, Wieczorkiewicz, Pawel, xen-devel
> On 30. Sep 2019, at 17:13, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
>
> On 9/28/19 4:13 PM, Pawel Wieczorkiewicz wrote:
>> Extend the XC python bindings library to support also all common
snip
>>
>> +
>> + fd = open(filename, O_RDONLY);
>> + if ( fd < 0 )
>> + goto error;
>> +
>> + if ( fstat(fd, &buf) != 0 )
>> + goto error;
>> +
>> + len = buf.st_size;
>> + fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
>> + if ( fbuf == MAP_FAILED )
>> + goto error;
>> +
>> + rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
>> +
>> + saved_errno = errno;
>> + munmap(fbuf, len);
>> + close(fd);
>> + errno = saved_errno;
>> +
>> +error:
>> + return rc ? pyxc_error_to_exception(self->xc_handle) : Py_None;
>> +}
>> +
> The fstat() and mmap() error paths leak fd on error.
>
Oh boy, the better is sometimes an enemy of good :-).
I will fix it, thanks!
> Regards,
> --
> Ross Lagerwall
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
` (11 preceding siblings ...)
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations Pawel Wieczorkiewicz
@ 2019-10-18 14:25 ` Konrad Rzeszutek Wilk
2019-10-18 15:34 ` Ross Lagerwall
12 siblings, 1 reply; 28+ messages in thread
From: Konrad Rzeszutek Wilk @ 2019-10-18 14:25 UTC (permalink / raw)
To: Pawel Wieczorkiewicz, jgross
Cc: Marek Marczykowski-Górecki, wipawel, Stefano Stabellini,
Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, mpohlack,
Tim Deegan, Ross Lagerwall, Julien Grall, Jan Beulich, xen-devel
On Sat, Sep 28, 2019 at 03:12:53PM +0000, Pawel Wieczorkiewicz wrote:
> This series introduces new features to the livepatch functionality as
> briefly discussed during Xen Developer Summit 2019: [a] and [b].
> It also provides a few fixes and some small improvements.
Heya,
Is there an v5 of the patches somewhere brewing so that I can give them
one last test?
Juergen,
Are you OK with giving them an Release-Ack? I think there are only two minor
changes that Jan requested.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
2019-10-18 14:25 ` [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Konrad Rzeszutek Wilk
@ 2019-10-18 15:34 ` Ross Lagerwall
2019-10-19 10:18 ` Wieczorkiewicz, Pawel
0 siblings, 1 reply; 28+ messages in thread
From: Ross Lagerwall @ 2019-10-18 15:34 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Pawel Wieczorkiewicz, jgross
Cc: Marek Marczykowski-Górecki, wipawel, Stefano Stabellini,
Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, mpohlack,
Tim Deegan, Julien Grall, Jan Beulich, xen-devel
On 10/18/19 3:25 PM, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 28, 2019 at 03:12:53PM +0000, Pawel Wieczorkiewicz wrote:
>> This series introduces new features to the livepatch functionality as
>> briefly discussed during Xen Developer Summit 2019: [a] and [b].
>> It also provides a few fixes and some small improvements.
>
> Heya,
>
> Is there an v5 of the patches somewhere brewing so that I can give them
> one last test?
>
> Juergen,
>
> Are you OK with giving them an Release-Ack? I think there are only two minor
> changes that Jan requested.
>
I have not yet had time to review some of the patches in v4. I expect I
will have time on Monday to look at the rest.
Thanks,
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
2019-10-18 15:34 ` Ross Lagerwall
@ 2019-10-19 10:18 ` Wieczorkiewicz, Pawel
2019-10-19 13:46 ` Jürgen Groß
0 siblings, 1 reply; 28+ messages in thread
From: Wieczorkiewicz, Pawel @ 2019-10-19 10:18 UTC (permalink / raw)
To: Ross Lagerwall, Konrad Rzeszutek Wilk
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Marek Marczykowski-Górecki,
Tim Deegan, Pohlack, Martin, Wieczorkiewicz, Pawel, Julien Grall,
Jan Beulich, Xen-devel
> On 18. Oct 2019, at 17:34, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
>
> On 10/18/19 3:25 PM, Konrad Rzeszutek Wilk wrote:
>> On Sat, Sep 28, 2019 at 03:12:53PM +0000, Pawel Wieczorkiewicz wrote:
>>> This series introduces new features to the livepatch functionality as
>>> briefly discussed during Xen Developer Summit 2019: [a] and [b].
>>> It also provides a few fixes and some small improvements.
>> Heya,
>> Is there an v5 of the patches somewhere brewing so that I can give them
>> one last test?
Yes, it is… but I am waiting for more reviews before publishing another revision.
>> Juergen,
>> Are you OK with giving them an Release-Ack? I think there are only two minor
>> changes that Jan requested.
>
> I have not yet had time to review some of the patches in v4. I expect I will have time on Monday to look at the rest.
>
Due to other urgent work I won’t be able to look into this for a week or two.
> Thanks,
> --
> Ross Lagerwall
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
2019-10-19 10:18 ` Wieczorkiewicz, Pawel
@ 2019-10-19 13:46 ` Jürgen Groß
0 siblings, 0 replies; 28+ messages in thread
From: Jürgen Groß @ 2019-10-19 13:46 UTC (permalink / raw)
To: Wieczorkiewicz, Pawel, Ross Lagerwall, Konrad Rzeszutek Wilk
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Marek Marczykowski-Górecki, Tim Deegan,
Pohlack, Martin, Julien Grall, Jan Beulich, Xen-devel
On 19.10.19 12:18, Wieczorkiewicz, Pawel wrote:
>
>
>> On 18. Oct 2019, at 17:34, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
>>
>> On 10/18/19 3:25 PM, Konrad Rzeszutek Wilk wrote:
>>> On Sat, Sep 28, 2019 at 03:12:53PM +0000, Pawel Wieczorkiewicz wrote:
>>>> This series introduces new features to the livepatch functionality as
>>>> briefly discussed during Xen Developer Summit 2019: [a] and [b].
>>>> It also provides a few fixes and some small improvements.
>>> Heya,
>>> Is there an v5 of the patches somewhere brewing so that I can give them
>>> one last test?
>
> Yes, it is… but I am waiting for more reviews before publishing another revision.
>
>>> Juergen,
>>> Are you OK with giving them an Release-Ack? I think there are only two minor
>>> changes that Jan requested.
>>
>> I have not yet had time to review some of the patches in v4. I expect I will have time on Monday to look at the rest.
>>
>
> Due to other urgent work I won’t be able to look into this for a week or two.
Which will _really_ make it too late for 4.13, sorry.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency Pawel Wieczorkiewicz
2019-09-30 11:05 ` Jan Beulich
@ 2019-11-08 10:36 ` Ross Lagerwall
1 sibling, 0 replies; 28+ messages in thread
From: Ross Lagerwall @ 2019-11-08 10:36 UTC (permalink / raw)
To: Pawel Wieczorkiewicz, xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, mpohlack, Tim Deegan,
Julien Grall, Jan Beulich
On 9/28/19 4:12 PM, Pawel Wieczorkiewicz wrote:
> By default Livepatch enforces the following buildid-based dependency
> chain between livepatch modules:
> 1) first module depends on given hypervisor buildid
> 2) every consecutive module depends on previous module's buildid
> This way proper livepatch stack order is maintained and enforced.
> While it is important for production livepatches it limits agility and
> blocks usage of testing or debug livepatches. These kinds of livepatch
> modules are typically expected to be loaded at any time irrespective
> of current state of the modules stack.
>
> To enable testing and debug livepatches allow user dynamically ignore
> the inter-modules dependency. In this case only hypervisor buildid
> match is verified and enforced.
>
> To allow userland pass additional paremeters for livepatch actions
> add support for action flags.
> Each of the apply, revert, unload and revert action gets additional
> 64-bit parameter 'flags' where extra flags can be applied in a mask
> form.
> Initially only one flag '--nodeps' is added for the apply action.
> This flag modifies the default buildid dependency check as described
> above.
> The global sysctl interface input flag parameter is defined with a
> single corresponding flag macro:
> LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
>
> The userland xen-livepatch tool is modified to support the '--nodeps'
> flag for apply and load commands. A general mechanism for specifying
> more flags in the future for apply and other action is however added.
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
> Reviewed-by: Petre Eftime <epetre@amazon.com>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Changed since v3:
> * simplified loop in xen-livepatch.c
>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks Pawel Wieczorkiewicz
@ 2019-11-08 10:43 ` Ross Lagerwall
0 siblings, 0 replies; 28+ messages in thread
From: Ross Lagerwall @ 2019-11-08 10:43 UTC (permalink / raw)
To: Pawel Wieczorkiewicz, xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, mpohlack, Tim Deegan,
Julien Grall, Jan Beulich
On 9/28/19 4:12 PM, Pawel Wieczorkiewicz wrote:
> This is an implementation of 4 new livepatch module vetoing hooks,
> that can be optionally supplied along with modules.
> Hooks that currently exists in the livepatch mechanism aren't agile
> enough and have various limitations:
> * run only from within a quiescing zone
> * cannot conditionally prevent applying or reverting
> * do not have access to the module context
> To address these limitations the following has been implemented:
> 1) pre-apply hook
> runs before the apply action is scheduled for execution. Its main
> purpose is to prevent from applying a livepatch when certain
> expected conditions aren't met or when mutating actions implemented
> in the hook fail or cannot be executed.
>
> 2) post-apply hook
> runs after the apply action has been executed and quiescing zone
> exited. Its main purpose is to provide an ability to follow-up on
> actions performed by the pre- hook, when module application was
> successful or undo certain preparation steps of the pre- hook in
> case of a failure. The success/failure error code is provided to
> the post- hooks via the rc field of the payload structure.
>
> 3) pre-revert hook
> runs before the revert action is scheduled for execution. Its main
> purpose is to prevent from reverting a livepatch when certain
> expected conditions aren't met or when mutating actions implemented
> in the hook fail or cannot be executed.
>
> 4) post-revert hook
> runs after the revert action has been executed and quiescing zone
> exited. Its main purpose is to perform cleanup of all previously
> executed mutating actions in order to restore the original system
> state from before the current module application.
> The success/failure error code is provided to the post- hooks via
> the rc field of the payload structure.
>
> The replace action performs atomically the following actions:
> - revert all applied modules
> - apply a single replacement module.
> With the vetoing hooks in place various inter-hook dependencies may
> arise. Also, during the revert part of the operation certain vetoing
> hooks may detect failing conditions that previously were satisfied.
> That could in turn lead to situation when the revert part must be
> rolled back with all the pre- and post- hooks re-applied, which again
> can't be guaranteed to always succeed.
> The simplest response to this complication is to disallow the replace
> action completely on modules with vetoing hooks.
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Petre Eftime <epetre@amazon.com>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker Pawel Wieczorkiewicz
@ 2019-11-08 10:52 ` Ross Lagerwall
0 siblings, 0 replies; 28+ messages in thread
From: Ross Lagerwall @ 2019-11-08 10:52 UTC (permalink / raw)
To: Pawel Wieczorkiewicz, xen-devel
Cc: wipawel, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, mpohlack, Tim Deegan,
Julien Grall, Jan Beulich
On 9/28/19 4:13 PM, Pawel Wieczorkiewicz wrote:
> Livepatch only tracks an entire payload applied/reverted state. But,
> with an option to supply the apply_payload() and/or revert_payload()
> functions as optional hooks, it becomes possible to intermix the
> execution of the original apply_payload()/revert_payload() functions
> with their dynamically supplied counterparts.
> It is important then to track the current state of every function
> being patched and prevent situations of unintentional double-apply
> or unapplied revert.
>
> To support that, it is necessary to extend public interface of the
> livepatch. The struct livepatch_func gets additional field holding
> the applied/reverted state marker.
>
> To reflect the livepatch payload ABI change, bump the version flag
> LIVEPATCH_PAYLOAD_VERSION up to 2.
>
> [And also update the top of the design document]
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Acked-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2019-11-08 10:53 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-28 15:12 [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 01/12] livepatch: Always check hypervisor build ID upon livepatch upload Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 02/12] livepatch: Allow to override inter-modules buildid dependency Pawel Wieczorkiewicz
2019-09-30 11:05 ` Jan Beulich
2019-09-30 11:14 ` Wieczorkiewicz, Pawel
2019-09-30 11:21 ` Jan Beulich
2019-11-08 10:36 ` Ross Lagerwall
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 03/12] livepatch: Export payload structure via livepatch_payload.h Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 04/12] livepatch: Implement pre-|post- apply|revert hooks Pawel Wieczorkiewicz
2019-11-08 10:43 ` Ross Lagerwall
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 05/12] livepatch: Add support for apply|revert action replacement hooks Pawel Wieczorkiewicz
2019-09-28 15:12 ` [Xen-devel] [PATCH v4 06/12] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 07/12] livepatch: Add per-function applied/reverted state tracking marker Pawel Wieczorkiewicz
2019-11-08 10:52 ` Ross Lagerwall
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 08/12] livepatch: Add support for inline asm livepatching expectations Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 09/12] livepatch: Add support for modules .modinfo section metadata Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 10/12] livepatch: Handle arbitrary size names with the list operation Pawel Wieczorkiewicz
2019-09-30 8:50 ` Jan Beulich
2019-09-30 10:58 ` Wieczorkiewicz, Pawel
2019-09-30 11:11 ` Jan Beulich
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 11/12] livepatch: Add metadata runtime retrieval mechanism Pawel Wieczorkiewicz
2019-09-28 15:13 ` [Xen-devel] [PATCH v4 12/12] livepatch: Add python bindings for livepatch operations Pawel Wieczorkiewicz
2019-09-30 15:13 ` Ross Lagerwall
2019-10-01 7:44 ` Wieczorkiewicz, Pawel
2019-10-18 14:25 ` [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes Konrad Rzeszutek Wilk
2019-10-18 15:34 ` Ross Lagerwall
2019-10-19 10:18 ` Wieczorkiewicz, Pawel
2019-10-19 13:46 ` Jürgen Groß
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.