Hi folks,
This is the third installment of the klp-convert tool for generating and
processing livepatch symbols for livepatch module builds. For those
following along at home, archive links to previous versions:
RFC:
https://lore.kernel.org/lkml/[email protected]/
v2:
https://lore.kernel.org/lkml/[email protected]/
(Note that I don't see v2 archived at lore, but that is a link to the
most recent subthread that lore did catch.)
Livepatches may use symbols which are not contained in its own scope,
and, because of that, may end up compiled with relocations that will
only be resolved during module load. Yet, when the referenced symbols are
not exported, solving this relocation requires information on the object
that holds the symbol (either vmlinux or modules) and its position inside
the object, as an object may contain multiple symbols with the same name.
Providing such information must be done accordingly to what is specified
in Documentation/livepatch/module-elf-format.txt.
Currently, there is no trivial way to embed the required information as
requested in the final livepatch elf object. klp-convert solves this
problem in two different forms: (i) by relying on a symbol map, which is
built during kernel compilation, to automatically infer the relocation
targeted symbol, and, when such inference is not possible (ii) by using
annotations in the elf object to convert the relocation accordingly to
the specification, enabling it to be handled by the livepatch loader.
Given the above, add support for symbol mapping in the form of
Symbols.list file; add klp-convert tool; integrate klp-convert tool into
kbuild; make livepatch modules discernible during kernel compilation
pipeline; add data-structure and macros to enable users to annotate
livepatch source code; make modpost stage compatible with livepatches;
update livepatch-sample and update documentation.
The patch was tested under three use-cases:
use-case 1: There is a relocation in the lp that can be automatically
resolved by klp-convert. For example. see the saved_command_line
variable in lib/livepatch/test_klp_convert2.c.
use-case 2: There is a relocation in the lp that cannot be automatically
resolved, as the name of the respective symbol appears in multiple
objects. The livepatch contains an annotation to enable a correct
relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
in lib/livepatch/test_klp_convert{1,2}.c.
use-case 3: There is a relocation in the lp that cannot be automatically
resolved similarly as 2, but no annotation was provided in the
livepatch, triggering an error during compilation. Reproducible by
removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
lib/livepatch/test_klp_convert{1,2}.c.
Branches
--------
Since I spent some time tinkering with v2 and accumulated a bunch of
fixes, I collected them up and am posting this new version. For review
purposes, I posted two branches up to github:
1 - an expanded branch that with changes separate from the original
https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded
2 - a squashed branch of (1) that comprises v3:
https://github.com/joe-lawrence/linux/commits/klp-convert-v3
Non-trivial commits in the expanded branch have some extra commentary
and details for debugging in the commit message that were dropped when
squashing into their respective parent commits.
TODO
----
There are a few outstanding items that I came across while reviewing v2,
but as changes started accumulating, it made sense to defer those to a
later version. I'll reply to this thread individual topics to start
discussion sub-threads for those.
Changelogs
----------
The commit changelogs were getting long, so I've moved them here for
posterity and review purposes:
livepatch: Create and include UAPI headers
v2:
- jmoreira: split up and changelog
v3:
- joe: convert to SPDX license tags
kbuild: Support for Symbols.list creation
v3:
- jmoreira: adjust for multiobject livepatch
- joe: add klpclean to PHONY
- joe: align KLP prefix
livepatch: Add klp-convert tool
v2:
- khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
at the end
- jmoreira: add support to automatic relocation conversion in
klp-convert.c, changelog
v3:
- joe: convert to SPDX license tags
- jmoreira: add rela symbol name to WARNs
- jmoreira: ignore relocations to .TOC and symbols with index 0
- joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks
- joe: fix symbol use-after-frees
- joe: fix remaining valgrind leak complaints (non-error paths only)
- joe: checkpatch nits
livepatch: Add klp-convert annotation helpers
v2:
- jmoreira: split up: move KLP_MODULE_RELOC from previous patch to
here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to
include/linux/livepatch.h
v3:
- joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte
align klp_module_reloc structures
modpost: Integrate klp-convert
v2:
- khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule
doesn't do that.f
- khlebnikov: fix bashisms for debian where /bin/sh is a symlink to
/bin/dash
- khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise
arg-check inside if_changed_rule compares cmd_link_module and
cmd_ld_ko_o.
- khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is
true.
- mbenes: remove modinfo call. LIVEPATCH_ in Makefile
- jmoreira: split up: move the .livepatch file-based scheme for
identifying livepatches to a previous patch, as it was required for
correctly building Symbols.list there.
v3:
- joe: adjust rule_ld_ko_o to call echo-cmd
- joe: rebase for v5.1
- joe: align KLP prefix
modpost: Add modinfo flag to livepatch modules
v2:
- jmoreira: fix modpost.c (add_livepatch_flag) to update module
structure with livepatch flag and prevent modpost from breaking due to
unresolved symbols
v3:
- joe: adjust modpost.c::get_modinfo() call for v5.0 version
livepatch: Add sample livepatch module
v3:
- joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag
documentation: Update on livepatch elf format
v2:
- jmoreira: update module to use KLP_SYMPOS
- jmoreira: Comments on symbol resolution scheme
- jmoreira: Update Makefile
- jmoreira: Remove MODULE_INFO statement
- jmoreira: Changelog
v3:
- joe: rebase for v5.1
- joe: code shuffle to better match original source file
livepatch/selftests: add klp-convert
v3:
- joe: clarify sympos=0 and sympos=1..n
And now the usual git cover-letter boilerplate...
Joao Moreira (2):
kbuild: Support for Symbols.list creation
documentation: Update on livepatch elf format
Joe Lawrence (1):
livepatch/selftests: add klp-convert
Josh Poimboeuf (5):
livepatch: Create and include UAPI headers
livepatch: Add klp-convert tool
livepatch: Add klp-convert annotation helpers
modpost: Integrate klp-convert
livepatch: Add sample livepatch module
Miroslav Benes (1):
modpost: Add modinfo flag to livepatch modules
.gitignore | 1 +
Documentation/livepatch/livepatch.txt | 3 +
Documentation/livepatch/module-elf-format.txt | 50 +-
MAINTAINERS | 2 +
Makefile | 30 +-
include/linux/livepatch.h | 13 +
include/uapi/linux/livepatch.h | 22 +
kernel/livepatch/core.c | 4 +-
lib/livepatch/Makefile | 15 +
lib/livepatch/test_klp_atomic_replace.c | 1 -
lib/livepatch/test_klp_callbacks_demo.c | 1 -
lib/livepatch/test_klp_callbacks_demo2.c | 1 -
lib/livepatch/test_klp_convert1.c | 106 +++
lib/livepatch/test_klp_convert2.c | 103 +++
lib/livepatch/test_klp_convert_mod_a.c | 25 +
lib/livepatch/test_klp_convert_mod_b.c | 13 +
lib/livepatch/test_klp_livepatch.c | 1 -
samples/livepatch/Makefile | 7 +
.../livepatch/livepatch-annotated-sample.c | 102 +++
samples/livepatch/livepatch-sample.c | 1 -
scripts/Kbuild.include | 4 +-
scripts/Makefile | 1 +
scripts/Makefile.build | 7 +
scripts/Makefile.modpost | 24 +-
scripts/livepatch/.gitignore | 1 +
scripts/livepatch/Makefile | 7 +
scripts/livepatch/elf.c | 753 ++++++++++++++++++
scripts/livepatch/elf.h | 72 ++
scripts/livepatch/klp-convert.c | 692 ++++++++++++++++
scripts/livepatch/klp-convert.h | 39 +
scripts/livepatch/list.h | 391 +++++++++
scripts/mod/modpost.c | 82 +-
scripts/mod/modpost.h | 1 +
.../selftests/livepatch/test-livepatch.sh | 64 ++
34 files changed, 2616 insertions(+), 23 deletions(-)
create mode 100644 include/uapi/linux/livepatch.h
create mode 100644 lib/livepatch/test_klp_convert1.c
create mode 100644 lib/livepatch/test_klp_convert2.c
create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
create mode 100644 lib/livepatch/test_klp_convert_mod_b.c
create mode 100644 samples/livepatch/livepatch-annotated-sample.c
create mode 100644 scripts/livepatch/.gitignore
create mode 100644 scripts/livepatch/Makefile
create mode 100644 scripts/livepatch/elf.c
create mode 100644 scripts/livepatch/elf.h
create mode 100644 scripts/livepatch/klp-convert.c
create mode 100644 scripts/livepatch/klp-convert.h
create mode 100644 scripts/livepatch/list.h
--
2.20.1
From: Josh Poimboeuf <[email protected]>
Livepatches may use symbols which are not contained in its own scope,
and, because of that, may end up compiled with relocations that will
only be resolved during module load. Yet, when the referenced symbols
are not exported, solving this relocation requires information on the
object that holds the symbol (either vmlinux or modules) and its
position inside the object, as an object may contain multiple symbols
with the same name. Providing such information must be done
accordingly to what is specified in
Documentation/livepatch/module-elf-format.txt.
Currently, there is no trivial way to embed the required information
as requested in the final livepatch elf object. klp-convert solves
this problem in two different forms: (i) by relying on Symbols.list,
which is built during kernel compilation, to automatically infer the
relocation targeted symbol, and, when such inference is not possible
(ii) by using annotations in the elf object to convert the relocation
accordingly to the specification, enabling it to be handled by the
livepatch loader.
Given the above, create scripts/livepatch to hold tools developed for
livepatches and add source files for klp-convert there.
The core file of klp-convert is scripts/livepatch/klp-convert.c, which
implements the heuristics used to solve the relocations and the
conversion of unresolved symbols into the expected format, as defined
in [1].
klp-convert receives as arguments the Symbols.list file, an input
livepatch module to be converted and the output name for the converted
livepatch. When it starts running, klp-convert parses Symbols.list and
builds two internal lists of symbols, one containing the exported and
another containing the non-exported symbols. Then, by parsing the rela
sections in the elf object, klp-convert identifies which symbols must
be converted, which are those unresolved and that do not have a
corresponding exported symbol, and attempts to convert them
accordingly to the specification.
By using Symbols.list, klp-convert identifies which symbols have names
that only appear in a single kernel object, thus being capable of
resolving these cases without the intervention of the developer. When
various homonymous symbols exist through kernel objects, it is not
possible to infer the right one, thus klp-convert falls back into
using developer annotations. If these were not provided, then the tool
will print a list with all acceptable targets for the symbol being
processed.
Annotations in the context of klp-convert are accessible as struct
klp_module_reloc entries in sections named
.klp.module_relocs.<objname>. These entries are pairs of symbol
references and positions which are to be resolved against definitions
in <objname>.
Define the structure klp_module_reloc in
include/linux/uapi/livepatch.h allowing developers to annotate the
livepatch source code with it.
klp-convert relies on libelf and on a list implementation. Add files
scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a
libelf interfacing layer and scripts/livepatch/list.h, which is a
list implementation.
Update Makefiles to correctly support the compilation of the new tool,
update MAINTAINERS file and add a .gitignore file.
[1] - Documentation/livepatch/module-elf-format.txt
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Konstantin Khlebnikov <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
MAINTAINERS | 1 +
include/uapi/linux/livepatch.h | 5 +
scripts/Makefile | 1 +
scripts/livepatch/.gitignore | 1 +
scripts/livepatch/Makefile | 7 +
scripts/livepatch/elf.c | 753 ++++++++++++++++++++++++++++++++
scripts/livepatch/elf.h | 72 +++
scripts/livepatch/klp-convert.c | 692 +++++++++++++++++++++++++++++
scripts/livepatch/klp-convert.h | 39 ++
scripts/livepatch/list.h | 391 +++++++++++++++++
10 files changed, 1962 insertions(+)
create mode 100644 scripts/livepatch/.gitignore
create mode 100644 scripts/livepatch/Makefile
create mode 100644 scripts/livepatch/elf.c
create mode 100644 scripts/livepatch/elf.h
create mode 100644 scripts/livepatch/klp-convert.c
create mode 100644 scripts/livepatch/klp-convert.h
create mode 100644 scripts/livepatch/list.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 65b4e8f481f0..5f66ad2e54f4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9023,6 +9023,7 @@ F: arch/x86/kernel/livepatch.c
F: Documentation/livepatch/
F: Documentation/ABI/testing/sysfs-kernel-livepatch
F: samples/livepatch/
+F: scripts/livepatch/
F: tools/testing/selftests/livepatch/
L: [email protected]
T: git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git
diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
index bb86243de805..a50ff68bec12 100644
--- a/include/uapi/linux/livepatch.h
+++ b/include/uapi/linux/livepatch.h
@@ -14,4 +14,9 @@
#define KLP_RELA_PREFIX ".klp.rela."
#define KLP_SYM_PREFIX ".klp.sym."
+struct klp_module_reloc {
+ void *sym;
+ unsigned int sympos;
+} __attribute__((packed));
+
#endif /* _UAPI_LIVEPATCH_H */
diff --git a/scripts/Makefile b/scripts/Makefile
index 9d442ee050bd..bf9ce74b70b0 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -39,6 +39,7 @@ build_unifdef: $(obj)/unifdef
subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
subdir-$(CONFIG_MODVERSIONS) += genksyms
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
+subdir-$(CONFIG_LIVEPATCH) += livepatch
# Let clean descend into subdirs
subdir- += basic dtc gdb kconfig mod package
diff --git a/scripts/livepatch/.gitignore b/scripts/livepatch/.gitignore
new file mode 100644
index 000000000000..dc22fe4b6a5b
--- /dev/null
+++ b/scripts/livepatch/.gitignore
@@ -0,0 +1 @@
+klp-convert
diff --git a/scripts/livepatch/Makefile b/scripts/livepatch/Makefile
new file mode 100644
index 000000000000..2842ecdba3fd
--- /dev/null
+++ b/scripts/livepatch/Makefile
@@ -0,0 +1,7 @@
+hostprogs-y := klp-convert
+always := $(hostprogs-y)
+
+klp-convert-objs := klp-convert.o elf.o
+
+HOST_EXTRACFLAGS := -g -I$(INSTALL_HDR_PATH)/include -Wall
+HOSTLDLIBS_klp-convert := -lelf
diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
new file mode 100644
index 000000000000..11746bccecf2
--- /dev/null
+++ b/scripts/livepatch/elf.c
@@ -0,0 +1,753 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * elf.c - ELF access library
+ *
+ * Adapted from kpatch (https://github.com/dynup/kpatch):
+ * Copyright (C) 2013-2016 Josh Poimboeuf <[email protected]>
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "elf.h"
+
+#define WARN(format, ...) \
+ fprintf(stderr, "%s: " format "\n", elf->name, ##__VA_ARGS__)
+
+/*
+ * Fallback for systems without this "read, mmaping if possible" cmd.
+ */
+#ifndef ELF_C_READ_MMAP
+#define ELF_C_READ_MMAP ELF_C_READ
+#endif
+
+bool is_rela_section(struct section *sec)
+{
+ return (sec->sh.sh_type == SHT_RELA);
+}
+
+struct section *find_section_by_name(struct elf *elf, const char *name)
+{
+ struct section *sec;
+
+ list_for_each_entry(sec, &elf->sections, list)
+ if (!strcmp(sec->name, name))
+ return sec;
+
+ return NULL;
+}
+
+static struct section *find_section_by_index(struct elf *elf,
+ unsigned int idx)
+{
+ struct section *sec;
+
+ list_for_each_entry(sec, &elf->sections, list)
+ if (sec->idx == idx)
+ return sec;
+
+ return NULL;
+}
+
+static struct symbol *find_symbol_by_index(struct elf *elf, unsigned int idx)
+{
+ struct symbol *sym;
+
+ list_for_each_entry(sym, &elf->symbols, list)
+ if (sym->idx == idx)
+ return sym;
+
+ return NULL;
+}
+
+static int read_sections(struct elf *elf)
+{
+ Elf_Scn *s = NULL;
+ struct section *sec;
+ size_t shstrndx, sections_nr;
+ int i;
+
+ if (elf_getshdrnum(elf->elf, §ions_nr)) {
+ perror("elf_getshdrnum");
+ return -1;
+ }
+
+ if (elf_getshdrstrndx(elf->elf, &shstrndx)) {
+ perror("elf_getshdrstrndx");
+ return -1;
+ }
+
+ for (i = 0; i < sections_nr; i++) {
+ sec = malloc(sizeof(*sec));
+ if (!sec) {
+ perror("malloc");
+ return -1;
+ }
+ memset(sec, 0, sizeof(*sec));
+
+ INIT_LIST_HEAD(&sec->relas);
+
+ list_add_tail(&sec->list, &elf->sections);
+
+ s = elf_getscn(elf->elf, i);
+ if (!s) {
+ perror("elf_getscn");
+ return -1;
+ }
+
+ sec->idx = elf_ndxscn(s);
+
+ if (!gelf_getshdr(s, &sec->sh)) {
+ perror("gelf_getshdr");
+ return -1;
+ }
+
+ sec->name = elf_strptr(elf->elf, shstrndx, sec->sh.sh_name);
+ if (!sec->name) {
+ perror("elf_strptr");
+ return -1;
+ }
+
+ sec->elf_data = elf_getdata(s, NULL);
+ if (!sec->elf_data) {
+ perror("elf_getdata");
+ return -1;
+ }
+
+ if (sec->elf_data->d_off != 0 ||
+ sec->elf_data->d_size != sec->sh.sh_size) {
+ WARN("unexpected data attributes for %s", sec->name);
+ return -1;
+ }
+
+ sec->data = sec->elf_data->d_buf;
+ sec->size = sec->elf_data->d_size;
+ }
+
+ /* sanity check, one more call to elf_nextscn() should return NULL */
+ if (elf_nextscn(elf->elf, s)) {
+ WARN("section entry mismatch");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int read_symbols(struct elf *elf)
+{
+ struct section *symtab;
+ struct symbol *sym;
+ int symbols_nr, i;
+
+ symtab = find_section_by_name(elf, ".symtab");
+ if (!symtab) {
+ WARN("missing symbol table");
+ return -1;
+ }
+
+ symbols_nr = symtab->sh.sh_size / symtab->sh.sh_entsize;
+
+ for (i = 0; i < symbols_nr; i++) {
+ sym = malloc(sizeof(*sym));
+ if (!sym) {
+ perror("malloc");
+ return -1;
+ }
+ memset(sym, 0, sizeof(*sym));
+
+ sym->idx = i;
+
+ if (!gelf_getsym(symtab->elf_data, i, &sym->sym)) {
+ perror("gelf_getsym");
+ goto err;
+ }
+
+ sym->name = elf_strptr(elf->elf, symtab->sh.sh_link,
+ sym->sym.st_name);
+ if (!sym->name) {
+ perror("elf_strptr");
+ goto err;
+ }
+
+ sym->type = GELF_ST_TYPE(sym->sym.st_info);
+ sym->bind = GELF_ST_BIND(sym->sym.st_info);
+
+ if (sym->sym.st_shndx > SHN_UNDEF &&
+ sym->sym.st_shndx < SHN_LORESERVE) {
+ sym->sec = find_section_by_index(elf,
+ sym->sym.st_shndx);
+ if (!sym->sec) {
+ WARN("couldn't find section for symbol %s",
+ sym->name);
+ goto err;
+ }
+ if (sym->type == STT_SECTION) {
+ sym->name = sym->sec->name;
+ sym->sec->sym = sym;
+ }
+ }
+
+ sym->offset = sym->sym.st_value;
+ sym->size = sym->sym.st_size;
+
+ list_add_tail(&sym->list, &elf->symbols);
+ }
+
+ return 0;
+
+err:
+ free(sym);
+ return -1;
+}
+
+static int read_relas(struct elf *elf)
+{
+ struct section *sec;
+ struct rela *rela;
+ int i;
+ unsigned int symndx;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (sec->sh.sh_type != SHT_RELA)
+ continue;
+
+ sec->base = find_section_by_name(elf, sec->name + 5);
+ if (!sec->base) {
+ WARN("can't find base section for rela section %s",
+ sec->name);
+ return -1;
+ }
+
+ sec->base->rela = sec;
+
+ for (i = 0; i < sec->sh.sh_size / sec->sh.sh_entsize; i++) {
+ rela = malloc(sizeof(*rela));
+ if (!rela) {
+ perror("malloc");
+ return -1;
+ }
+ memset(rela, 0, sizeof(*rela));
+
+ if (!gelf_getrela(sec->elf_data, i, &rela->rela)) {
+ perror("gelf_getrela");
+ return -1;
+ }
+
+ rela->type = GELF_R_TYPE(rela->rela.r_info);
+ rela->addend = rela->rela.r_addend;
+ rela->offset = rela->rela.r_offset;
+ symndx = GELF_R_SYM(rela->rela.r_info);
+ rela->sym = find_symbol_by_index(elf, symndx);
+ if (!rela->sym) {
+ WARN("can't find rela entry symbol %d for %s",
+ symndx, sec->name);
+ return -1;
+ }
+
+ list_add_tail(&rela->list, &sec->relas);
+ }
+ }
+
+ return 0;
+}
+
+struct section *create_rela_section(struct elf *elf, const char *name,
+ struct section *base)
+{
+ struct section *sec;
+
+ sec = malloc(sizeof(*sec));
+ if (!sec) {
+ WARN("malloc failed");
+ return NULL;
+ }
+ memset(sec, 0, sizeof(*sec));
+ INIT_LIST_HEAD(&sec->relas);
+
+ sec->base = base;
+ sec->name = strdup(name);
+ if (!sec->name) {
+ WARN("strdup failed");
+ return NULL;
+ }
+ sec->sh.sh_name = -1;
+ sec->sh.sh_type = SHT_RELA;
+ sec->sh.sh_entsize = sizeof(GElf_Rela);
+ sec->sh.sh_addralign = 8;
+ sec->sh.sh_flags = SHF_ALLOC;
+
+ sec->elf_data = malloc(sizeof(*sec->elf_data));
+ if (!sec->elf_data) {
+ WARN("malloc failed");
+ return NULL;
+ }
+ memset(sec->elf_data, 0, sizeof(*sec->elf_data));
+ sec->elf_data->d_type = ELF_T_RELA;
+
+ list_add_tail(&sec->list, &elf->sections);
+
+ return sec;
+}
+
+static int update_shstrtab(struct elf *elf)
+{
+ struct section *shstrtab, *sec;
+ size_t orig_size, new_size = 0, offset, len;
+ char *buf;
+
+ shstrtab = find_section_by_name(elf, ".shstrtab");
+ if (!shstrtab) {
+ WARN("can't find .shstrtab");
+ return -1;
+ }
+
+ orig_size = new_size = shstrtab->size;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (sec->sh.sh_name != -1)
+ continue;
+ new_size += strlen(sec->name) + 1;
+ }
+
+ if (new_size == orig_size)
+ return 0;
+
+ buf = malloc(new_size);
+ if (!buf) {
+ WARN("malloc failed");
+ return -1;
+ }
+ memcpy(buf, (void *)shstrtab->data, orig_size);
+
+ offset = orig_size;
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (sec->sh.sh_name != -1)
+ continue;
+ sec->sh.sh_name = offset;
+ len = strlen(sec->name) + 1;
+ memcpy(buf + offset, sec->name, len);
+ offset += len;
+ }
+
+ shstrtab->elf_data->d_buf = shstrtab->data = buf;
+ shstrtab->elf_data->d_size = shstrtab->size = new_size;
+ shstrtab->sh.sh_size = new_size;
+
+ return 1;
+}
+
+static void free_shstrtab(struct elf *elf)
+{
+ struct section *shstrtab;
+
+ shstrtab = find_section_by_name(elf, ".shstrtab");
+ if (!shstrtab)
+ return;
+
+ free(shstrtab->elf_data->d_buf);
+}
+
+static int update_strtab(struct elf *elf)
+{
+ struct section *strtab;
+ struct symbol *sym;
+ size_t orig_size, new_size = 0, offset, len;
+ char *buf;
+
+ strtab = find_section_by_name(elf, ".strtab");
+ if (!strtab) {
+ WARN("can't find .strtab");
+ return -1;
+ }
+
+ orig_size = new_size = strtab->size;
+
+ list_for_each_entry(sym, &elf->symbols, list) {
+ if (sym->sym.st_name != -1)
+ continue;
+ new_size += strlen(sym->name) + 1;
+ }
+
+ if (new_size == orig_size)
+ return 0;
+
+ buf = malloc(new_size);
+ if (!buf) {
+ WARN("malloc failed");
+ return -1;
+ }
+ memcpy(buf, (void *)strtab->data, orig_size);
+
+ offset = orig_size;
+ list_for_each_entry(sym, &elf->symbols, list) {
+ if (sym->sym.st_name != -1)
+ continue;
+ sym->sym.st_name = offset;
+ len = strlen(sym->name) + 1;
+ memcpy(buf + offset, sym->name, len);
+ offset += len;
+ }
+
+ strtab->elf_data->d_buf = strtab->data = buf;
+ strtab->elf_data->d_size = strtab->size = new_size;
+ strtab->sh.sh_size = new_size;
+
+ return 1;
+}
+
+static void free_strtab(struct elf *elf)
+{
+ struct section *strtab;
+
+ strtab = find_section_by_name(elf, ".strtab");
+ if (!strtab)
+ return;
+
+ if (strtab->elf_data)
+ free(strtab->elf_data->d_buf);
+}
+
+static int update_symtab(struct elf *elf)
+{
+ struct section *symtab, *sec;
+ struct symbol *sym;
+ char *buf;
+ size_t size;
+ int offset = 0, nr_locals = 0, idx, nr_syms;
+
+ idx = 0;
+ list_for_each_entry(sec, &elf->sections, list)
+ sec->idx = idx++;
+
+ idx = 0;
+ list_for_each_entry(sym, &elf->symbols, list) {
+ sym->idx = idx++;
+ if (sym->sec)
+ sym->sym.st_shndx = sym->sec->idx;
+ }
+ nr_syms = idx;
+
+ symtab = find_section_by_name(elf, ".symtab");
+ if (!symtab) {
+ WARN("can't find symtab");
+ return -1;
+ }
+
+ symtab->sh.sh_link = find_section_by_name(elf, ".strtab")->idx;
+
+ /* create new symtab buffer */
+ size = nr_syms * symtab->sh.sh_entsize;
+ buf = malloc(size);
+ if (!buf) {
+ WARN("malloc failed");
+ return -1;
+ }
+ memset(buf, 0, size);
+
+ offset = 0;
+ list_for_each_entry(sym, &elf->symbols, list) {
+ memcpy(buf + offset, &sym->sym, symtab->sh.sh_entsize);
+ offset += symtab->sh.sh_entsize;
+
+ if (sym->bind == STB_LOCAL)
+ nr_locals++;
+ }
+
+ symtab->elf_data->d_buf = symtab->data = buf;
+ symtab->elf_data->d_size = symtab->size = size;
+ symtab->sh.sh_size = size;
+
+ /* update symtab section header */
+ symtab->sh.sh_info = nr_locals;
+
+ return 1;
+}
+
+static void free_symtab(struct elf *elf)
+{
+ struct section *symtab;
+
+ symtab = find_section_by_name(elf, ".symtab");
+ if (!symtab)
+ return;
+
+ free(symtab->elf_data->d_buf);
+}
+
+static int update_relas(struct elf *elf)
+{
+ struct section *sec, *symtab;
+ struct rela *rela;
+ int nr_relas, idx, size;
+ GElf_Rela *relas;
+
+ symtab = find_section_by_name(elf, ".symtab");
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (!is_rela_section(sec))
+ continue;
+
+ sec->sh.sh_link = symtab->idx;
+ if (sec->base)
+ sec->sh.sh_info = sec->base->idx;
+
+ nr_relas = 0;
+ list_for_each_entry(rela, &sec->relas, list)
+ nr_relas++;
+
+ size = nr_relas * sizeof(*relas);
+ relas = malloc(size);
+ if (!relas) {
+ WARN("malloc failed");
+ return -1;
+ }
+
+ sec->elf_data->d_buf = sec->data = relas;
+ sec->elf_data->d_size = sec->size = size;
+ sec->sh.sh_size = size;
+
+ idx = 0;
+ list_for_each_entry(rela, &sec->relas, list) {
+ relas[idx].r_offset = rela->offset;
+ relas[idx].r_addend = rela->addend;
+ relas[idx].r_info = GELF_R_INFO(rela->sym->idx,
+ rela->type);
+ idx++;
+ }
+ }
+
+ return 1;
+}
+
+static void free_relas(struct elf *elf)
+{
+ struct section *sec, *symtab;
+
+ symtab = find_section_by_name(elf, ".symtab");
+ if (!symtab)
+ return;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (!is_rela_section(sec))
+ continue;
+
+ free(sec->elf_data->d_buf);
+ }
+}
+
+static int write_file(struct elf *elf, const char *file)
+{
+ int fd;
+ Elf *e;
+ GElf_Ehdr eh, ehout;
+ Elf_Scn *scn;
+ Elf_Data *data;
+ GElf_Shdr sh;
+ struct section *sec;
+
+ fd = creat(file, 0664);
+ if (fd == -1) {
+ WARN("couldn't create %s", file);
+ return -1;
+ }
+
+ e = elf_begin(fd, ELF_C_WRITE, NULL);
+ if (!e) {
+ WARN("elf_begin failed");
+ return -1;
+ }
+
+ if (!gelf_newehdr(e, gelf_getclass(elf->elf))) {
+ WARN("gelf_newehdr failed");
+ return -1;
+ }
+
+ if (!gelf_getehdr(e, &ehout)) {
+ WARN("gelf_getehdr failed");
+ return -1;
+ }
+
+ if (!gelf_getehdr(elf->elf, &eh)) {
+ WARN("gelf_getehdr failed");
+ return -1;
+ }
+
+ memset(&ehout, 0, sizeof(ehout));
+ ehout.e_ident[EI_DATA] = eh.e_ident[EI_DATA];
+ ehout.e_machine = eh.e_machine;
+ ehout.e_type = eh.e_type;
+ ehout.e_version = EV_CURRENT;
+ ehout.e_shstrndx = find_section_by_name(elf, ".shstrtab")->idx;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (!sec->idx)
+ continue;
+ scn = elf_newscn(e);
+ if (!scn) {
+ WARN("elf_newscn failed");
+ return -1;
+ }
+
+ data = elf_newdata(scn);
+ if (!data) {
+ WARN("elf_newdata failed");
+ return -1;
+ }
+
+ if (!elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY)) {
+ WARN("elf_flagdata failed");
+ return -1;
+ }
+
+ data->d_type = sec->elf_data->d_type;
+ data->d_buf = sec->elf_data->d_buf;
+ data->d_size = sec->elf_data->d_size;
+
+ if (!gelf_getshdr(scn, &sh)) {
+ WARN("gelf_getshdr failed");
+ return -1;
+ }
+
+ sh = sec->sh;
+
+ if (!gelf_update_shdr(scn, &sh)) {
+ WARN("gelf_update_shdr failed");
+ return -1;
+ }
+ }
+
+ if (!gelf_update_ehdr(e, &ehout)) {
+ WARN("gelf_update_ehdr failed");
+ return -1;
+ }
+
+ if (elf_update(e, ELF_C_WRITE) < 0) {
+ fprintf(stderr, "%s\n", elf_errmsg(-1));
+ WARN("elf_update failed");
+ return -1;
+ }
+
+ elf_end(e);
+
+ return 0;
+}
+
+int elf_write_file(struct elf *elf, const char *file)
+{
+ int ret_shstrtab;
+ int ret_strtab;
+ int ret_symtab;
+ int ret_relas;
+ int ret;
+
+ ret_shstrtab = update_shstrtab(elf);
+ if (ret_shstrtab < 0)
+ return ret_shstrtab;
+
+ ret_strtab = update_strtab(elf);
+ if (ret_strtab < 0)
+ return ret_strtab;
+
+ ret_symtab = update_symtab(elf);
+ if (ret_symtab < 0)
+ return ret_symtab;
+
+ ret_relas = update_relas(elf);
+ if (ret_relas < 0)
+ return ret_relas;
+
+ ret = write_file(elf, file);
+ if (ret)
+ return ret;
+
+ if (ret_relas > 0)
+ free_relas(elf);
+ if (ret_symtab > 0)
+ free_symtab(elf);
+ if (ret_strtab > 0)
+ free_strtab(elf);
+ if (ret_shstrtab > 0)
+ free_shstrtab(elf);
+
+ return 0;
+}
+
+struct elf *elf_open(const char *name)
+{
+ struct elf *elf;
+
+ elf_version(EV_CURRENT);
+
+ elf = malloc(sizeof(*elf));
+ if (!elf) {
+ perror("malloc");
+ return NULL;
+ }
+ memset(elf, 0, sizeof(*elf));
+
+ INIT_LIST_HEAD(&elf->sections);
+ INIT_LIST_HEAD(&elf->symbols);
+
+ elf->fd = open(name, O_RDONLY);
+ if (elf->fd == -1) {
+ perror("open");
+ goto err;
+ }
+
+ elf->elf = elf_begin(elf->fd, ELF_C_READ_MMAP, NULL);
+ if (!elf->elf) {
+ perror("elf_begin");
+ goto err;
+ }
+
+ if (!gelf_getehdr(elf->elf, &elf->ehdr)) {
+ perror("gelf_getehdr");
+ goto err;
+ }
+
+ if (read_sections(elf))
+ goto err;
+
+ if (read_symbols(elf))
+ goto err;
+
+ if (read_relas(elf))
+ goto err;
+
+ return elf;
+
+err:
+ elf_close(elf);
+ return NULL;
+}
+
+void elf_close(struct elf *elf)
+{
+ struct section *sec, *tmpsec;
+ struct symbol *sym, *tmpsym;
+ struct rela *rela, *tmprela;
+
+ list_for_each_entry_safe(sym, tmpsym, &elf->symbols, list) {
+ list_del(&sym->list);
+ free(sym);
+ }
+ list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ list_del(&rela->list);
+ free(rela);
+ }
+ list_del(&sec->list);
+ free(sec);
+ }
+ if (elf->fd > 0)
+ close(elf->fd);
+ if (elf->elf)
+ elf_end(elf->elf);
+ free(elf);
+}
diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
new file mode 100644
index 000000000000..3b26dc28524f
--- /dev/null
+++ b/scripts/livepatch/elf.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2015-2016 Josh Poimboeuf <[email protected]>
+ */
+
+#ifndef _KLP_POST_ELF_H
+#define _KLP_POST_ELF_H
+
+#include <stdio.h>
+#include <stdbool.h>
+#include <gelf.h>
+#include "list.h"
+
+#ifdef LIBELF_USE_DEPRECATED
+# define elf_getshdrnum elf_getshnum
+# define elf_getshdrstrndx elf_getshstrndx
+#endif
+
+struct section {
+ struct list_head list;
+ GElf_Shdr sh;
+ struct section *base, *rela;
+ struct list_head relas;
+ struct symbol *sym;
+ Elf_Data *elf_data;
+ char *name;
+ int idx;
+ void *data;
+ unsigned int size;
+};
+
+struct symbol {
+ struct list_head list;
+ GElf_Sym sym;
+ struct section *sec;
+ char *name;
+ unsigned int idx;
+ unsigned char bind, type;
+ unsigned long offset;
+ unsigned int size;
+};
+
+struct rela {
+ struct list_head list;
+ GElf_Rela rela;
+ struct symbol *sym;
+ unsigned int type;
+ unsigned long offset;
+ int addend;
+};
+
+struct elf {
+ Elf *elf;
+ GElf_Ehdr ehdr;
+ int fd;
+ char *name;
+ struct list_head sections;
+ struct list_head symbols;
+};
+
+
+struct elf *elf_open(const char *name);
+bool is_rela_section(struct section *sec);
+struct section *find_section_by_name(struct elf *elf, const char *name);
+struct section *create_rela_section(struct elf *elf, const char *name,
+ struct section *base);
+
+void elf_close(struct elf *elf);
+int elf_write_file(struct elf *elf, const char *file);
+
+
+#endif /* _KLP_POST_ELF_H */
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
new file mode 100644
index 000000000000..62bd26941081
--- /dev/null
+++ b/scripts/livepatch/klp-convert.c
@@ -0,0 +1,692 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
+ * Copyright (C) 2017 Joao Moreira <[email protected]>
+ */
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include "elf.h"
+#include "list.h"
+#include "klp-convert.h"
+
+/*
+ * Symbols parsed from Symbols.list are kept in two lists:
+ * - symbols: keeps non-exported symbols
+ * - exp_symbols: keeps exported symbols (__ksymtab_prefixed)
+ */
+static LIST_HEAD(symbols);
+static LIST_HEAD(exp_symbols);
+
+/* In-livepatch user-provided symbol positions are kept in list usr_symbols */
+static LIST_HEAD(usr_symbols);
+
+static void free_syms_lists(void)
+{
+ struct symbol_entry *entry, *aux;
+ struct sympos *sp, *sp_aux;
+
+ list_for_each_entry_safe(entry, aux, &symbols, list) {
+ free(entry->object_name);
+ free(entry->symbol_name);
+ list_del(&entry->list);
+ free(entry);
+ }
+
+ list_for_each_entry_safe(entry, aux, &exp_symbols, list) {
+ free(entry->object_name);
+ free(entry->symbol_name);
+ list_del(&entry->list);
+ free(entry);
+ }
+
+ list_for_each_entry_safe(sp, sp_aux, &usr_symbols, list) {
+ free(sp->object_name);
+ free(sp->symbol_name);
+ list_del(&sp->list);
+ free(sp);
+ }
+}
+
+/* Parses file and fill symbols and exp_symbols list */
+static bool load_syms_lists(const char *symbols_list)
+{
+ FILE *fsyms;
+ struct symbol_entry *entry;
+ size_t len = 0;
+ ssize_t n;
+ char *obj = NULL, *sym = NULL;
+
+ fsyms = fopen(symbols_list, "r");
+ if (!fsyms) {
+ WARN("Unable to open Symbol list: %s", symbols_list);
+ return false;
+ }
+
+ /* read file format version */
+ n = getline(&sym, &len, fsyms);
+ if (n <= 0) {
+ WARN("Unable to read Symbol list: %s", symbols_list);
+ return false;
+ }
+
+ if (strncmp(sym, "klp-convert-symbol-data.0.1", 27) != 0) {
+ WARN("Symbol list is in unknown format.");
+ return false;
+ }
+
+ len = 0;
+ free(sym);
+ sym = NULL;
+
+ /* read file */
+ n = getline(&sym, &len, fsyms);
+ while (n > 0) {
+ if (sym[n-1] == '\n')
+ sym[n-1] = '\0';
+
+ /* Objects in Symbols.list are flagged with '*' */
+ if (sym[0] == '*') {
+ if (obj)
+ free(obj);
+ obj = strdup(sym+1);
+ if (!obj) {
+ WARN("Unable to allocate object name\n");
+ return false;
+ }
+ free(sym);
+ } else {
+ entry = calloc(1, sizeof(struct symbol_entry));
+ if (!entry) {
+ WARN("Unable to allocate Symbol entry\n");
+ return false;
+ }
+
+ entry->object_name = strdup(obj);
+ if (!entry->object_name) {
+ WARN("Unable to allocate entry object name\n");
+ return false;
+ }
+
+ entry->symbol_name = sym;
+ if (strncmp(entry->symbol_name, "__ksymtab_", 10) == 0)
+ list_add(&entry->list, &exp_symbols);
+ else
+ list_add(&entry->list, &symbols);
+ }
+ len = 0;
+ sym = NULL;
+ n = getline(&sym, &len, fsyms);
+ }
+ free(sym);
+ free(obj);
+ fclose(fsyms);
+ return true;
+}
+
+/* Searches for sympos of specific symbol in usr_symbols list */
+static bool get_usr_sympos(struct symbol *s, struct sympos *sp)
+{
+ struct sympos *aux;
+
+ list_for_each_entry(aux, &usr_symbols, list) {
+ if (strcmp(aux->symbol_name, s->name) == 0) {
+ sp->symbol_name = aux->symbol_name;
+ sp->object_name = aux->object_name;
+ sp->pos = aux->pos;
+ return true;
+ }
+ }
+ return false;
+}
+
+/* Removes symbols used for sympos annotation from livepatch elf object */
+static void clear_sympos_symbols(struct section *sec, struct elf *klp_elf)
+{
+ struct symbol *sym, *aux;
+
+ list_for_each_entry_safe(sym, aux, &klp_elf->symbols, list) {
+ if (sym->sec == sec) {
+
+ struct section *sec;
+ struct rela *rela, *tmprela;
+
+ list_for_each_entry(sec, &klp_elf->sections, list) {
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ if (rela->sym == sym) {
+ list_del(&rela->list);
+ free(rela);
+ }
+ }
+ }
+
+ list_del(&sym->list);
+ free(sym);
+ }
+ }
+}
+
+/* Removes annotation from livepatch elf object */
+static void clear_sympos_annontations(struct elf *klp_elf)
+{
+ struct section *sec, *aux;
+
+ list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+ if (strncmp(sec->name, ".klp.module_relocs.", 19) == 0) {
+ clear_sympos_symbols(sec, klp_elf);
+ list_del(&sec->list);
+ free(sec);
+ continue;
+ }
+ if (strncmp(sec->name, ".rela.klp.module_relocs.", 24) == 0) {
+
+ struct rela *rela, *tmprela;
+
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ list_del(&rela->list);
+ free(rela);
+ }
+ list_del(&sec->list);
+ free(sec);
+ continue;
+ }
+ }
+}
+
+/* Checks if two or more elements in usr_symbols have the same name */
+static bool sympos_sanity_check(void)
+{
+ bool sane = true;
+ struct sympos *sp, *aux;
+
+ list_for_each_entry(sp, &usr_symbols, list) {
+ aux = list_next_entry(sp, list);
+ list_for_each_entry_from(aux, &usr_symbols, list) {
+ if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+ WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
+ sp->object_name, sp->symbol_name, sp->pos,
+ aux->object_name, aux->symbol_name, aux->pos);
+ sane = false;
+ }
+ }
+ }
+ return sane;
+}
+
+/* Parses the livepatch elf object and fills usr_symbols */
+static bool load_usr_symbols(struct elf *klp_elf)
+{
+ char objname[MODULE_NAME_LEN];
+ struct sympos *sp;
+ struct section *sec, *aux, *relasec;
+ struct rela *rela;
+ struct klp_module_reloc *reloc;
+ int i, nr_entries;
+
+ list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+ if (sscanf(sec->name, ".klp.module_relocs.%55s", objname) != 1)
+ continue;
+
+ relasec = sec->rela;
+ reloc = sec->data;
+ i = 0;
+ nr_entries = sec->size / sizeof(*reloc);
+ list_for_each_entry(rela, &relasec->relas, list) {
+ if (i >= nr_entries) {
+ WARN("section %s length beyond nr_entries\n",
+ relasec->name);
+ return false;
+ }
+ sp = calloc(1, sizeof(struct sympos));
+ if (!sp) {
+ WARN("Unable to allocate sympos memory\n");
+ return false;
+ }
+ sp->object_name = strdup(objname);
+ if (!sp->object_name) {
+ WARN("Unable to allocate object name\n");
+ return false;
+ }
+ sp->symbol_name = strdup(rela->sym->name);
+ if (!sp->symbol_name) {
+ WARN("Unable to allocate symbol name\n");
+ return false;
+ }
+ sp->pos = reloc[i].sympos;
+ list_add(&sp->list, &usr_symbols);
+ i++;
+ }
+ if (i != nr_entries) {
+ WARN("nr_entries mismatch (%d != %d) for %s\n",
+ i, nr_entries, relasec->name);
+ return false;
+ }
+ }
+ clear_sympos_annontations(klp_elf);
+ return sympos_sanity_check();
+}
+
+/* prints list of valid sympos for symbol with provided name */
+static void print_valid_module_relocs(char *name)
+{
+ struct symbol_entry *e;
+ char *cur_obj = "";
+ int counter = 0;
+ bool first = true;
+
+ /* Symbols from the same object are locally gathered in the list */
+ fprintf(stderr, "Valid KLP_SYMPOS for symbol %s:\n", name);
+ fprintf(stderr, "-------------------------------------------------\n");
+ list_for_each_entry(e, &symbols, list) {
+ if (strcmp(e->object_name, cur_obj) != 0) {
+ cur_obj = e->object_name;
+ counter = 0;
+ }
+ if (strcmp(e->symbol_name, name) == 0) {
+ if (counter == 0) {
+ if (!first)
+ fprintf(stderr, "}\n");
+
+ fprintf(stderr, "KLP_MODULE_RELOC(%s){\n",
+ cur_obj);
+ first = false;
+ }
+ fprintf(stderr, "\tKLP_SYMPOS(%s,%d)\n", name, counter);
+ counter++;
+ }
+ }
+ fprintf(stderr, "-------------------------------------------------\n");
+}
+
+/*
+ * Searches for symbol in symbols list and returns its sympos if it is unique,
+ * otherwise prints a list with all considered valid sympos
+ */
+static struct symbol_entry *find_sym_entry_by_name(char *name)
+{
+ struct symbol_entry *found = NULL;
+ struct symbol_entry *e;
+
+ list_for_each_entry(e, &symbols, list) {
+ if (strcmp(e->symbol_name, name) == 0) {
+
+ /*
+ * If there exist multiple symbols with the same
+ * name then user-provided sympos is required
+ */
+ if (found) {
+ WARN("Define KLP_SYMPOS for the symbol: %s",
+ e->symbol_name);
+
+ print_valid_module_relocs(name);
+ return NULL;
+ }
+ found = e;
+ }
+ }
+ if (found)
+ return found;
+
+ return NULL;
+}
+
+/* Checks if sympos is valid, otherwise prints valid sympos list */
+static bool valid_sympos(struct sympos *sp)
+{
+ struct symbol_entry *e;
+
+ if (sp->pos == 0) {
+
+ /*
+ * sympos of 0 is reserved for uniquely named obj:sym,
+ * verify that this is the case
+ */
+ int counter = 0;
+
+ list_for_each_entry(e, &symbols, list) {
+ if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
+ (strcmp(e->object_name, sp->object_name) == 0)) {
+ counter++;
+ }
+ }
+ if (counter == 1)
+ return true;
+
+ WARN("Provided KLP_SYMPOS of 0, but found %d symbols matching: %s.%s,%d",
+ counter, sp->object_name, sp->symbol_name,
+ sp->pos);
+
+ } else {
+
+ /*
+ * sympos > 0 indicates a specific commonly-named obj:sym,
+ * indexing starts with 1
+ */
+ int index = 1;
+
+ list_for_each_entry(e, &symbols, list) {
+ if ((strcmp(e->symbol_name, sp->symbol_name) == 0) &&
+ (strcmp(e->object_name, sp->object_name) == 0)) {
+ if (index == sp->pos)
+ return true;
+ index++;
+ }
+ }
+
+ WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d",
+ sp->object_name, sp->symbol_name, sp->pos);
+ }
+
+ print_valid_module_relocs(sp->symbol_name);
+
+ return false;
+}
+
+/* Returns the right sympos respective to a symbol to be relocated */
+static bool find_missing_position(struct symbol *s, struct sympos *sp)
+{
+ struct symbol_entry *entry;
+
+ if (get_usr_sympos(s, sp)) {
+ if (valid_sympos(sp))
+ return true;
+ return false;
+ }
+
+ /* if no user-provided sympos, search symbol in symbols list */
+ entry = find_sym_entry_by_name(s->name);
+ if (entry) {
+ sp->symbol_name = entry->symbol_name;
+ sp->object_name = entry->object_name;
+ sp->pos = 0;
+ return true;
+ }
+ return false;
+}
+
+/*
+ * Finds or creates a klp rela section based on another given section (@oldsec)
+ * and sympos (@*sp), then returns it
+ */
+static struct section *get_or_create_klp_rela_section(struct section *oldsec,
+ struct sympos *sp, struct elf *klp_elf)
+{
+ char *name;
+ struct section *sec;
+ unsigned int length;
+
+ length = strlen(KLP_RELA_PREFIX) + strlen(sp->object_name)
+ + strlen(oldsec->base->name) + 2;
+
+ name = calloc(1, length);
+ if (!name) {
+ WARN("Memory allocation failed (%s%s.%s)\n", KLP_RELA_PREFIX,
+ sp->object_name, oldsec->base->name);
+ return NULL;
+ }
+
+ if (snprintf(name, length, KLP_RELA_PREFIX "%s.%s", sp->object_name,
+ oldsec->base->name) >= length) {
+ WARN("Length error (%s)", name);
+ free(name);
+ return NULL;
+ }
+
+ sec = find_section_by_name(klp_elf, name);
+ if (!sec)
+ sec = create_rela_section(klp_elf, name, oldsec->base);
+
+ if (sec)
+ sec->sh.sh_flags |= SHF_RELA_LIVEPATCH;
+
+ free(name);
+ return sec;
+}
+
+/* Converts rela symbol names */
+static bool convert_klp_symbol(struct symbol *s, struct sympos *sp)
+{
+ char *name;
+ char pos[4]; /* assume that pos will never be > 999 */
+ unsigned int length;
+
+ if (snprintf(pos, sizeof(pos), "%d", sp->pos) > sizeof(pos)) {
+ WARN("Insuficient buffer for expanding sympos (%s.%s,%d)\n",
+ sp->object_name, sp->symbol_name, sp->pos);
+ return false;
+ }
+
+ length = strlen(KLP_SYM_PREFIX) + strlen(sp->object_name)
+ + strlen(sp->symbol_name) + sizeof(pos) + 3;
+
+ name = calloc(1, length);
+ if (!name) {
+ WARN("Memory allocation failed (%s%s.%s,%s)\n", KLP_SYM_PREFIX,
+ sp->object_name, sp->symbol_name, pos);
+ return false;
+ }
+
+ if (snprintf(name, length, KLP_SYM_PREFIX "%s.%s,%s", sp->object_name,
+ sp->symbol_name, pos) >= length) {
+
+ WARN("Length error (%s%s.%s,%s)", KLP_SYM_PREFIX,
+ sp->object_name, sp->symbol_name, pos);
+
+ return false;
+ }
+
+ /*
+ * Despite the memory waste, we don't mind freeing the original symbol
+ * name memory chunk. Keeping it there is harmless and, since removing
+ * bytes from the string section is non-trivial, it is unworthy.
+ */
+ s->name = name;
+ s->sec = NULL;
+ s->sym.st_name = -1;
+ s->sym.st_shndx = SHN_LIVEPATCH;
+
+ return true;
+}
+
+/*
+ * Convert rela that cannot be resolved by the clasic module loader
+ * to the special klp rela one.
+ */
+static bool convert_rela(struct section *oldsec, struct rela *r,
+ struct sympos *sp, struct elf *klp_elf)
+{
+ struct section *sec;
+ struct rela *r1, *r2;
+
+ sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
+ if (!sec) {
+ WARN("Can't create or access klp.rela section (%s.%s)\n",
+ sp->object_name, sp->symbol_name);
+ return false;
+ }
+
+ if (!convert_klp_symbol(r->sym, sp)) {
+ WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
+ r->sym->name);
+ return false;
+ }
+
+ /* Move the converted rela to klp rela section */
+ list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
+ if (r1->sym->name == r->sym->name) {
+ list_del(&r1->list);
+ list_add(&r1->list, &sec->relas);
+ }
+ }
+ return true;
+}
+
+/* Checks if given symbol name matches a symbol in exp_symbols */
+static bool is_exported(char *sname)
+{
+ struct symbol_entry *e;
+
+ /*
+ * exp_symbols itens are prefixed with __ksymtab_ - comparisons must
+ * skip prefix and check if both are properly null-terminated
+ */
+ list_for_each_entry(e, &exp_symbols, list) {
+ if (strcmp(e->symbol_name + 10, sname) == 0)
+ return true;
+ }
+ return false;
+}
+
+/* Checks if a symbol was previously klp-converted based on its name */
+static bool is_converted(char *sname)
+{
+ int len = strlen(KLP_SYM_PREFIX);
+
+ if (strncmp(sname, KLP_SYM_PREFIX, len) == 0)
+ return true;
+ return false;
+}
+
+/*
+ * Checks if symbol must be converted (conditions):
+ * not resolved, not already converted or isn't an exported symbol
+ */
+static bool must_convert(struct symbol *sym)
+{
+ /* already resolved? */
+ if (sym->sec)
+ return false;
+
+ /* skip symbol with index 0 */
+ if (!sym->idx)
+ return false;
+
+ /* we should not touch .TOC. on ppc64le */
+ if (strcmp(sym->name, ".TOC.") == 0)
+ return false;
+
+ return (!(is_converted(sym->name) || is_exported(sym->name)));
+}
+
+/* Checks if a section is a klp rela section */
+static bool is_klp_rela_section(char *sname)
+{
+ int len = strlen(KLP_RELA_PREFIX);
+
+ if (strncmp(sname, KLP_RELA_PREFIX, len) == 0)
+ return true;
+ return false;
+}
+
+/*
+ * Frees the new names and rela sections as created by convert_rela()
+ */
+static void free_converted_resources(struct elf *klp_elf)
+{
+ struct symbol *sym;
+ struct section *sec;
+
+ list_for_each_entry(sym, &klp_elf->symbols, list) {
+ if (sym->name && is_converted(sym->name)) {
+ free(sym->name);
+ }
+ }
+
+ list_for_each_entry(sec, &klp_elf->sections, list) {
+ if (is_klp_rela_section(sec->name)) {
+ free(sec->elf_data);
+ free(sec->name);
+ }
+ }
+}
+
+int main(int argc, const char **argv)
+{
+ const char *klp_in_module, *klp_out_module, *symbols_list;
+ struct rela *rela, *tmprela;
+ struct section *sec, *aux;
+ struct sympos sp;
+ struct elf *klp_elf;
+
+ if (argc != 4) {
+ WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
+ return -1;
+ }
+
+ symbols_list = argv[1];
+ klp_in_module = argv[2];
+ klp_out_module = argv[3];
+
+ klp_elf = elf_open(klp_in_module);
+ if (!klp_elf) {
+ WARN("Unable to read elf file %s\n", klp_in_module);
+ return -1;
+ }
+
+ if (!load_syms_lists(symbols_list))
+ return -1;
+
+ if (!load_usr_symbols(klp_elf)) {
+ WARN("Unable to load user-provided sympos");
+ return -1;
+ }
+
+ list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+ if (!is_rela_section(sec))
+ continue;
+
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ if (!must_convert(rela->sym))
+ continue;
+
+ if (!find_missing_position(rela->sym, &sp)) {
+ WARN("Unable to find missing symbol: %s",
+ rela->sym->name);
+ return -1;
+ }
+ if (!convert_rela(sec, rela, &sp, klp_elf)) {
+ WARN("Unable to convert relocation: %s",
+ rela->sym->name);
+ return -1;
+ }
+ }
+ }
+
+ free_syms_lists();
+ if (elf_write_file(klp_elf, klp_out_module))
+ return -1;
+
+ free_converted_resources(klp_elf);
+ elf_close(klp_elf);
+
+ return 0;
+}
+
+/* Functions kept commented since they might be useful for future debugging */
+
+/* Dumps sympos list (useful for debugging purposes)
+ * static void dump_sympos(void)
+ * {
+ * struct sympos *sp;
+ *
+ * fprintf(stderr, "BEGIN OF SYMPOS DUMP\n");
+ * list_for_each_entry(sp, &usr_symbols, list) {
+ * fprintf(stderr, "%s %s %d\n", sp->symbol_name, sp->object_name,
+ * sp->pos);
+ * }
+ * fprintf(stderr, "END OF SYMPOS DUMP\n");
+ * }
+ *
+ *
+ * / Dump symbols list for debugging purposes /
+ * static void dump_symbols(void)
+ * {
+ * struct symbol_entry *entry;
+ *
+ * fprintf(stderr, "BEGIN OF SYMBOLS DUMP\n");
+ * list_for_each_entry(entry, &symbols, list)
+ * printf("%s %s\n", entry->object_name, entry->symbol_name);
+ * fprintf(stderr, "END OF SYMBOLS DUMP\n");
+ * }
+ */
diff --git a/scripts/livepatch/klp-convert.h b/scripts/livepatch/klp-convert.h
new file mode 100644
index 000000000000..fb92b1ea4b52
--- /dev/null
+++ b/scripts/livepatch/klp-convert.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
+ * Copyright (C) 2017 Joao Moreira <[email protected]>
+ *
+ */
+
+#define SHN_LIVEPATCH 0xff20
+#define SHF_RELA_LIVEPATCH 0x00100000
+#define MODULE_NAME_LEN (64 - sizeof(GElf_Addr))
+#define WARN(format, ...) \
+ fprintf(stderr, "klp-convert: " format "\n", ##__VA_ARGS__)
+
+/*
+ * klp-convert uses macros defined in the linux sources package. To prevent the
+ * dependency when building locally, they are defined below. Also notice that
+ * these should match the definitions from the targeted kernel.
+ */
+
+#define KLP_RELA_PREFIX ".klp.rela."
+#define KLP_SYM_PREFIX ".klp.sym."
+
+struct symbol_entry {
+ struct list_head list;
+ char *symbol_name;
+ char *object_name;
+};
+
+struct sympos {
+ struct list_head list;
+ char *symbol_name;
+ char *object_name;
+ int pos;
+};
+
+struct klp_module_reloc {
+ void *sym;
+ unsigned int sympos;
+} __attribute__((packed));
diff --git a/scripts/livepatch/list.h b/scripts/livepatch/list.h
new file mode 100644
index 000000000000..4d429120fabf
--- /dev/null
+++ b/scripts/livepatch/list.h
@@ -0,0 +1,391 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_LIST_H
+#define _LINUX_LIST_H
+
+/*
+ * Simple doubly linked list implementation.
+ *
+ * Some of the internal functions ("__xxx") are useful when
+ * manipulating whole lists rather than single entries, as
+ * sometimes we already know the next/prev entries and we can
+ * generate better code by using them directly rather than
+ * using the generic single-entry routines.
+ */
+
+#define WRITE_ONCE(a, b) (a = b)
+#define READ_ONCE(a) a
+
+#undef offsetof
+#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr: the pointer to the member.
+ * @type: the type of the container struct this is embedded in.
+ * @member: the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({ \
+ const typeof(((type *)0)->member) * __mptr = (ptr); \
+ (type *)((char *)__mptr - offsetof(type, member)); })
+
+struct list_head {
+ struct list_head *next, *prev;
+};
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+ struct list_head name = LIST_HEAD_INIT(name)
+
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+ WRITE_ONCE(list->next, list);
+ list->prev = list;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *new,
+ struct list_head *prev,
+ struct list_head *next)
+{
+ next->prev = new;
+ new->next = next;
+ new->prev = prev;
+ WRITE_ONCE(prev->next, new);
+}
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+ __list_add(new, head, head->next);
+}
+
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+ __list_add(new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head *prev, struct list_head *next)
+{
+ next->prev = prev;
+ WRITE_ONCE(prev->next, next);
+}
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void __list_del_entry(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+}
+
+static inline void list_del(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+}
+
+/**
+ * list_is_last - tests whether @list is the last entry in list @head
+ * @list: the entry to test
+ * @head: the head of the list
+ */
+static inline int list_is_last(const struct list_head *list,
+ const struct list_head *head)
+{
+ return list->next == head;
+}
+
+/**
+ * list_empty - tests whether a list is empty
+ * @head: the list to test.
+ */
+static inline int list_empty(const struct list_head *head)
+{
+ return READ_ONCE(head->next) == head;
+}
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr: the &struct list_head pointer.
+ * @type: the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ */
+#define list_entry(ptr, type, member) \
+ container_of(ptr, type, member)
+
+/**
+ * list_first_entry - get the first element from a list
+ * @ptr: the list head to take the element from.
+ * @type: the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_first_entry(ptr, type, member) \
+ list_entry((ptr)->next, type, member)
+
+/**
+ * list_last_entry - get the last element from a list
+ * @ptr: the list head to take the element from.
+ * @type: the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_last_entry(ptr, type, member) \
+ list_entry((ptr)->prev, type, member)
+
+/**
+ * list_first_entry_or_null - get the first element from a list
+ * @ptr: the list head to take the element from.
+ * @type: the type of the struct this is embedded in.
+ * @member: the name of the list_head within the struct.
+ *
+ * Note that if the list is empty, it returns NULL.
+ */
+#define list_first_entry_or_null(ptr, type, member) \
+ (!list_empty(ptr) ? list_first_entry(ptr, type, member) : NULL)
+
+/**
+ * list_next_entry - get the next element in list
+ * @pos: the type * to cursor
+ * @member: the name of the list_head within the struct.
+ */
+#define list_next_entry(pos, member) \
+ list_entry((pos)->member.next, typeof(*(pos)), member)
+
+/**
+ * list_prev_entry - get the prev element in list
+ * @pos: the type * to cursor
+ * @member: the name of the list_head within the struct.
+ */
+#define list_prev_entry(pos, member) \
+ list_entry((pos)->member.prev, typeof(*(pos)), member)
+
+/**
+ * list_for_each - iterate over a list
+ * @pos: the &struct list_head to use as a loop cursor.
+ * @head: the head for your list.
+ */
+#define list_for_each(pos, head) \
+ for (pos = (head)->next; pos != (head); pos = pos->next)
+
+/**
+ * list_for_each_prev - iterate over a list backwards
+ * @pos: the &struct list_head to use as a loop cursor.
+ * @head: the head for your list.
+ */
+#define list_for_each_prev(pos, head) \
+ for (pos = (head)->prev; pos != (head); pos = pos->prev)
+
+/**
+ * list_for_each_safe - iterate over a list safe against removal of list entry
+ * @pos: the &struct list_head to use as a loop cursor.
+ * @n: another &struct list_head to use as temporary storage
+ * @head: the head for your list.
+ */
+#define list_for_each_safe(pos, n, head) \
+ for (pos = (head)->next, n = pos->next; pos != (head); \
+ pos = n, n = pos->next)
+
+/**
+ * list_for_each_prev_safe - iterate over a list backwards safe against removal
+ of list entry
+ * @pos: the &struct list_head to use as a loop cursor.
+ * @n: another &struct list_head to use as temporary storage
+ * @head: the head for your list.
+ */
+#define list_for_each_prev_safe(pos, n, head) \
+ for (pos = (head)->prev, n = pos->prev; \
+ pos != (head); \
+ pos = n, n = pos->prev)
+
+/**
+ * list_for_each_entry - iterate over list of given type
+ * @pos: the type * to use as a loop cursor.
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ */
+#define list_for_each_entry(pos, head, member) \
+ for (pos = list_first_entry(head, typeof(*pos), member); \
+ &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_reverse - iterate backwards over list of given type.
+ * @pos: the type * to use as a loop cursor.
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ */
+#define list_for_each_entry_reverse(pos, head, member) \
+ for (pos = list_last_entry(head, typeof(*pos), member); \
+ &pos->member != (head); \
+ pos = list_prev_entry(pos, member))
+
+/**
+ * list_prepare_entry - prepare a pos entry for use in
+ list_for_each_entry_continue()
+ * @pos: the type * to use as a start point
+ * @head: the head of the list
+ * @member: the name of the list_head within the struct.
+ *
+ * Prepares a pos entry for use as a start point in
+ list_for_each_entry_continue().
+ */
+#define list_prepare_entry(pos, head, member) \
+ ((pos) ? : list_entry(head, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_continue - continue iteration over list of given type
+ * @pos: the type * to use as a loop cursor.
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Continue to iterate over list of given type, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue(pos, head, member) \
+ for (pos = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_continue_reverse - iterate backwards from the given point
+ * @pos: the type * to use as a loop cursor.
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Start to iterate over list of given type backwards, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue_reverse(pos, head, member) \
+ for (pos = list_prev_entry(pos, member); \
+ &pos->member != (head); \
+ pos = list_prev_entry(pos, member))
+
+/**
+ * list_for_each_entry_from - iterate over list of given type from the current
+ point
+ * @pos: the type * to use as a loop cursor.
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Iterate over list of given type, continuing from current position.
+ */
+#define list_for_each_entry_from(pos, head, member) \
+ for (; &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_safe - iterate over list of given type safe against
+ removal of list entry
+ * @pos: the type * to use as a loop cursor.
+ * @n: another type * to use as temporary storage
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ */
+#define list_for_each_entry_safe(pos, n, head, member) \
+ for (pos = list_first_entry(head, typeof(*pos), member), \
+ n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_continue - continue list iteration safe against
+ * removal
+ * @pos: the type * to use as a loop cursor.
+ * @n: another type * to use as temporary storage
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Iterate over list of given type, continuing after current point,
+ * safe against removal of list entry.
+ */
+#define list_for_each_entry_safe_continue(pos, n, head, member) \
+ for (pos = list_next_entry(pos, member), \
+ n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_from - iterate over list from current point safe
+ * against removal
+ * @pos: the type * to use as a loop cursor.
+ * @n: another type * to use as temporary storage
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Iterate over list of given type from current point, safe against
+ * removal of list entry.
+ */
+#define list_for_each_entry_safe_from(pos, n, head, member) \
+ for (n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+/**
+ * list_for_each_entry_safe_reverse - iterate backwards over list safe against
+ * removal
+ * @pos: the type * to use as a loop cursor.
+ * @n: another type * to use as temporary storage
+ * @head: the head for your list.
+ * @member: the name of the list_head within the struct.
+ *
+ * Iterate backwards over list of given type, safe against removal
+ * of list entry.
+ */
+#define list_for_each_entry_safe_reverse(pos, n, head, member) \
+ for (pos = list_last_entry(head, typeof(*pos), member), \
+ n = list_prev_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_prev_entry(n, member))
+
+/**
+ * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
+ * @pos: the loop cursor used in the list_for_each_entry_safe loop
+ * @n: temporary storage used in list_for_each_entry_safe
+ * @member: the name of the list_head within the struct.
+ *
+ * list_safe_reset_next is not safe to use in general if the list may be
+ * modified concurrently (eg. the lock is dropped in the loop body). An
+ * exception to this is if the cursor element (pos) is pinned in the list,
+ * and list_safe_reset_next is called after re-taking the lock and before
+ * completing the current iteration of the loop body.
+ */
+#define list_safe_reset_next(pos, n, member) \
+ (n = list_next_entry(pos, member))
+
+#endif
--
2.20.1
From: Josh Poimboeuf <[email protected]>
Add a new livepatch sample in samples/livepatch/ to make use of
symbols that must be post-processed to enable load-time relocation
resolution. As the new sample is to be used as an example, it is
annotated with KLP_MODULE_RELOC and with KLP_SYMPOS macros.
The livepatch sample updates the function cmdline_proc_show to
print the string referenced by the symbol saved_command_line
appended by the string "livepatch=1".
Update livepatch-sample.c to remove livepatch MODULE_INFO
statement.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
samples/livepatch/Makefile | 2 +
.../livepatch/livepatch-annotated-sample.c | 102 ++++++++++++++++++
2 files changed, 104 insertions(+)
create mode 100644 samples/livepatch/livepatch-annotated-sample.c
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 5fb3280bbdc4..dea530840725 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -2,6 +2,7 @@ LIVEPATCH_livepatch-sample := y
LIVEPATCH_livepatch-shadow-fix1 := y
LIVEPATCH_livepatch-shadow-fix2 := y
LIVEPATCH_livepatch-callbacks-demo := y
+LIVEPATCH_livepatch-annotated-sample := y
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
@@ -10,3 +11,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-annotated-sample.o
diff --git a/samples/livepatch/livepatch-annotated-sample.c b/samples/livepatch/livepatch-annotated-sample.c
new file mode 100644
index 000000000000..556ce7e0bdab
--- /dev/null
+++ b/samples/livepatch/livepatch-annotated-sample.c
@@ -0,0 +1,102 @@
+/*
+ * livepatch-annotated-sample.c - Kernel Live Patching Sample Module
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/*
+ * This (dumb) live patch overrides the function that prints the
+ * kernel boot cmdline when /proc/cmdline is read.
+ *
+ * This livepatch uses the symbol saved_command_line whose relocation
+ * must be resolved during load time. To enable that, this module
+ * must be post-processed by a tool called klp-convert, which embeds
+ * information to be used by the loader to solve the relocation.
+ *
+ * The module is annotated with KLP_MODULE_RELOC/KLP_SYMPOS macros.
+ * These annotations are used by klp-convert to infer that the symbol
+ * saved_command_line is in the object vmlinux.
+ *
+ * As saved_command_line has no other homonimous symbol across
+ * kernel objects, this annotation is not a requirement, and can be
+ * suppressed with no harm to klp-convert. Yet, it is kept here as an
+ * example on how to annotate livepatch modules that contain symbols
+ * whose names are used in more than one kernel object.
+ *
+ * Example:
+ *
+ * $ cat /proc/cmdline
+ * <your cmdline>
+ *
+ * $ insmod livepatch-sample.ko
+ * $ cat /proc/cmdline
+ * <your cmdline> livepatch=1
+ *
+ * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
+ * $ cat /proc/cmdline
+ * <your cmdline>
+ */
+
+extern char *saved_command_line;
+
+#include <linux/seq_file.h>
+static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
+{
+ seq_printf(m, "%s livepatch=1\n", saved_command_line);
+ return 0;
+}
+
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+ KLP_SYMPOS(saved_command_line, 0)
+};
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "cmdline_proc_show",
+ .new_func = livepatch_cmdline_proc_show,
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int livepatch_init(void)
+{
+ return klp_enable_patch(&patch);
+}
+
+static void livepatch_exit(void)
+{
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_LICENSE("GPL");
--
2.20.1
From: Miroslav Benes <[email protected]>
Currently, livepatch infrastructure in the kernel relies on
MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
kernel module loader knows a module is indeed livepatch module and can
behave accordingly.
klp-convert, on the other hand relies on LIVEPATCH_* statement in the
module's Makefile for exactly the same reason.
Remove dependency on modinfo and generate MODULE_INFO flag
automatically in modpost when LIVEPATCH_* is defined in the module's
Makefile. Generate a list of all built livepatch modules based on
the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
this list as an argument for modpost which will use it to identify
livepatch modules.
As MODULE_INFO is no longer needed, remove it.
Signed-off-by: Miroslav Benes <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
lib/livepatch/Makefile | 5 ++
lib/livepatch/test_klp_atomic_replace.c | 1 -
lib/livepatch/test_klp_callbacks_demo.c | 1 -
lib/livepatch/test_klp_callbacks_demo2.c | 1 -
lib/livepatch/test_klp_livepatch.c | 1 -
samples/livepatch/Makefile | 4 ++
samples/livepatch/livepatch-sample.c | 1 -
scripts/Makefile.modpost | 8 ++-
scripts/mod/modpost.c | 84 ++++++++++++++++++++++--
9 files changed, 94 insertions(+), 12 deletions(-)
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 26900ddaef82..513d200b7942 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -2,6 +2,11 @@
#
# Makefile for livepatch test code.
+LIVEPATCH_test_klp_atomic_replace := y
+LIVEPATCH_test_klp_callbacks_demo := y
+LIVEPATCH_test_klp_callbacks_demo2 := y
+LIVEPATCH_test_klp_livepatch := y
+
obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
test_klp_callbacks_demo.o \
test_klp_callbacks_demo2.o \
diff --git a/lib/livepatch/test_klp_atomic_replace.c b/lib/livepatch/test_klp_atomic_replace.c
index 5af7093ca00c..3bf08a1b7b12 100644
--- a/lib/livepatch/test_klp_atomic_replace.c
+++ b/lib/livepatch/test_klp_atomic_replace.c
@@ -52,6 +52,5 @@ static void test_klp_atomic_replace_exit(void)
module_init(test_klp_atomic_replace_init);
module_exit(test_klp_atomic_replace_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence <[email protected]>");
MODULE_DESCRIPTION("Livepatch test: atomic replace");
diff --git a/lib/livepatch/test_klp_callbacks_demo.c b/lib/livepatch/test_klp_callbacks_demo.c
index 3fd8fe1cd1cc..76e2f51a6771 100644
--- a/lib/livepatch/test_klp_callbacks_demo.c
+++ b/lib/livepatch/test_klp_callbacks_demo.c
@@ -116,6 +116,5 @@ static void test_klp_callbacks_demo_exit(void)
module_init(test_klp_callbacks_demo_init);
module_exit(test_klp_callbacks_demo_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence <[email protected]>");
MODULE_DESCRIPTION("Livepatch test: livepatch demo");
diff --git a/lib/livepatch/test_klp_callbacks_demo2.c b/lib/livepatch/test_klp_callbacks_demo2.c
index 5417573e80af..76db98fc3071 100644
--- a/lib/livepatch/test_klp_callbacks_demo2.c
+++ b/lib/livepatch/test_klp_callbacks_demo2.c
@@ -88,6 +88,5 @@ static void test_klp_callbacks_demo2_exit(void)
module_init(test_klp_callbacks_demo2_init);
module_exit(test_klp_callbacks_demo2_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence <[email protected]>");
MODULE_DESCRIPTION("Livepatch test: livepatch demo2");
diff --git a/lib/livepatch/test_klp_livepatch.c b/lib/livepatch/test_klp_livepatch.c
index aff08199de71..d94d0ae232db 100644
--- a/lib/livepatch/test_klp_livepatch.c
+++ b/lib/livepatch/test_klp_livepatch.c
@@ -46,6 +46,5 @@ static void test_klp_livepatch_exit(void)
module_init(test_klp_livepatch_init);
module_exit(test_klp_livepatch_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Seth Jennings <[email protected]>");
MODULE_DESCRIPTION("Livepatch test: livepatch module");
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 8b9b42a258ad..5fb3280bbdc4 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1,4 +1,8 @@
LIVEPATCH_livepatch-sample := y
+LIVEPATCH_livepatch-shadow-fix1 := y
+LIVEPATCH_livepatch-shadow-fix2 := y
+LIVEPATCH_livepatch-callbacks-demo := y
+
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index 01c9cf003ca2..8900a175046b 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -79,4 +79,3 @@ static void livepatch_exit(void)
module_init(livepatch_init);
module_exit(livepatch_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 1e8bb7442689..9fe4c5760aca 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -65,6 +65,11 @@ MODLISTCMD := find $(MODVERDIR) -name '*.mod' | xargs -r grep -h '\.ko$$' | sort
__modules := $(shell $(MODLISTCMD))
modules := $(patsubst %.o,%.ko, $(wildcard $(__modules:.ko=.o)))
+# find all .livepatch files listed in $(MODVERDIR)/
+ifdef CONFIG_LIVEPATCH
+$(shell find $(MODVERDIR) -name '*.livepatch' | xargs -r -I{} basename {} .livepatch > $(MODVERDIR)/livepatchmods)
+endif
+
# Stop after building .o files if NOFINAL is set. Makes compile tests quicker
_modpost: $(if $(KBUILD_MODPOST_NOFINAL), $(modules:.ko:.o),$(modules))
@@ -78,7 +83,8 @@ modpost = scripts/mod/modpost \
$(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
$(if $(KBUILD_EXTMOD),-o $(modulesymfile)) \
$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E) \
- $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w) \
+ $(if $(CONFIG_LIVEPATCH),-l $(MODVERDIR)/livepatchmods)
MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 374b22c76ec5..b3ab17d9d834 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1974,10 +1974,6 @@ static void read_symbols(const char *modname)
license = get_next_modinfo(&info, "license", license);
}
- /* Livepatch modules have unresolved symbols resolved by klp-convert */
- if (get_modinfo(&info, "livepatch"))
- mod->livepatch = 1;
-
for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
symname = remove_dot(info.strtab + sym->st_name);
@@ -2416,6 +2412,76 @@ static void write_dump(const char *fname)
free(buf.p);
}
+struct livepatch_mod_list {
+ struct livepatch_mod_list *next;
+ char *livepatch_mod;
+};
+
+static struct livepatch_mod_list *load_livepatch_mods(const char *fname)
+{
+ struct livepatch_mod_list *list_iter, *list_start = NULL;
+ unsigned long size, pos = 0;
+ void *file = grab_file(fname, &size);
+ char *line;
+
+ if (!file)
+ return NULL;
+
+ while ((line = get_next_line(&pos, file, size))) {
+ list_iter = NOFAIL(malloc(sizeof(*list_iter)));
+ list_iter->next = list_start;
+ list_iter->livepatch_mod = NOFAIL(strdup(line));
+ list_start = list_iter;
+ }
+ release_file(file, size);
+
+ return list_start;
+}
+
+static void free_livepatch_mods(struct livepatch_mod_list *list_start)
+{
+ struct livepatch_mod_list *list_iter;
+
+ while (list_start) {
+ list_iter = list_start->next;
+ free(list_start->livepatch_mod);
+ free(list_start);
+ list_start = list_iter;
+ }
+}
+
+static int is_livepatch_mod(const char *modname,
+ struct livepatch_mod_list *list)
+{
+ const char *myname;
+
+ if (!list)
+ return 0;
+
+ myname = strrchr(modname, '/');
+ if (myname)
+ myname++;
+ else
+ myname = modname;
+
+ while (list) {
+ if (!strcmp(myname, list->livepatch_mod))
+ return 1;
+ list = list->next;
+ }
+ return 0;
+}
+
+static void add_livepatch_flag(struct buffer *b, struct module *mod,
+ struct livepatch_mod_list *list)
+{
+ if (is_livepatch_mod(mod->name, list)) {
+ buf_printf(b, "\nMODULE_INFO(livepatch, \"Y\");\n");
+ mod->livepatch = 1;
+ }
+}
+
+
struct ext_sym_list {
struct ext_sym_list *next;
const char *file;
@@ -2431,8 +2497,9 @@ int main(int argc, char **argv)
int err;
struct ext_sym_list *extsym_iter;
struct ext_sym_list *extsym_start = NULL;
+ struct livepatch_mod_list *livepatch_mods = NULL;
- while ((opt = getopt(argc, argv, "i:I:e:mnsT:o:awE")) != -1) {
+ while ((opt = getopt(argc, argv, "i:I:e:l:mnsT:o:awE")) != -1) {
switch (opt) {
case 'i':
kernel_read = optarg;
@@ -2449,6 +2516,9 @@ int main(int argc, char **argv)
extsym_iter->file = optarg;
extsym_start = extsym_iter;
break;
+ case 'l':
+ livepatch_mods = load_livepatch_mods(optarg);
+ break;
case 'm':
modversions = 1;
break;
@@ -2506,15 +2576,16 @@ int main(int argc, char **argv)
buf.pos = 0;
err |= check_modname_len(mod);
- err |= check_exports(mod);
add_header(&buf, mod);
add_intree_flag(&buf, !external_module);
add_retpoline(&buf);
add_staging_flag(&buf, mod->name);
+ add_livepatch_flag(&buf, mod, livepatch_mods);
err |= add_versions(&buf, mod);
add_depends(&buf, mod);
add_moddevtable(&buf, mod);
add_srcversion(&buf, mod);
+ err |= check_exports(mod);
sprintf(fname, "%s.mod.c", mod->name);
write_if_changed(&buf, fname);
@@ -2524,6 +2595,7 @@ int main(int argc, char **argv)
if (sec_mismatch_count && sec_mismatch_fatal)
fatal("modpost: Section mismatches detected.\n"
"Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+ free_livepatch_mods(livepatch_mods);
free(buf.p);
return err;
--
2.20.1
From: Joao Moreira <[email protected]>
For automatic resolution of livepatch relocations, a file called
Symbols.list is used. This file maps symbols within every compiled
kernel object allowing the identification of symbols whose name is
unique, thus relocation can be automatically inferred, or providing
information that helps developers when code annotation is required for
solving the matter.
Add support for creating Symbols.list in the main Makefile. First,
ensure that built-in is compiled when CONFIG_LIVEPATCH is enabled (as
required to achieve a complete Symbols.list file). Define the command to
build Symbols.list (cmd_klp_map) and hook it in the modules rule.
As it is undesirable to have symbols from livepatch objects inside
Symbols.list, make livepatches discernible by modifying
scripts/Makefile.build to create a .livepatch file for each livepatch
in $(MODVERDIR). This file then used by cmd_klp_map to identify and
bypass livepatches.
For identifying livepatches during the build process, a flag variable
LIVEPATCH_$(basetarget).o is considered in scripts/Makefile.build. This
way, set this flag for the livepatch sample Makefile in
samples/livepatch/Makefile.
Finally, Add a clean rule to ensure that Symbols.list is removed during
clean.
Notes:
To achieve a correct Symbols.list file, all kernel objects must be
considered, thus, its construction require these objects to be priorly
built. On the other hand, invoking scripts/Makefile.modpost without
having a complete Symbols.list in place would occasionally lead to
in-tree livepatches being post-processed incorrectly. To prevent this
from becoming a circular dependency, the construction of Symbols.list
uses non-post-processed kernel objects and such does not cause harm as
the symbols normally referenced from within livepatches are visible at
this stage. Also due to these requirements, the spot in-between modules
compilation and the invocation of scripts/Makefile.modpost was picked
for hooking cmd_klp_map.
The approach based on .livepatch files was proposed as an alternative
to using MODULE_INFO statements. This approach was originally
proposed by Miroslav Benes as a workaround for identifying livepathes
without depending on modinfo during the modpost stage. It was moved to
this patch as the approach also shown to be useful while building
Symbols.list.
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
.gitignore | 1 +
Makefile | 30 ++++++++++++++++++++++++++----
samples/livepatch/Makefile | 1 +
scripts/Makefile.build | 7 +++++++
4 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/.gitignore b/.gitignore
index a20ac26aa2f5..5cd5758f5ffe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,6 +45,7 @@
*.xz
Module.symvers
modules.builtin
+Symbols.list
#
# Top-level generic files
diff --git a/Makefile b/Makefile
index 15c8251d4d5e..2c07bdd87b2f 100644
--- a/Makefile
+++ b/Makefile
@@ -574,10 +574,13 @@ KBUILD_BUILTIN := 1
# If we have only "make modules", don't compile built-in objects.
# When we're building modules with modversions, we need to consider
# the built-in objects during the descend as well, in order to
-# make sure the checksums are up to date before we record them.
+# make sure the checksums are up to date before we record them. The
+# same applies for building livepatches, as built-in objects may hold
+# symbols which are referenced from livepatches and are required by
+# klp-convert post-processing tool for resolving these cases.
ifeq ($(MAKECMDGOALS),modules)
- KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
+ KBUILD_BUILTIN := $(if $(or $(CONFIG_MODVERSIONS), $(CONFIG_LIVEPATCH)),1)
endif
# If we have "make <whatever> modules", compile modules
@@ -1261,9 +1264,25 @@ all: modules
# duplicate lines in modules.order files. Those are removed
# using awk while concatenating to the final file.
+quiet_cmd_klp_map = KLP Symbols.list
+SLIST = $(objtree)/Symbols.list
+
+define cmd_klp_map
+ $(shell echo "klp-convert-symbol-data.0.1" > $(SLIST)) \
+ $(shell echo "*vmlinux" >> $(SLIST)) \
+ $(shell nm -f posix $(objtree)/vmlinux | cut -d\ -f1 >> $(SLIST)) \
+ $(foreach m, $(wildcard $(MODVERDIR)/*.mod), \
+ $(eval mod = $(patsubst %.ko,%.o,$(shell head -n1 $(m)))) \
+ $(if $(wildcard $(MODVERDIR)/$(shell basename -s .o $(mod)).livepatch),,\
+ $(eval fmod = $(subst $(quote),_,$(subst -,_,$(mod)))) \
+ $(shell echo "*$(shell basename -s .o $(fmod))" >> $(SLIST)) \
+ $(shell nm -f posix $(mod) | cut -d\ -f1 >> $(SLIST))))
+endef
+
PHONY += modules
modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin
$(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order
+ $(if $(CONFIG_LIVEPATCH), $(call cmd,klp_map))
@$(kecho) ' Building modules, stage 2.';
$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
@@ -1350,7 +1369,7 @@ clean: rm-dirs := $(CLEAN_DIRS)
clean: rm-files := $(CLEAN_FILES)
clean-dirs := $(addprefix _clean_, . $(vmlinux-alldirs) Documentation samples)
-PHONY += $(clean-dirs) clean archclean vmlinuxclean
+PHONY += $(clean-dirs) clean archclean vmlinuxclean klpclean
$(clean-dirs):
$(Q)$(MAKE) $(clean)=$(patsubst _clean_%,%,$@)
@@ -1358,7 +1377,10 @@ vmlinuxclean:
$(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
$(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
-clean: archclean vmlinuxclean
+klpclean:
+ $(Q) rm -f $(objtree)/Symbols.list
+
+clean: archclean vmlinuxclean klpclean
# mrproper - Delete all generated files, including .config
#
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 2472ce39a18d..8b9b42a258ad 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1,3 +1,4 @@
+LIVEPATCH_livepatch-sample := y
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 76ca30cc4791..ca76bd2080f0 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -246,6 +246,11 @@ cmd_gen_ksymdeps = \
$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
endif
+ifdef CONFIG_LIVEPATCH
+cmd_livepatch = $(if $(LIVEPATCH_$(basetarget)), \
+ $(shell touch $(MODVERDIR)/$(basetarget).livepatch))
+endif
+
define rule_cc_o_c
$(call cmd,checksrc)
$(call cmd_and_fixdep,cc_o_c)
@@ -280,6 +285,7 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
$(call cmd,force_checksrc)
$(call if_changed_rule,cc_o_c)
+ $(call cmd_livepatch)
@{ echo $(@:.o=.ko); echo $@; \
$(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
@@ -456,6 +462,7 @@ cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ $(filter %.o,$^) $(cmd_secanalysis
$(multi-used-m): FORCE
$(call if_changed,link_multi-m)
+ $(call cmd,livepatch)
@{ echo $(@:.o=.ko); echo $(filter %.o,$^); \
$(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
$(call multi_depend, $(multi-used-m), .o, -objs -y -m)
--
2.20.1
From: Josh Poimboeuf <[email protected]>
Define klp prefixes in include/uapi/linux/livepatch.h, and use them for
replacing hard-coded values in kernel/livepatch/core.c.
Update MAINTAINERS.
Note: Add defines to uapi as these are also to be used by a newly
introduced klp-convert script.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
MAINTAINERS | 1 +
include/linux/livepatch.h | 1 +
include/uapi/linux/livepatch.h | 17 +++++++++++++++++
kernel/livepatch/core.c | 4 ++--
4 files changed, 21 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/livepatch.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 2359e12e4c41..65b4e8f481f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9017,6 +9017,7 @@ R: Joe Lawrence <[email protected]>
S: Maintained
F: kernel/livepatch/
F: include/linux/livepatch.h
+F: include/uapi/linux/livepatch.h
F: arch/x86/include/asm/livepatch.h
F: arch/x86/kernel/livepatch.c
F: Documentation/livepatch/
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..16b48e8b29a2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -25,6 +25,7 @@
#include <linux/ftrace.h>
#include <linux/completion.h>
#include <linux/list.h>
+#include <uapi/linux/livepatch.h>
#if IS_ENABLED(CONFIG_LIVEPATCH)
diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
new file mode 100644
index 000000000000..bb86243de805
--- /dev/null
+++ b/include/uapi/linux/livepatch.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * livepatch.h - Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
+ */
+
+#ifndef _UAPI_LIVEPATCH_H
+#define _UAPI_LIVEPATCH_H
+
+#include <linux/types.h>
+
+#define KLP_RELA_PREFIX ".klp.rela."
+#define KLP_SYM_PREFIX ".klp.sym."
+
+#endif /* _UAPI_LIVEPATCH_H */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..3d9ed895b252 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -235,7 +235,7 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
/* Format: .klp.sym.objname.symname,sympos */
cnt = sscanf(strtab + sym->st_name,
- ".klp.sym.%55[^.].%127[^,],%lu",
+ KLP_SYM_PREFIX "%55[^.].%127[^,],%lu",
objname, symname, &sympos);
if (cnt != 3) {
pr_err("symbol %s has an incorrectly formatted name\n",
@@ -281,7 +281,7 @@ static int klp_write_object_relocations(struct module *pmod,
* See comment in klp_resolve_symbols() for an explanation
* of the selected field width value.
*/
- cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
+ cnt = sscanf(secname, KLP_RELA_PREFIX "%55[^.]", sec_objname);
if (cnt != 1) {
pr_err("section %s has an incorrectly formatted name\n",
secname);
--
2.20.1
From: Josh Poimboeuf <[email protected]>
Define macros KLP_MODULE_RELOC and KLP_SYMPOS in
include/linux/livepatch.h to improve user-friendliness of the
livepatch annotation process.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
include/linux/livepatch.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 16b48e8b29a2..947cfc2d1980 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -236,6 +236,18 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
+/* Used to annotate symbol relocations in livepatches */
+#define KLP_MODULE_RELOC(obj) \
+ struct klp_module_reloc \
+ __attribute__((__section__(".klp.module_relocs." #obj))) \
+ __attribute__((aligned (4)))
+
+#define KLP_SYMPOS(symbol, pos) \
+ { \
+ .sym = &symbol, \
+ .sympos = pos, \
+ },
+
#else /* !CONFIG_LIVEPATCH */
static inline int klp_module_coming(struct module *mod) { return 0; }
--
2.20.1
From: Josh Poimboeuf <[email protected]>
Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
cmd_klp_convert invokes klp-convert with the right arguments for the
conversion of unresolved symbols inside a livepatch.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Konstantin Khlebnikov <[email protected]>
Signed-off-by: Miroslav Benes <[email protected]>
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
scripts/Kbuild.include | 4 +++-
scripts/Makefile.modpost | 16 +++++++++++++++-
scripts/mod/modpost.c | 6 +++++-
scripts/mod/modpost.h | 1 +
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 7484b9d8272f..f8b7d12e44c9 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -236,6 +236,8 @@ endif
# (needed for the shell)
make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,$$$$,$(cmd_$(1)))))
+save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
+
# Find any prerequisites that is newer than target or that does not exist.
# PHONY targets skipped in both cases.
any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
@@ -243,7 +245,7 @@ any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
# Execute command if command has changed or prerequisite(s) are updated.
if_changed = $(if $(strip $(any-prereq) $(arg-check)), \
$(cmd); \
- printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
+ $(save-cmd), @:)
# Execute the command and also postprocess generated .d dependencies file.
if_changed_dep = $(if $(strip $(any-prereq) $(arg-check)),$(cmd_and_fixdep),@:)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 6b7f354f189a..1e8bb7442689 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -124,8 +124,22 @@ quiet_cmd_ld_ko_o = LD [M] $@
-o $@ $(real-prereqs) ; \
$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
+SLIST = $(objtree)/Symbols.list
+KLP_CONVERT = scripts/livepatch/klp-convert
+quiet_cmd_klp_convert = KLP $@
+ cmd_klp_convert = mv $@ $(@:.ko=.klp.o); \
+ $(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
+
+define rule_ld_ko_o
+ $(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; \
+ $(call save-cmd,ld_ko_o) ; \
+ $(if $(CONFIG_LIVEPATCH), \
+ $(if $(wildcard $(MODVERDIR)/$(basetarget).livepatch), \
+ $(call echo-cmd,klp_convert) $(cmd_klp_convert) ))
+endef
+
$(modules): %.ko :%.o %.mod.o FORCE
- +$(call if_changed,ld_ko_o)
+ +$(call if_changed_rule,ld_ko_o)
targets += $(modules)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..374b22c76ec5 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1974,6 +1974,10 @@ static void read_symbols(const char *modname)
license = get_next_modinfo(&info, "license", license);
}
+ /* Livepatch modules have unresolved symbols resolved by klp-convert */
+ if (get_modinfo(&info, "livepatch"))
+ mod->livepatch = 1;
+
for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
symname = remove_dot(info.strtab + sym->st_name);
@@ -2101,7 +2105,7 @@ static int check_exports(struct module *mod)
const char *basename;
exp = find_symbol(s->name);
if (!exp || exp->module == mod) {
- if (have_vmlinux && !s->weak) {
+ if (have_vmlinux && !s->weak && !mod->livepatch) {
if (warn_unresolved) {
warn("\"%s\" [%s.ko] undefined!\n",
s->name, mod->name);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 8453d6ac2f77..2acfaae064ec 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -118,6 +118,7 @@ struct module {
int skip;
int has_init;
int has_cleanup;
+ int livepatch;
struct buffer dev_table_buf;
char srcversion[25];
int is_dot_o;
--
2.20.1
From: Joao Moreira <[email protected]>
Add a section to Documentation/livepatch/module-elf-format.txt
describing how klp-convert works for fixing relocations.
Signed-off-by: Joao Moreira <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
Documentation/livepatch/livepatch.txt | 3 ++
Documentation/livepatch/module-elf-format.txt | 50 ++++++++++++++++---
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 4627b41ff02e..873c11aee038 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -274,6 +274,9 @@ into three levels:
absolute position in the database, but rather the order it has been found
only for a particular object ( vmlinux or a kernel module ). Note that
kallsyms allows for searching symbols according to the object name.
+ Uniquely named symbols may use a symbol position of 0. Non-unique
+ symbols need to specify their object / kallsyms position, starting
+ at position 1.
+ struct klp_object defines an array of patched functions (struct
klp_func) in the same object. Where the object is either vmlinux
diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
index f21a5289a09c..7bef8432352a 100644
--- a/Documentation/livepatch/module-elf-format.txt
+++ b/Documentation/livepatch/module-elf-format.txt
@@ -2,7 +2,8 @@
Livepatch module Elf format
===========================
-This document outlines the Elf format requirements that livepatch modules must follow.
+This document outlines the Elf format requirements that livepatch modules must
+follow.
-----------------
Table of Contents
@@ -25,8 +26,9 @@ Table of Contents
3.3.2 Required name format
3.3.3 Example livepatch symbol names
3.3.4 Example `readelf --symbols` output
-4. Architecture-specific sections
-5. Symbol table and Elf section access
+4. Automatic conversion of unresolved relocations
+5. Architecture-specific sections
+6. Symbol table and Elf section access
----------------------------
0. Background and motivation
@@ -270,7 +272,8 @@ Livepatch symbol names must conform to the following format:
[D] The position of the symbol in the object (as according to kallsyms)
This is used to differentiate duplicate symbols within the same
object. The symbol position is expressed numerically (0, 1, 2...).
- The symbol position of a unique symbol is 0.
+ The symbol position of a unique symbol is 0. The symbol position of
+ the first non-unique symbol is 1, the second is 2, etc.
3.3.3 Example livepatch symbol names:
-------------------------------------
@@ -293,8 +296,43 @@ Symbol table '.symtab' contains 127 entries:
[*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
"OS" means OS-specific.
+--------------------------------------------------
+4. Automatic conversion of unresolved relocations
+--------------------------------------------------
+Sometimes livepatches may operate on symbols which are not self-contained nor
+exported. When this happens, these symbols remain unresolved in the elf object
+and will trigger an error during the livepatch instantiation.
+
+Whenever possible, the kernel building infrastructure solves this problem
+automatically. First, a symbol database containing information on all compiled
+objects is built. Second, this database - a file named Symbols.list, placed in
+the kernel source root directory - is used to identify targets for unresolved
+relocations, converting them in the livepatch elf accordingly to the
+specifications above-described. While the first stage is fully handled by the
+building system, the second is done by a tool called klp-convert, which can be
+found in "scripts/livepatch".
+
+When an unresolved relocation has as target a symbol whose name is also used by
+different symbols throughout the kernel, the relocation cannot be resolved
+automatically. In these cases, the livepatch developer must add annotations to
+the livepatch, making it possible for the system to identify which is the
+correct target amongst multiple homonymous symbols. Such annotations must be
+done through a data structure as follows:
+
+struct KLP_MODULE_RELOC(object) data_structure_name[] = {
+ KLP_SYMPOS(symbol, pos)
+};
+
+In the above example, object refers to the object file which contains the
+symbol, being vmlinux or a module; symbol refers to the symbol name that will
+be relocated and pos is its position in the object.
+
+When a data structure like this is added to the livepatch, the resulting elf
+will hold symbols that will be identified by klp-convert and used to solve name
+ambiguities.
+
---------------------------------
-4. Architecture-specific sections
+5. Architecture-specific sections
---------------------------------
Architectures may override arch_klp_init_object_loaded() to perform
additional arch-specific tasks when a target module loads, such as applying
@@ -305,7 +343,7 @@ be easily identified when iterating through a patch module's Elf sections
(See arch/x86/kernel/livepatch.c for a complete example).
--------------------------------------
-5. Symbol table and Elf section access
+6. Symbol table and Elf section access
--------------------------------------
A livepatch module's symbol table is accessible through module->symtab.
--
2.20.1
Add a simple klp-convert livepatch selftest that exercises various
symbol homonym sympos scenarios.
Signed-off-by: Joe Lawrence <[email protected]>
---
lib/livepatch/Makefile | 10 ++
lib/livepatch/test_klp_convert1.c | 106 ++++++++++++++++++
lib/livepatch/test_klp_convert2.c | 103 +++++++++++++++++
lib/livepatch/test_klp_convert_mod_a.c | 25 +++++
lib/livepatch/test_klp_convert_mod_b.c | 13 +++
.../selftests/livepatch/test-livepatch.sh | 64 +++++++++++
6 files changed, 321 insertions(+)
create mode 100644 lib/livepatch/test_klp_convert1.c
create mode 100644 lib/livepatch/test_klp_convert2.c
create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
create mode 100644 lib/livepatch/test_klp_convert_mod_b.c
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 513d200b7942..b19253c72d72 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -5,6 +5,8 @@
LIVEPATCH_test_klp_atomic_replace := y
LIVEPATCH_test_klp_callbacks_demo := y
LIVEPATCH_test_klp_callbacks_demo2 := y
+LIVEPATCH_test_klp_convert1 := y
+LIVEPATCH_test_klp_convert2 := y
LIVEPATCH_test_klp_livepatch := y
obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
@@ -12,9 +14,17 @@ obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
test_klp_callbacks_demo2.o \
test_klp_callbacks_busy.o \
test_klp_callbacks_mod.o \
+ test_klp_convert1.o \
+ test_klp_convert2.o \
+ test_klp_convert_mod.o \
test_klp_livepatch.o \
test_klp_shadow_vars.o
+test_klp_convert_mod-y := \
+ test_klp_convert_mod_a.o \
+ test_klp_convert_mod_b.o
+
# Target modules to be livepatched require CC_FLAGS_FTRACE
CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
CFLAGS_test_klp_callbacks_mod.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_convert_mod.o += $(CC_FLAGS_FTRACE)
diff --git a/lib/livepatch/test_klp_convert1.c b/lib/livepatch/test_klp_convert1.c
new file mode 100644
index 000000000000..594ec3a7cf70
--- /dev/null
+++ b/lib/livepatch/test_klp_convert1.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <[email protected]>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/* klp-convert symbols - vmlinux */
+extern char *saved_command_line;
+/* klp-convert symbols - test_klp_convert_mod.ko */
+extern char driver_name[];
+extern char homonym_string[];
+extern const char *get_homonym_string(void);
+extern const char *get_driver_name(void);
+
+void print_saved_command_line(void)
+{
+ pr_info("saved_command_line, 0: %s\n", saved_command_line);
+}
+
+void print_driver_name(void)
+{
+ pr_info("driver_name, 0: %s\n", driver_name);
+ pr_info("get_driver_name(), 0: %s\n", get_driver_name());
+}
+
+void print_homonym_string(void)
+{
+ pr_info("homonym_string, 1: %s\n", homonym_string);
+ pr_info("get_homonym_string(), 1: %s\n", get_homonym_string());
+}
+
+/*
+ * saved_command_line is a unique symbol, so the sympos annotation is
+ * optional. Provide to test that sympos=0 works correctly.
+ */
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+ KLP_SYMPOS(saved_command_line, 0)
+};
+
+/*
+ * driver_name symbols can be found in vmlinux (multiple) and also
+ * test_klp_convert_mod, therefore the annotation is required to
+ * clarify that we want the one from test_klp_convert_mod.
+ *
+ * test_klp_convert_mod contains multiple homonym_string and
+ * get_homonym_string symbols, test resolving the first set here and
+ * the others in test_klp_convert2.c
+ *
+ * get_driver_name is a uniquely named symbol, test that sympos=0
+ * work correctly.
+ */
+KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
+ KLP_SYMPOS(driver_name, 0)
+ KLP_SYMPOS(homonym_string, 1)
+ KLP_SYMPOS(get_homonym_string, 1)
+ KLP_SYMPOS(get_driver_name, 0)
+};
+
+static struct klp_func funcs[] = {
+ {
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ },
+ {
+ .name = "test_klp_convert_mod",
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int test_klp_convert_init(void)
+{
+ int ret;
+
+ ret = klp_enable_patch(&patch);
+ if (ret)
+ return ret;
+
+ print_saved_command_line();
+ print_driver_name();
+ print_homonym_string();
+
+ return 0;
+}
+
+static void test_klp_convert_exit(void)
+{
+}
+
+module_init(test_klp_convert_init);
+module_exit(test_klp_convert_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <[email protected]>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert1");
diff --git a/lib/livepatch/test_klp_convert2.c b/lib/livepatch/test_klp_convert2.c
new file mode 100644
index 000000000000..587dcecd546d
--- /dev/null
+++ b/lib/livepatch/test_klp_convert2.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <[email protected]>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+/* klp-convert symbols - vmlinux */
+extern char *saved_command_line;
+/* klp-convert symbols - test_klp_convert_mod.ko */
+extern char driver_name[];
+extern char homonym_string[];
+extern const char *get_homonym_string(void);
+extern const char *get_driver_name(void);
+
+void print_saved_command_line(void)
+{
+ pr_info("saved_command_line (auto): %s\n", saved_command_line);
+}
+
+void print_driver_name(void)
+{
+ pr_info("driver_name, 0: %s\n", driver_name);
+ pr_info("get_driver_name(), (auto): %s\n", get_driver_name());
+}
+
+void print_homonym_string(void)
+{
+ pr_info("homonym_string, 2: %s\n", homonym_string);
+ pr_info("get_homonym_string(), 2: %s\n", get_homonym_string());
+}
+
+/*
+ * saved_command_line is a uniquely named symbol, so the sympos
+ * annotation is optional. Skip it and test that klp-convert can
+ * resolve the symbol on its own.
+ */
+
+/*
+ * driver_name symbols can be found in vmlinux (multiple) and also
+ * test_klp_convert_mod, therefore the annotation is required to
+ * clarify that we want the one from test_klp_convert_mod.
+ *
+ * test_klp_convert_mod contains multiple homonym_string symbols,
+ * test_klp_convert1.c resolved to the first one, resolve to the
+ * second one here.
+ *
+ * get_driver_name is a uniquely named symbol, test klp-convert can
+ * resolve it automatically.
+ */
+KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
+ KLP_SYMPOS(driver_name, 0)
+ KLP_SYMPOS(homonym_string, 2)
+ KLP_SYMPOS(get_homonym_string, 2)
+};
+
+static struct klp_func funcs[] = {
+ {
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ },
+ {
+ .name = "test_klp_convert_mod",
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int test_klp_convert_init(void)
+{
+ int ret;
+
+ ret = klp_enable_patch(&patch);
+ if (ret)
+ return ret;
+
+ print_saved_command_line();
+ print_driver_name();
+ print_homonym_string();
+
+ return 0;
+}
+
+static void test_klp_convert_exit(void)
+{
+}
+
+module_init(test_klp_convert_init);
+module_exit(test_klp_convert_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <[email protected]>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert2");
diff --git a/lib/livepatch/test_klp_convert_mod_a.c b/lib/livepatch/test_klp_convert_mod_a.c
new file mode 100644
index 000000000000..b2db9942fbd2
--- /dev/null
+++ b/lib/livepatch/test_klp_convert_mod_a.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <[email protected]>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+/* Unique symbols that don't need sympos annotation */
+static const char driver_name[] = KBUILD_MODNAME;
+__used static const char *get_driver_name(void)
+{
+ return driver_name;
+}
+
+/* Common symbol names that need sympos */
+static const char homonym_string[] = "homonym string A";
+__used static const char *get_homonym_string(void)
+{
+ return homonym_string;
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <[email protected]>");
+MODULE_DESCRIPTION("Livepatch test: klp-convert module");
diff --git a/lib/livepatch/test_klp_convert_mod_b.c b/lib/livepatch/test_klp_convert_mod_b.c
new file mode 100644
index 000000000000..a3d529ff5f1f
--- /dev/null
+++ b/lib/livepatch/test_klp_convert_mod_b.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence <[email protected]>
+
+/*
+ * A second compilation unit to provide another set of similarly named
+ * symbols, forcing a livepatch to use sympos annotations.
+ */
+
+static char homonym_string[] = "homonym string B";
+__used static char *get_homonym_string(void)
+{
+ return homonym_string;
+}
diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index f05268aea859..9c3745e8ba1d 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -6,6 +6,9 @@
MOD_LIVEPATCH=test_klp_livepatch
MOD_REPLACE=test_klp_atomic_replace
+MOD_KLP_CONVERT_MOD=test_klp_convert_mod
+MOD_KLP_CONVERT1=test_klp_convert1
+MOD_KLP_CONVERT2=test_klp_convert2
set_dynamic_debug
@@ -165,4 +168,65 @@ livepatch: '$MOD_REPLACE': unpatching complete
% rmmod $MOD_REPLACE"
+# TEST: klp-convert symbols
+# - load a livepatch that modifies the output from /proc/cmdline
+# including a reference to vmlinux-local symbol that klp-convert
+# will process
+# - verify correct behavior
+# - unload the livepatch and make sure the patch was removed
+
+echo -n "TEST: klp-convert symbols ... "
+dmesg -C
+
+saved_cmdline=$(cat /proc/cmdline)
+
+load_mod $MOD_KLP_CONVERT_MOD
+
+load_lp $MOD_KLP_CONVERT1
+disable_lp $MOD_KLP_CONVERT1
+unload_lp $MOD_KLP_CONVERT1
+
+load_lp $MOD_KLP_CONVERT2
+disable_lp $MOD_KLP_CONVERT2
+unload_lp $MOD_KLP_CONVERT2
+
+unload_mod $MOD_KLP_CONVERT_MOD
+
+check_result "% modprobe $MOD_KLP_CONVERT_MOD
+% modprobe $MOD_KLP_CONVERT1
+livepatch: enabling patch '$MOD_KLP_CONVERT1'
+livepatch: '$MOD_KLP_CONVERT1': initializing patching transition
+livepatch: '$MOD_KLP_CONVERT1': starting patching transition
+$MOD_KLP_CONVERT1: saved_command_line, 0: $saved_cmdline
+$MOD_KLP_CONVERT1: driver_name, 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT1: get_driver_name(), 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT1: homonym_string, 1: homonym string A
+$MOD_KLP_CONVERT1: get_homonym_string(), 1: homonym string A
+livepatch: '$MOD_KLP_CONVERT1': completing patching transition
+livepatch: '$MOD_KLP_CONVERT1': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_KLP_CONVERT1/enabled
+livepatch: '$MOD_KLP_CONVERT1': initializing unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': starting unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': completing unpatching transition
+livepatch: '$MOD_KLP_CONVERT1': unpatching complete
+% rmmod $MOD_KLP_CONVERT1
+% modprobe $MOD_KLP_CONVERT2
+livepatch: enabling patch '$MOD_KLP_CONVERT2'
+livepatch: '$MOD_KLP_CONVERT2': initializing patching transition
+livepatch: '$MOD_KLP_CONVERT2': starting patching transition
+$MOD_KLP_CONVERT2: saved_command_line (auto): $saved_cmdline
+$MOD_KLP_CONVERT2: driver_name, 0: $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT2: get_driver_name(), (auto): $MOD_KLP_CONVERT_MOD
+$MOD_KLP_CONVERT2: homonym_string, 2: homonym string B
+$MOD_KLP_CONVERT2: get_homonym_string(), 2: homonym string B
+livepatch: '$MOD_KLP_CONVERT2': completing patching transition
+livepatch: '$MOD_KLP_CONVERT2': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_KLP_CONVERT2/enabled
+livepatch: '$MOD_KLP_CONVERT2': initializing unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': starting unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': completing unpatching transition
+livepatch: '$MOD_KLP_CONVERT2': unpatching complete
+% rmmod $MOD_KLP_CONVERT2
+% rmmod $MOD_KLP_CONVERT_MOD"
+
exit 0
--
2.20.1
On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> new file mode 100644
> index 000000000000..62bd26941081
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.c
>
> [ ... snip ... ]
>
> + if (argc != 4) {
> + WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
^^^^^^^^^^
nit: since we're using .klp.o prefix, should this be "input.klp.o"
-- Joe
On Wed, Apr 10, 2019 at 11:50:53AM -0400, Joe Lawrence wrote:
> From: Josh Poimboeuf <[email protected]>
>
> Define macros KLP_MODULE_RELOC and KLP_SYMPOS in
> include/linux/livepatch.h to improve user-friendliness of the
> livepatch annotation process.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joao Moreira <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> include/linux/livepatch.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 16b48e8b29a2..947cfc2d1980 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -236,6 +236,18 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
>
> +/* Used to annotate symbol relocations in livepatches */
> +#define KLP_MODULE_RELOC(obj) \
> + struct klp_module_reloc \
> + __attribute__((__section__(".klp.module_relocs." #obj))) \
> + __attribute__((aligned (4)))
> +
> +#define KLP_SYMPOS(symbol, pos) \
> + { \
> + .sym = &symbol, \
> + .sympos = pos, \
> + },
^^
nit: if we dropped the trailing array comma delimiter from KLP_SYMPOS
macro, the invocations would look more intuitively like an array. For
example:
KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
KLP_SYMPOS(driver_name, 0),
KLP_SYMPOS(homonym_string, 2),
KLP_SYMPOS(get_homonym_string, 2),
};
But I could not figure out a good regex to reference if other such
kernel preprocessor macros include or exclude the delimiter. Are there
reasons to include it?
-- Joe
On Thu, Apr 11, 2019 at 12:52 AM Joe Lawrence <[email protected]> wrote:
>
> From: Josh Poimboeuf <[email protected]>
>
> Define klp prefixes in include/uapi/linux/livepatch.h, and use them for
> replacing hard-coded values in kernel/livepatch/core.c.
>
> Update MAINTAINERS.
>
> Note: Add defines to uapi as these are also to be used by a newly
> introduced klp-convert script.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joao Moreira <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> MAINTAINERS | 1 +
> include/linux/livepatch.h | 1 +
> include/uapi/linux/livepatch.h | 17 +++++++++++++++++
> kernel/livepatch/core.c | 4 ++--
> 4 files changed, 21 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/livepatch.h
> diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> new file mode 100644
> index 000000000000..bb86243de805
> --- /dev/null
> +++ b/include/uapi/linux/livepatch.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
In my understanding, UAPI headers should be licensed under:
/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +/*
> + * livepatch.h - Kernel Live Patching Core
> + *
> + * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
> + */
> +
> +#ifndef _UAPI_LIVEPATCH_H
> +#define _UAPI_LIVEPATCH_H
> +
> +#include <linux/types.h>
Why is this include needed?
> +#define KLP_RELA_PREFIX ".klp.rela."
> +#define KLP_SYM_PREFIX ".klp.sym."
These do not depend on <linux/types.h>
> +
> +#endif /* _UAPI_LIVEPATCH_H */
--
Best Regards
Masahiro Yamada
On Wed, Apr 10, 2019 at 11:50:51AM -0400, Joe Lawrence wrote:
> -clean: archclean vmlinuxclean
> +klpclean:
> + $(Q) rm -f $(objtree)/Symbols.list
nit: $(SLIST) can be used here.
> +clean: archclean vmlinuxclean klpclean
>
> # mrproper - Delete all generated files, including .config
> #
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 2472ce39a18d..8b9b42a258ad 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -1,3 +1,4 @@
> +LIVEPATCH_livepatch-sample := y
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 76ca30cc4791..ca76bd2080f0 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -246,6 +246,11 @@ cmd_gen_ksymdeps = \
> $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
> endif
>
> +ifdef CONFIG_LIVEPATCH
> +cmd_livepatch = $(if $(LIVEPATCH_$(basetarget)), \
> + $(shell touch $(MODVERDIR)/$(basetarget).livepatch))
> +endif
> +
> define rule_cc_o_c
> $(call cmd,checksrc)
> $(call cmd_and_fixdep,cc_o_c)
> @@ -280,6 +285,7 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
> $(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
> $(call cmd,force_checksrc)
> $(call if_changed_rule,cc_o_c)
> + $(call cmd_livepatch)
nit: maybe use "cmd,livepatch" to be consistent with the other call of
this function.
> @{ echo $(@:.o=.ko); echo $@; \
> $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
>
> @@ -456,6 +462,7 @@ cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ $(filter %.o,$^) $(cmd_secanalysis
>
> $(multi-used-m): FORCE
> $(call if_changed,link_multi-m)
> + $(call cmd,livepatch)
> @{ echo $(@:.o=.ko); echo $(filter %.o,$^); \
> $(cmd_undef_syms); } > $(MODVERDIR)/$(@F:.o=.mod)
> $(call multi_depend, $(multi-used-m), .o, -objs -y -m)
> --
> 2.20.1
>
--
Artem
On 4/10/19 8:32 PM, Masahiro Yamada wrote:
> On Thu, Apr 11, 2019 at 12:52 AM Joe Lawrence <[email protected]> wrote:
>>
>> From: Josh Poimboeuf <[email protected]>
>>
>> Define klp prefixes in include/uapi/linux/livepatch.h, and use them for
>> replacing hard-coded values in kernel/livepatch/core.c.
>>
>> [ ... snip ... ]
>>
>> diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
>> new file mode 100644
>> index 000000000000..bb86243de805
>> --- /dev/null
>> +++ b/include/uapi/linux/livepatch.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>
>
> In my understanding, UAPI headers should be licensed under:
>
> /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
>
Good eye, that is an interesting licensing detail. Easy enough to
update assuming Josh (original author) is fine with adding the modifier.
>> +
>> +/*
>> + * livepatch.h - Kernel Live Patching Core
>> + *
>> + * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
>> + */
>> +
>> +#ifndef _UAPI_LIVEPATCH_H
>> +#define _UAPI_LIVEPATCH_H
>> +
>> +#include <linux/types.h>
>
>
> Why is this include needed?
>
>> +#define KLP_RELA_PREFIX ".klp.rela."
>> +#define KLP_SYM_PREFIX ".klp.sym."
>
> These do not depend on <linux/types.h>
>
Hmmm, types.h has been included since v1, but I just verified that
removing it results in a clean build, so you are correct that it is
(now) unnecessary.
Thanks,
-- Joe
On 4/11/19 5:18 AM, Artem Savkov wrote:
> On Wed, Apr 10, 2019 at 11:50:51AM -0400, Joe Lawrence wrote:
>> -clean: archclean vmlinuxclean
>> +klpclean:
>> + $(Q) rm -f $(objtree)/Symbols.list
>
> nit: $(SLIST) can be used here.
>
>> +clean: archclean vmlinuxclean klpclean
>>
>> # mrproper - Delete all generated files, including .config
>> #
>> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
>> index 2472ce39a18d..8b9b42a258ad 100644
>> --- a/samples/livepatch/Makefile
>> +++ b/samples/livepatch/Makefile
>> @@ -1,3 +1,4 @@
>> +LIVEPATCH_livepatch-sample := y
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 76ca30cc4791..ca76bd2080f0 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -246,6 +246,11 @@ cmd_gen_ksymdeps = \
>> $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
>> endif
>>
>> +ifdef CONFIG_LIVEPATCH
>> +cmd_livepatch = $(if $(LIVEPATCH_$(basetarget)), \
>> + $(shell touch $(MODVERDIR)/$(basetarget).livepatch))
>> +endif
>> +
>> define rule_cc_o_c
>> $(call cmd,checksrc)
>> $(call cmd_and_fixdep,cc_o_c)
>> @@ -280,6 +285,7 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
>> $(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
>> $(call cmd,force_checksrc)
>> $(call if_changed_rule,cc_o_c)
>> + $(call cmd_livepatch)
>
> nit: maybe use "cmd,livepatch" to be consistent with the other call of
> this function.
>
Both of these make sense, thanks Artem.
-- Joe
On Thu, Apr 11, 2019 at 10:30:45AM -0400, Joe Lawrence wrote:
> On 4/10/19 8:32 PM, Masahiro Yamada wrote:
> > On Thu, Apr 11, 2019 at 12:52 AM Joe Lawrence <[email protected]> wrote:
> > >
> > > From: Josh Poimboeuf <[email protected]>
> > >
> > > Define klp prefixes in include/uapi/linux/livepatch.h, and use them for
> > > replacing hard-coded values in kernel/livepatch/core.c.
> > >
> > > [ ... snip ... ]
> >>
> > > diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h
> > > new file mode 100644
> > > index 000000000000..bb86243de805
> > > --- /dev/null
> > > +++ b/include/uapi/linux/livepatch.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> >
> >
> > In my understanding, UAPI headers should be licensed under:
> >
> > /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >
> >
>
> Good eye, that is an interesting licensing detail. Easy enough to update
> assuming Josh (original author) is fine with adding the modifier.
Sure.
(and sorry, still behind on reviewing the rest of the changes...)
--
Josh
On Wed, Apr 10, 2019 at 11:50:54AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 6b7f354f189a..1e8bb7442689 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -124,8 +124,22 @@ quiet_cmd_ld_ko_o = LD [M] $@
> -o $@ $(real-prereqs) ; \
> $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>
> +SLIST = $(objtree)/Symbols.list
> +KLP_CONVERT = scripts/livepatch/klp-convert
> +quiet_cmd_klp_convert = KLP $@
> + cmd_klp_convert = mv $@ $(@:.ko=.klp.o); \
> + $(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
> +
> +define rule_ld_ko_o
> + $(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; \
> + $(call save-cmd,ld_ko_o) ; \
> + $(if $(CONFIG_LIVEPATCH), \
> + $(if $(wildcard $(MODVERDIR)/$(basetarget).livepatch), \
> + $(call echo-cmd,klp_convert) $(cmd_klp_convert) ))
> +endef
> +
> $(modules): %.ko :%.o %.mod.o FORCE
> - +$(call if_changed,ld_ko_o)
> + +$(call if_changed_rule,ld_ko_o)
>
> targets += $(modules)
I just noticed that rule_ld_ko_o produces verbose output when linking
modules:
% make
...
echo ' LD [M] drivers/ata/ata_generic.ko'; ld -r -m elf_x86_64 -z max-page-size=0x200000 -T ./scripts/module-common.lds --build-id -o drivers/ata/ata_generic.ko drivers/ata/ata_generic.o drivers/ata/ata_generic.mod.o ; true ; printf '%s\n' 'cmd_drivers/ata/ata_generic.ko := ld -r -m elf_x86_64 -z max-page-size=0x200000 -T ./scripts/module-common.lds --build-id -o drivers/ata/ata_generic.ko drivers/ata/ata_generic.o drivers/ata/ata_generic.mod.o ; true' > drivers/ata/.ata_generic.ko.cmd ;
LD [M] drivers/ata/ata_generic.ko
so I think we need to use the $(Q) macro to respect the KBUILD_VERBOSE
and V kbuild settings.
Masahiro, does something like this look correct?
Thanks,
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
From 5c8a2c58c7be98c0a9156155f201b88cc61bf0bd Mon Sep 17 00:00:00 2001
From: Joe Lawrence <[email protected]>
Date: Thu, 11 Apr 2019 11:32:37 -0400
Subject: [PATCH] [squash] modpost: fix rule_ld_ko_o verbosity
Note: squash with ("modpost: Integrate klp-convert")
rule_ld_ko_o should include $(Q) to honor build verbosity setting.
Cargo-cult-stolen from 70923bd26c73 ("perf tools: Make flex/bison calls
honour V=1").
[joe: quiet down rule_ld_ko_o]
Signed-off-by: Joe Lawrence <[email protected]>
---
scripts/Makefile.modpost | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 9fe4c5760aca..f2aee6b8dcfd 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -135,11 +135,11 @@ KLP_CONVERT = scripts/livepatch/klp-convert
quiet_cmd_klp_convert = KLP $@
cmd_klp_convert = mv $@ $(@:.ko=.klp.o); \
$(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
define rule_ld_ko_o
- $(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; \
+ $(Q)$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; \
$(call save-cmd,ld_ko_o) ; \
$(if $(CONFIG_LIVEPATCH), \
$(if $(wildcard $(MODVERDIR)/$(basetarget).livepatch), \
$(call echo-cmd,klp_convert) $(cmd_klp_convert) ))
endef
--
2.20.1
> >> +
> >> +/*
> >> + * livepatch.h - Kernel Live Patching Core
> >> + *
> >> + * Copyright (C) 2016 Josh Poimboeuf <[email protected]>
> >> + */
> >> +
> >> +#ifndef _UAPI_LIVEPATCH_H
> >> +#define _UAPI_LIVEPATCH_H
> >> +
> >> +#include <linux/types.h>
> >
> >
> > Why is this include needed?
> >
> >> +#define KLP_RELA_PREFIX ".klp.rela."
> >> +#define KLP_SYM_PREFIX ".klp.sym."
> >
> > These do not depend on <linux/types.h>
> >
>
> Hmmm, types.h has been included since v1, but I just verified that removing it
> results in a clean build, so you are correct that it is (now) unnecessary.
Yes, I mentioned it while reviewing v2. Unfortunately, it seems to get
lost due to unexplained submitting hiccups. Better to walk through my
emails again and make sure nothing more falls through the cracks...
Miroslav
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 2472ce39a18d..8b9b42a258ad 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -1,3 +1,4 @@
> +LIVEPATCH_livepatch-sample := y
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
We discussed this and I think there has been no conclusion yet. Shouldn't
we do the same (add LIVEPATCH_) for the other samples here as well? So
they are not picked up by cmd_klp_map and their symbols stored in
Symbols.list?
Miroslav
On Wed, 10 Apr 2019, Joe Lawrence wrote:
> On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
> >
> > [ ... snip ... ]
> >
> > diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> > new file mode 100644
> > index 000000000000..62bd26941081
> > --- /dev/null
> > +++ b/scripts/livepatch/klp-convert.c
> >
> > [ ... snip ... ]
> >
> > + if (argc != 4) {
> > + WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]);
> ^^^^^^^^^^
> nit: since we're using .klp.o prefix, should this be "input.klp.o"
I'd leave input.ko there. It is clear then that it is an ordinary kernel
module and nothing special, but it is up to you.
I would change out.ko to output.ko for consistency.
Miroslav
On Wed, 10 Apr 2019, Joe Lawrence wrote:
> On Wed, Apr 10, 2019 at 11:50:53AM -0400, Joe Lawrence wrote:
> > From: Josh Poimboeuf <[email protected]>
> >
> > Define macros KLP_MODULE_RELOC and KLP_SYMPOS in
> > include/linux/livepatch.h to improve user-friendliness of the
> > livepatch annotation process.
> >
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > Signed-off-by: Joao Moreira <[email protected]>
> > Signed-off-by: Joe Lawrence <[email protected]>
> > ---
> > include/linux/livepatch.h | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 16b48e8b29a2..947cfc2d1980 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -236,6 +236,18 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> > void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> > void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
> >
> > +/* Used to annotate symbol relocations in livepatches */
> > +#define KLP_MODULE_RELOC(obj) \
> > + struct klp_module_reloc \
> > + __attribute__((__section__(".klp.module_relocs." #obj))) \
> > + __attribute__((aligned (4)))
> > +
> > +#define KLP_SYMPOS(symbol, pos) \
> > + { \
> > + .sym = &symbol, \
> > + .sympos = pos, \
> > + },
> ^^
> nit: if we dropped the trailing array comma delimiter from KLP_SYMPOS
> macro, the invocations would look more intuitively like an array. For
> example:
>
> KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
> KLP_SYMPOS(driver_name, 0),
> KLP_SYMPOS(homonym_string, 2),
> KLP_SYMPOS(get_homonym_string, 2),
> };
>
> But I could not figure out a good regex to reference if other such
> kernel preprocessor macros include or exclude the delimiter. Are there
> reasons to include it?
No, I don't think so. We should change it. Good catch.
Miroslav
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 8b9b42a258ad..5fb3280bbdc4 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -1,4 +1,8 @@
> LIVEPATCH_livepatch-sample := y
> +LIVEPATCH_livepatch-shadow-fix1 := y
> +LIVEPATCH_livepatch-shadow-fix2 := y
> +LIVEPATCH_livepatch-callbacks-demo := y
You could also remove MODULE_INFO(livepatch, "Y") from the three files.
Miroslav
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> Hi folks,
>
> This is the third installment of the klp-convert tool for generating and
> processing livepatch symbols for livepatch module builds. For those
> following along at home, archive links to previous versions:
>
> RFC:
> https://lore.kernel.org/lkml/[email protected]/
> v2:
> https://lore.kernel.org/lkml/[email protected]/
>
> (Note that I don't see v2 archived at lore, but that is a link to the
> most recent subthread that lore did catch.)
>
>
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols are
> not exported, solving this relocation requires information on the object
> that holds the symbol (either vmlinux or modules) and its position inside
> the object, as an object may contain multiple symbols with the same name.
> Providing such information must be done accordingly to what is specified
> in Documentation/livepatch/module-elf-format.txt.
>
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem in two different forms: (i) by relying on a symbol map, which is
> built during kernel compilation, to automatically infer the relocation
> targeted symbol, and, when such inference is not possible (ii) by using
> annotations in the elf object to convert the relocation accordingly to
> the specification, enabling it to be handled by the livepatch loader.
>
> Given the above, add support for symbol mapping in the form of
> Symbols.list file; add klp-convert tool; integrate klp-convert tool into
> kbuild; make livepatch modules discernible during kernel compilation
> pipeline; add data-structure and macros to enable users to annotate
> livepatch source code; make modpost stage compatible with livepatches;
> update livepatch-sample and update documentation.
>
> The patch was tested under three use-cases:
>
> use-case 1: There is a relocation in the lp that can be automatically
> resolved by klp-convert. For example. see the saved_command_line
> variable in lib/livepatch/test_klp_convert2.c.
>
> use-case 2: There is a relocation in the lp that cannot be automatically
> resolved, as the name of the respective symbol appears in multiple
> objects. The livepatch contains an annotation to enable a correct
> relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> in lib/livepatch/test_klp_convert{1,2}.c.
>
> use-case 3: There is a relocation in the lp that cannot be automatically
> resolved similarly as 2, but no annotation was provided in the
> livepatch, triggering an error during compilation. Reproducible by
> removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> lib/livepatch/test_klp_convert{1,2}.c.
>
>
> Branches
> --------
>
> Since I spent some time tinkering with v2 and accumulated a bunch of
> fixes, I collected them up and am posting this new version. For review
> purposes, I posted two branches up to github:
>
> 1 - an expanded branch that with changes separate from the original
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded
>
> 2 - a squashed branch of (1) that comprises v3:
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3
>
> Non-trivial commits in the expanded branch have some extra commentary
> and details for debugging in the commit message that were dropped when
> squashing into their respective parent commits.
>
>
> TODO
> ----
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
>
>
> Changelogs
> ----------
>
> The commit changelogs were getting long, so I've moved them here for
> posterity and review purposes:
>
> livepatch: Create and include UAPI headers
> v2:
> - jmoreira: split up and changelog
> v3:
> - joe: convert to SPDX license tags
>
> kbuild: Support for Symbols.list creation
> v3:
> - jmoreira: adjust for multiobject livepatch
> - joe: add klpclean to PHONY
> - joe: align KLP prefix
>
> livepatch: Add klp-convert tool
> v2:
> - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
> at the end
> - jmoreira: add support to automatic relocation conversion in
> klp-convert.c, changelog
> v3:
> - joe: convert to SPDX license tags
> - jmoreira: add rela symbol name to WARNs
> - jmoreira: ignore relocations to .TOC and symbols with index 0
> - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks
> - joe: fix symbol use-after-frees
> - joe: fix remaining valgrind leak complaints (non-error paths only)
> - joe: checkpatch nits
>
> livepatch: Add klp-convert annotation helpers
> v2:
> - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to
> here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to
> include/linux/livepatch.h
> v3:
> - joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte
> align klp_module_reloc structures
>
> modpost: Integrate klp-convert
> v2:
> - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule
> doesn't do that.f
> - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to
> /bin/dash
> - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise
> arg-check inside if_changed_rule compares cmd_link_module and
> cmd_ld_ko_o.
> - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is
> true.
> - mbenes: remove modinfo call. LIVEPATCH_ in Makefile
> - jmoreira: split up: move the .livepatch file-based scheme for
> identifying livepatches to a previous patch, as it was required for
> correctly building Symbols.list there.
> v3:
> - joe: adjust rule_ld_ko_o to call echo-cmd
> - joe: rebase for v5.1
> - joe: align KLP prefix
>
> modpost: Add modinfo flag to livepatch modules
> v2:
> - jmoreira: fix modpost.c (add_livepatch_flag) to update module
> structure with livepatch flag and prevent modpost from breaking due to
> unresolved symbols
> v3:
> - joe: adjust modpost.c::get_modinfo() call for v5.0 version
>
> livepatch: Add sample livepatch module
> v3:
> - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag
>
> documentation: Update on livepatch elf format
> v2:
> - jmoreira: update module to use KLP_SYMPOS
> - jmoreira: Comments on symbol resolution scheme
> - jmoreira: Update Makefile
> - jmoreira: Remove MODULE_INFO statement
> - jmoreira: Changelog
> v3:
> - joe: rebase for v5.1
> - joe: code shuffle to better match original source file
>
> livepatch/selftests: add klp-convert
> v3:
> - joe: clarify sympos=0 and sympos=1..n
>
>
> And now the usual git cover-letter boilerplate...
>
> Joao Moreira (2):
> kbuild: Support for Symbols.list creation
> documentation: Update on livepatch elf format
>
> Joe Lawrence (1):
> livepatch/selftests: add klp-convert
>
> Josh Poimboeuf (5):
> livepatch: Create and include UAPI headers
> livepatch: Add klp-convert tool
> livepatch: Add klp-convert annotation helpers
> modpost: Integrate klp-convert
> livepatch: Add sample livepatch module
>
> Miroslav Benes (1):
> modpost: Add modinfo flag to livepatch modules
>
> .gitignore | 1 +
> Documentation/livepatch/livepatch.txt | 3 +
> Documentation/livepatch/module-elf-format.txt | 50 +-
> MAINTAINERS | 2 +
> Makefile | 30 +-
> include/linux/livepatch.h | 13 +
> include/uapi/linux/livepatch.h | 22 +
> kernel/livepatch/core.c | 4 +-
> lib/livepatch/Makefile | 15 +
> lib/livepatch/test_klp_atomic_replace.c | 1 -
> lib/livepatch/test_klp_callbacks_demo.c | 1 -
> lib/livepatch/test_klp_callbacks_demo2.c | 1 -
> lib/livepatch/test_klp_convert1.c | 106 +++
> lib/livepatch/test_klp_convert2.c | 103 +++
> lib/livepatch/test_klp_convert_mod_a.c | 25 +
> lib/livepatch/test_klp_convert_mod_b.c | 13 +
> lib/livepatch/test_klp_livepatch.c | 1 -
> samples/livepatch/Makefile | 7 +
> .../livepatch/livepatch-annotated-sample.c | 102 +++
> samples/livepatch/livepatch-sample.c | 1 -
> scripts/Kbuild.include | 4 +-
> scripts/Makefile | 1 +
> scripts/Makefile.build | 7 +
> scripts/Makefile.modpost | 24 +-
> scripts/livepatch/.gitignore | 1 +
> scripts/livepatch/Makefile | 7 +
> scripts/livepatch/elf.c | 753 ++++++++++++++++++
> scripts/livepatch/elf.h | 72 ++
> scripts/livepatch/klp-convert.c | 692 ++++++++++++++++
> scripts/livepatch/klp-convert.h | 39 +
> scripts/livepatch/list.h | 391 +++++++++
> scripts/mod/modpost.c | 82 +-
> scripts/mod/modpost.h | 1 +
> .../selftests/livepatch/test-livepatch.sh | 64 ++
> 34 files changed, 2616 insertions(+), 23 deletions(-)
> create mode 100644 include/uapi/linux/livepatch.h
> create mode 100644 lib/livepatch/test_klp_convert1.c
> create mode 100644 lib/livepatch/test_klp_convert2.c
> create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
> create mode 100644 lib/livepatch/test_klp_convert_mod_b.c
> create mode 100644 samples/livepatch/livepatch-annotated-sample.c
> create mode 100644 scripts/livepatch/.gitignore
> create mode 100644 scripts/livepatch/Makefile
> create mode 100644 scripts/livepatch/elf.c
> create mode 100644 scripts/livepatch/elf.h
> create mode 100644 scripts/livepatch/klp-convert.c
> create mode 100644 scripts/livepatch/klp-convert.h
> create mode 100644 scripts/livepatch/list.h
>
> --
> 2.20.1
[ cc += Jason and Evgenii ]
Apologies for the long brain dump, but I promised to reply to this
thread with some of the larger TODO issues I came across with this
patchset. Static key support is probably the most complicated item, so
there is a lot of debugging output I can provide.
See the tl;dr and Future Work sections for the executive summary.
Static key support
==================
tl;dr
-----
Livepatch symbols are created when building livepatch modules that
reference the (non-exported) static keys of a target object.
When loading a livepatch that contains klp-converted static key symbols,
the module loader crashes.
Testing setup
-------------
The easiest way to see the source and repro is to grab this branch:
https://github.com/joe-lawrence/linux/commits/klp-convert-v4-static-keys
and note these last two commits:
- livepatch/klp-convert: abort on static key conversion: this will
prevent klp-convert from converting any relocations to the
__jump_table. Revert this commit to see the crash. If we don't
have a fix before merging, I would suggest a temporary abort
like this to avoid silently creating dangerous livepatches.
- livepatch/selftests: add livepatch static keys: this adds the
self-test which will repro the crash.
I added a new livepatch selftest to verify klp-convert and static key
behavior: it consists of two kernel modules: test_klp_static_keys_mod.ko
and livepatch module that patches it, test_klp_static_keys.ko.
The base module contains a few different types of static keys: default
true / false, exported / not-exported, and one created by the trace
event macro infrastructure.
The livepatch module references each of these, relying upon klp-convert.
This module also provides its own static key for reference.
test_klp_static_keys_mod.ko
---------------------------
module_key (false) - exported
module_key2 (true) <----
TRACE_EVENT(test_klp_static_keys) <-- |
| |
| | klp-convert resolves
test_klp_static_keys.ko - livepatch | | these references
----------------------------------- | |
klp_key (true) | |
test_klp_static_keys ----------------- |
module_key2 ----------------------------
module_key
Currently .klp symbols are created as well as a relocation section for those
symbols in the their corresponding .text locations.
test_klp_static_keys_mod.ko
---------------------------
% eu-readelf --symbols lib/livepatch/test_klp_static_keys_mod.ko | \
grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>'
49: 0000000000000010 16 OBJECT LOCAL DEFAULT 44 module_key
65: 0000000000000010 16 OBJECT LOCAL DEFAULT 32 module_key2
96: 0000000000000000 48 OBJECT GLOBAL DEFAULT 40 __tracepoint_test_klp_static_keys
test_klp_static_keys.klp.o
--------------------------
% eu-readelf --symbols lib/livepatch/test_klp_static_keys.klp.o | \
grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>'
71: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF __tracepoint_test_klp_static_keys
78: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key2
84: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key
Lots of __tracepoint_test_klp_static_keys relocations since each
module function registers an event when it is called:
% eu-readelf --reloc lib/livepatch/test_klp_static_keys.klp.o
Relocation section [ 4] '.rela.text' for section [ 3] '.text' at offset 0x2e7d8 contains 88 entries:
Offset Type Value Addend Name
...
0x000000000000002c X86_64_32S 000000000000000000 +0 module_key2
...
0x000000000000007c X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x00000000000000d9 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x0000000000000169 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x00000000000001f9 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x000000000000028c X86_64_32S 000000000000000000 +0 module_key
...
0x00000000000002dc X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x000000000000038c X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
...
0x00000000000003eb X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys
Relocations generated for the __jump_table itself, note that I grouped
each jump entry <code, target, key> relocation set:
Relocation section [19] '.rela__jump_table' for section [18] '__jump_table' at offset 0x2fb28 contains 30 entries:
Offset Type Value Addend Name
000000000000000000 X86_64_PC32 000000000000000000 +12 .text
0x0000000000000004 X86_64_PC32 000000000000000000 +102 .text
0x0000000000000008 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000010 X86_64_PC32 000000000000000000 +188 .text
0x0000000000000014 X86_64_PC32 000000000000000000 +195 .text
0x0000000000000018 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000020 X86_64_PC32 000000000000000000 +304 .text
0x0000000000000024 X86_64_PC32 000000000000000000 +0 .text.unlikely
0x0000000000000028 X86_64_PC64 000000000000000000 +16 .bss
0x0000000000000030 X86_64_PC32 000000000000000000 +332 .text
0x0000000000000034 X86_64_PC32 000000000000000000 +339 .text
0x0000000000000038 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000040 X86_64_PC32 000000000000000000 +448 .text
0x0000000000000044 X86_64_PC32 000000000000000000 +52 .text.unlikely
0x0000000000000048 X86_64_PC64 000000000000000000 +0 module_key
0x0000000000000050 X86_64_PC32 000000000000000000 +476 .text
0x0000000000000054 X86_64_PC32 000000000000000000 +483 .text
0x0000000000000058 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000060 X86_64_PC32 000000000000000000 +592 .text
0x0000000000000064 X86_64_PC32 000000000000000000 +104 .text.unlikely
0x0000000000000068 X86_64_PC64 000000000000000000 +1 module_key2
0x0000000000000070 X86_64_PC32 000000000000000000 +620 .text
0x0000000000000074 X86_64_PC32 000000000000000000 +710 .text
0x0000000000000078 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000080 X86_64_PC32 000000000000000000 +796 .text
0x0000000000000084 X86_64_PC32 000000000000000000 +886 .text
0x0000000000000088 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
0x0000000000000090 X86_64_PC32 000000000000000000 +962 .text
0x0000000000000094 X86_64_PC32 000000000000000000 +981 .text
0x0000000000000098 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys
test_klp_static_keys.ko
-----------------------
Finally klp-convert has transformed a bunch of symbols for us:
% eu-readelf --symbols lib/livepatch/test_klp_static_keys.ko | \
grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>'
71: 0000000000000000 0 NOTYPE GLOBAL DEFAULT LOOS+0 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
78: 0000000000000000 0 NOTYPE GLOBAL DEFAULT LOOS+0 .klp.sym.test_klp_static_keys_mod.module_key2,0
84: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key
% eu-readelf --reloc lib/livepatch/test_klp_static_keys.ko
Relocation section [19] '.rela__jump_table' for section [18] '__jump_table' at offset 0x2480 contains 30 entries:
Offset Type Value Addend Name
000000000000000000 X86_64_PC32 000000000000000000 +12 .text
0x0000000000000004 X86_64_PC32 000000000000000000 +102 .text
0x0000000000000008 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000010 X86_64_PC32 000000000000000000 +188 .text
0x0000000000000014 X86_64_PC32 000000000000000000 +195 .text
0x0000000000000018 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000020 X86_64_PC32 000000000000000000 +304 .text
0x0000000000000024 X86_64_PC32 000000000000000000 +0 .text.unlikely
0x0000000000000028 X86_64_PC64 000000000000000000 +16 .bss
0x0000000000000030 X86_64_PC32 000000000000000000 +332 .text
0x0000000000000034 X86_64_PC32 000000000000000000 +339 .text
0x0000000000000038 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000040 X86_64_PC32 000000000000000000 +448 .text
0x0000000000000044 X86_64_PC32 000000000000000000 +52 .text.unlikely
0x0000000000000048 X86_64_PC64 000000000000000000 +0 module_key
0x0000000000000050 X86_64_PC32 000000000000000000 +476 .text
0x0000000000000054 X86_64_PC32 000000000000000000 +483 .text
0x0000000000000058 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000060 X86_64_PC32 000000000000000000 +592 .text
0x0000000000000064 X86_64_PC32 000000000000000000 +104 .text.unlikely
0x0000000000000068 X86_64_PC64 000000000000000000 +1 .klp.sym.test_klp_static_keys_mod.module_key2,0
0x0000000000000070 X86_64_PC32 000000000000000000 +620 .text
0x0000000000000074 X86_64_PC32 000000000000000000 +710 .text
0x0000000000000078 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000080 X86_64_PC32 000000000000000000 +796 .text
0x0000000000000084 X86_64_PC32 000000000000000000 +886 .text
0x0000000000000088 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000090 X86_64_PC32 000000000000000000 +962 .text
0x0000000000000094 X86_64_PC32 000000000000000000 +981 .text
0x0000000000000098 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
And here's our final set of klp-converted relocations which include the
unexported trace point symbol and module_key2 symbols:
Relocation section [44] '.klp.rela.test_klp_static_keys_mod..text' for section [ 3] '.text' at offset 0x55c18 contains 12 entries:
Offset Type Value Addend Name
...
0x00000000000003eb X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x000000000000038c X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x00000000000002dc X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x00000000000001f9 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x0000000000000169 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x00000000000000d9 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x000000000000007c X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0
0x000000000000002c X86_64_32S 000000000000000000 +0 .klp.sym.test_klp_static_keys_mod.module_key2,0
...
Current behavior
----------------
Not good. The livepatch successfully builds but crashes on load:
% insmod lib/livepatch/test_klp_static_keys_mod.ko
% insmod lib/livepatch/test_klp_static_keys.ko
BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 9367 Comm: insmod Tainted: G E K 5.1.0-rc4+ #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
RIP: 0010:jump_label_apply_nops+0x3b/0x60
Code: 02 00 00 48 c1 e5 04 48 01 dd 48 39 eb 74 3a 72 0b eb 36 48 83 c3 10 48 39 dd 76 2d 48 8b 43 08 48 89 c2 83 e0 01 48 83 e2 fc <48> 8b 54 13 10 83 e2 01 38 c2 75 dd 48 89 df 31 f6 48 83 c3 10 e8
RSP: 0018:ffffa8874068fcf8 EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffffffffc07fd000 RCX: 000000000000000d
RDX: 000000003f803000 RSI: ffffffffa5077be0 RDI: ffffffffc07fe540
RBP: ffffffffc07fd0a0 R08: ffffa88740f43878 R09: ffffa88740eed000
R10: 0000000000055a4b R11: ffffa88740f43878 R12: ffffa88740f430b8
R13: 0000000000000000 R14: ffffa88740f42df8 R15: 0000000000042b01
FS: 00007f4f1dafb740(0000) GS:ffff9a81fbb80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 00000000b5d8a000 CR4: 00000000000006e0
Call Trace:
module_finalize+0x184/0x1c0
load_module+0x1400/0x1910
? kernel_read_file+0x18d/0x1c0
? __do_sys_finit_module+0xa8/0x110
__do_sys_finit_module+0xa8/0x110
do_syscall_64+0x55/0x1a0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f4f1cae82bd
Future work
-----------
At the very least, I think this call-chain ordering is wrong for
livepatch static key symbols:
load_module
apply_relocations
post_relocation
module_finalize
jump_label_apply_nops <<
...
prepare_coming_module
blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod);
jump_label_module_notify
case MODULE_STATE_COMING
jump_label_add_module <<
do_init_module
do_one_initcall(mod->init)
__init patch_init [kpatch-patch]
klp_register_patch
klp_init_patch
klp_for_each_object(patch, obj)
klp_init_object
klp_init_object_loaded
klp_write_object_relocations <<
blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_LIVE, mod);
jump_label_module_notify
case MODULE_STATE_LIVE
jump_label_invalidate_module_init
where klp_write_object_relocations() is called way *after*
jump_label_apply_nops() and jump_label_add_module().
Detection
---------
I have been tinkering with some prototype code to defer
jump_label_apply_nops() and jump_label_add_module(), but it has been
slow going. I think the jist of it is that we're going to need to call
these dynamically when individual klp_objects are patched, not when the
livepatch module itself loads. If anyone with static key expertise
wants to jump in here, let me know.
In the meantime, I cooked up a potential followup commit to detect
conversion of static key symbols and klp-convert failure. It basically
runs through the output .ko's ELF symbols and verifies that none of the
converted ones can be found as a .rela__jump_table relocated symbol. It
accurately catches the problematic references in test_klp_static_keys.ko
thus far.
This was based on a similar issue reported as a bug against
kpatch-build, in which Josh wrote code to detect this scenario:
https://github.com/dynup/kpatch/issues/946
https://github.com/jpoimboe/kpatch/commit/2cd2d27607566aee9590c367e615207ce1ce24c6
I can post ("livepatch/klp-convert: abort on static key conversion")
here as a follow commit if it looks reasonable and folks wish to review
it... or we can try and tackle static keys before merging klp-convert.
-- Joe
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> Hi folks,
>
> This is the third installment of the klp-convert tool for generating and
> processing livepatch symbols for livepatch module builds. For those
> following along at home, archive links to previous versions:
>
> RFC:
> https://lore.kernel.org/lkml/[email protected]/
> v2:
> https://lore.kernel.org/lkml/[email protected]/
>
> (Note that I don't see v2 archived at lore, but that is a link to the
> most recent subthread that lore did catch.)
>
>
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols are
> not exported, solving this relocation requires information on the object
> that holds the symbol (either vmlinux or modules) and its position inside
> the object, as an object may contain multiple symbols with the same name.
> Providing such information must be done accordingly to what is specified
> in Documentation/livepatch/module-elf-format.txt.
>
Could we get some details with examples or a sample, sorry I might be dense
and missing simple information. The problem being solved is not clear to
me from the changelog.
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem in two different forms: (i) by relying on a symbol map, which is
> built during kernel compilation, to automatically infer the relocation
> targeted symbol, and, when such inference is not possible (ii) by using
> annotations in the elf object to convert the relocation accordingly to
> the specification, enabling it to be handled by the livepatch loader.
>
> Given the above, add support for symbol mapping in the form of
> Symbols.list file; add klp-convert tool; integrate klp-convert tool into
> kbuild; make livepatch modules discernible during kernel compilation
> pipeline; add data-structure and macros to enable users to annotate
> livepatch source code; make modpost stage compatible with livepatches;
> update livepatch-sample and update documentation.
>
> The patch was tested under three use-cases:
>
> use-case 1: There is a relocation in the lp that can be automatically
> resolved by klp-convert. For example. see the saved_command_line
> variable in lib/livepatch/test_klp_convert2.c.
>
> use-case 2: There is a relocation in the lp that cannot be automatically
> resolved, as the name of the respective symbol appears in multiple
> objects. The livepatch contains an annotation to enable a correct
> relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> in lib/livepatch/test_klp_convert{1,2}.c.
>
> use-case 3: There is a relocation in the lp that cannot be automatically
> resolved similarly as 2, but no annotation was provided in the
> livepatch, triggering an error during compilation. Reproducible by
> removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> lib/livepatch/test_klp_convert{1,2}.c.
>
Are these a part of the series?
Balbir Singh
>
> Branches
> --------
>
> Since I spent some time tinkering with v2 and accumulated a bunch of
> fixes, I collected them up and am posting this new version. For review
> purposes, I posted two branches up to github:
>
> 1 - an expanded branch that with changes separate from the original
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded
>
> 2 - a squashed branch of (1) that comprises v3:
> https://github.com/joe-lawrence/linux/commits/klp-convert-v3
>
> Non-trivial commits in the expanded branch have some extra commentary
> and details for debugging in the commit message that were dropped when
> squashing into their respective parent commits.
>
>
> TODO
> ----
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
>
>
> Changelogs
> ----------
>
> The commit changelogs were getting long, so I've moved them here for
> posterity and review purposes:
>
> livepatch: Create and include UAPI headers
> v2:
> - jmoreira: split up and changelog
> v3:
> - joe: convert to SPDX license tags
>
> kbuild: Support for Symbols.list creation
> v3:
> - jmoreira: adjust for multiobject livepatch
> - joe: add klpclean to PHONY
> - joe: align KLP prefix
>
> livepatch: Add klp-convert tool
> v2:
> - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be
> at the end
> - jmoreira: add support to automatic relocation conversion in
> klp-convert.c, changelog
> v3:
> - joe: convert to SPDX license tags
> - jmoreira: add rela symbol name to WARNs
> - jmoreira: ignore relocations to .TOC and symbols with index 0
> - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks
> - joe: fix symbol use-after-frees
> - joe: fix remaining valgrind leak complaints (non-error paths only)
> - joe: checkpatch nits
>
> livepatch: Add klp-convert annotation helpers
> v2:
> - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to
> here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to
> include/linux/livepatch.h
> v3:
> - joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte
> align klp_module_reloc structures
>
> modpost: Integrate klp-convert
> v2:
> - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule
> doesn't do that.f
> - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to
> /bin/dash
> - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise
> arg-check inside if_changed_rule compares cmd_link_module and
> cmd_ld_ko_o.
> - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is
> true.
> - mbenes: remove modinfo call. LIVEPATCH_ in Makefile
> - jmoreira: split up: move the .livepatch file-based scheme for
> identifying livepatches to a previous patch, as it was required for
> correctly building Symbols.list there.
> v3:
> - joe: adjust rule_ld_ko_o to call echo-cmd
> - joe: rebase for v5.1
> - joe: align KLP prefix
>
> modpost: Add modinfo flag to livepatch modules
> v2:
> - jmoreira: fix modpost.c (add_livepatch_flag) to update module
> structure with livepatch flag and prevent modpost from breaking due to
> unresolved symbols
> v3:
> - joe: adjust modpost.c::get_modinfo() call for v5.0 version
>
> livepatch: Add sample livepatch module
> v3:
> - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag
>
> documentation: Update on livepatch elf format
> v2:
> - jmoreira: update module to use KLP_SYMPOS
> - jmoreira: Comments on symbol resolution scheme
> - jmoreira: Update Makefile
> - jmoreira: Remove MODULE_INFO statement
> - jmoreira: Changelog
> v3:
> - joe: rebase for v5.1
> - joe: code shuffle to better match original source file
>
> livepatch/selftests: add klp-convert
> v3:
> - joe: clarify sympos=0 and sympos=1..n
>
>
> And now the usual git cover-letter boilerplate...
>
> Joao Moreira (2):
> kbuild: Support for Symbols.list creation
> documentation: Update on livepatch elf format
>
> Joe Lawrence (1):
> livepatch/selftests: add klp-convert
>
> Josh Poimboeuf (5):
> livepatch: Create and include UAPI headers
> livepatch: Add klp-convert tool
> livepatch: Add klp-convert annotation helpers
> modpost: Integrate klp-convert
> livepatch: Add sample livepatch module
>
> Miroslav Benes (1):
> modpost: Add modinfo flag to livepatch modules
>
> .gitignore | 1 +
> Documentation/livepatch/livepatch.txt | 3 +
> Documentation/livepatch/module-elf-format.txt | 50 +-
> MAINTAINERS | 2 +
> Makefile | 30 +-
> include/linux/livepatch.h | 13 +
> include/uapi/linux/livepatch.h | 22 +
> kernel/livepatch/core.c | 4 +-
> lib/livepatch/Makefile | 15 +
> lib/livepatch/test_klp_atomic_replace.c | 1 -
> lib/livepatch/test_klp_callbacks_demo.c | 1 -
> lib/livepatch/test_klp_callbacks_demo2.c | 1 -
> lib/livepatch/test_klp_convert1.c | 106 +++
> lib/livepatch/test_klp_convert2.c | 103 +++
> lib/livepatch/test_klp_convert_mod_a.c | 25 +
> lib/livepatch/test_klp_convert_mod_b.c | 13 +
> lib/livepatch/test_klp_livepatch.c | 1 -
> samples/livepatch/Makefile | 7 +
> .../livepatch/livepatch-annotated-sample.c | 102 +++
> samples/livepatch/livepatch-sample.c | 1 -
> scripts/Kbuild.include | 4 +-
> scripts/Makefile | 1 +
> scripts/Makefile.build | 7 +
> scripts/Makefile.modpost | 24 +-
> scripts/livepatch/.gitignore | 1 +
> scripts/livepatch/Makefile | 7 +
> scripts/livepatch/elf.c | 753 ++++++++++++++++++
> scripts/livepatch/elf.h | 72 ++
> scripts/livepatch/klp-convert.c | 692 ++++++++++++++++
> scripts/livepatch/klp-convert.h | 39 +
> scripts/livepatch/list.h | 391 +++++++++
> scripts/mod/modpost.c | 82 +-
> scripts/mod/modpost.h | 1 +
> .../selftests/livepatch/test-livepatch.sh | 64 ++
> 34 files changed, 2616 insertions(+), 23 deletions(-)
> create mode 100644 include/uapi/linux/livepatch.h
> create mode 100644 lib/livepatch/test_klp_convert1.c
> create mode 100644 lib/livepatch/test_klp_convert2.c
> create mode 100644 lib/livepatch/test_klp_convert_mod_a.c
> create mode 100644 lib/livepatch/test_klp_convert_mod_b.c
> create mode 100644 samples/livepatch/livepatch-annotated-sample.c
> create mode 100644 scripts/livepatch/.gitignore
> create mode 100644 scripts/livepatch/Makefile
> create mode 100644 scripts/livepatch/elf.c
> create mode 100644 scripts/livepatch/elf.h
> create mode 100644 scripts/livepatch/klp-convert.c
> create mode 100644 scripts/livepatch/klp-convert.h
> create mode 100644 scripts/livepatch/list.h
>
> --
> 2.20.1
On Tue, 16 Apr 2019, Balbir Singh wrote:
> On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
> > Hi folks,
> >
> > This is the third installment of the klp-convert tool for generating and
> > processing livepatch symbols for livepatch module builds. For those
> > following along at home, archive links to previous versions:
> >
> > RFC:
> > https://lore.kernel.org/lkml/[email protected]/
> > v2:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > (Note that I don't see v2 archived at lore, but that is a link to the
> > most recent subthread that lore did catch.)
> >
> >
> > Livepatches may use symbols which are not contained in its own scope,
> > and, because of that, may end up compiled with relocations that will
> > only be resolved during module load. Yet, when the referenced symbols are
> > not exported, solving this relocation requires information on the object
> > that holds the symbol (either vmlinux or modules) and its position inside
> > the object, as an object may contain multiple symbols with the same name.
> > Providing such information must be done accordingly to what is specified
> > in Documentation/livepatch/module-elf-format.txt.
> >
>
>
> Could we get some details with examples or a sample, sorry I might be dense
> and missing simple information. The problem being solved is not clear to
> me from the changelog.
The patch set tries to solve the problem described in
Documentation/livepatch/module-elf-format.txt. Currently, there is no tool
in upstream to automatically generate the relocation records. kpatch-build
can do it and we'd like to have it as well. klp-convert should thus do the
job.
The sample module is introduced in patch 7 and you can look at the
selftests in patch 9 too.
Maybe we can describe the problem better in the cover letter, but on the
other hand patch changelogs are more important and they are good (well, in
my opinion of course).
> > Currently, there is no trivial way to embed the required information as
> > requested in the final livepatch elf object. klp-convert solves this
> > problem in two different forms: (i) by relying on a symbol map, which is
> > built during kernel compilation, to automatically infer the relocation
> > targeted symbol, and, when such inference is not possible (ii) by using
> > annotations in the elf object to convert the relocation accordingly to
> > the specification, enabling it to be handled by the livepatch loader.
> >
> > Given the above, add support for symbol mapping in the form of
> > Symbols.list file; add klp-convert tool; integrate klp-convert tool into
> > kbuild; make livepatch modules discernible during kernel compilation
> > pipeline; add data-structure and macros to enable users to annotate
> > livepatch source code; make modpost stage compatible with livepatches;
> > update livepatch-sample and update documentation.
> >
> > The patch was tested under three use-cases:
> >
> > use-case 1: There is a relocation in the lp that can be automatically
> > resolved by klp-convert. For example. see the saved_command_line
> > variable in lib/livepatch/test_klp_convert2.c.
> >
> > use-case 2: There is a relocation in the lp that cannot be automatically
> > resolved, as the name of the respective symbol appears in multiple
> > objects. The livepatch contains an annotation to enable a correct
> > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections
> > in lib/livepatch/test_klp_convert{1,2}.c.
> >
> > use-case 3: There is a relocation in the lp that cannot be automatically
> > resolved similarly as 2, but no annotation was provided in the
> > livepatch, triggering an error during compilation. Reproducible by
> > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in
> > lib/livepatch/test_klp_convert{1,2}.c.
> >
>
>
> Are these a part of the series?
Yes, see 9/9.
Regards,
Miroslav
[...]
> Current behavior
> ----------------
>
> Not good. The livepatch successfully builds but crashes on load:
>
> % insmod lib/livepatch/test_klp_static_keys_mod.ko
> % insmod lib/livepatch/test_klp_static_keys.ko
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> #PF error: [normal kernel read fault]
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 3 PID: 9367 Comm: insmod Tainted: G E K 5.1.0-rc4+ #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
> RIP: 0010:jump_label_apply_nops+0x3b/0x60
> Code: 02 00 00 48 c1 e5 04 48 01 dd 48 39 eb 74 3a 72 0b eb 36 48 83 c3 10 48 39 dd 76 2d 48 8b 43 08 48 89 c2 83 e0 01 48 83 e2 fc <48> 8b 54 13 10 83 e2 01 38 c2 75 dd 48 89 df 31 f6 48 83 c3 10 e8
> RSP: 0018:ffffa8874068fcf8 EFLAGS: 00010206
> RAX: 0000000000000000 RBX: ffffffffc07fd000 RCX: 000000000000000d
> RDX: 000000003f803000 RSI: ffffffffa5077be0 RDI: ffffffffc07fe540
> RBP: ffffffffc07fd0a0 R08: ffffa88740f43878 R09: ffffa88740eed000
> R10: 0000000000055a4b R11: ffffa88740f43878 R12: ffffa88740f430b8
> R13: 0000000000000000 R14: ffffa88740f42df8 R15: 0000000000042b01
> FS: 00007f4f1dafb740(0000) GS:ffff9a81fbb80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 00000000b5d8a000 CR4: 00000000000006e0
> Call Trace:
> module_finalize+0x184/0x1c0
> load_module+0x1400/0x1910
> ? kernel_read_file+0x18d/0x1c0
> ? __do_sys_finit_module+0xa8/0x110
> __do_sys_finit_module+0xa8/0x110
> do_syscall_64+0x55/0x1a0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f4f1cae82bd
>
>
> Future work
> -----------
>
> At the very least, I think this call-chain ordering is wrong for
> livepatch static key symbols:
>
> load_module
>
> apply_relocations
>
> post_relocation
> module_finalize
> jump_label_apply_nops <<
>
> ...
>
> prepare_coming_module
> blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod);
> jump_label_module_notify
> case MODULE_STATE_COMING
> jump_label_add_module <<
>
> do_init_module
>
> do_one_initcall(mod->init)
> __init patch_init [kpatch-patch]
> klp_register_patch
> klp_init_patch
> klp_for_each_object(patch, obj)
> klp_init_object
> klp_init_object_loaded
> klp_write_object_relocations <<
>
> blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_LIVE, mod);
> jump_label_module_notify
> case MODULE_STATE_LIVE
> jump_label_invalidate_module_init
>
> where klp_write_object_relocations() is called way *after*
> jump_label_apply_nops() and jump_label_add_module().
Quick look, but it seems quite similar to the problem we had with
apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
introduced it.
I think, we should do the same for jump labels. Add
jump_label_apply_nops() from module_finalize() to
arch_klp_init_object_loaded() and convert jump_table ELF section so its
processing is delayed.
Which leads me another TODO... klp-convert does not convert even
.altinstructions and .parainstructions sections, so it has that problem as
well. If I remember, it was on Josh's TODO list when he first introduced
klp-convert. See [email protected].
The selftest for the alternatives would be appreciated too. One day.
And of course we should look at the other supported architectures and
their module_finalize() functions. I have it on my TODO list somewhere,
but you know how it works with those :/. I am sure there are more hidden
surprises there.
> Detection
> ---------
>
> I have been tinkering with some prototype code to defer
> jump_label_apply_nops() and jump_label_add_module(), but it has been
> slow going. I think the jist of it is that we're going to need to call
> these dynamically when individual klp_objects are patched, not when the
> livepatch module itself loads. If anyone with static key expertise
> wants to jump in here, let me know.
>
> In the meantime, I cooked up a potential followup commit to detect
> conversion of static key symbols and klp-convert failure. It basically
> runs through the output .ko's ELF symbols and verifies that none of the
> converted ones can be found as a .rela__jump_table relocated symbol. It
> accurately catches the problematic references in test_klp_static_keys.ko
> thus far.
>
> This was based on a similar issue reported as a bug against
> kpatch-build, in which Josh wrote code to detect this scenario:
>
> https://github.com/dynup/kpatch/issues/946
> https://github.com/jpoimboe/kpatch/commit/2cd2d27607566aee9590c367e615207ce1ce24c6
>
> I can post ("livepatch/klp-convert: abort on static key conversion")
> here as a follow commit if it looks reasonable and folks wish to review
> it... or we can try and tackle static keys before merging klp-convert.
Good idea. I'd rather fix it, but I think it could be a lot of work, so
something like this patch seems to be a good idea.
Thanks
Miroslav
On 4/16/19 1:24 AM, Balbir Singh wrote:
>
> Could we get some details with examples or a sample, sorry I might be dense
> and missing simple information. The problem being solved is not clear to
> me from the changelog.
>
Hi Balbir,
I see that Miroslav has already pointed to documentation, samples and
self-tests for the patchset, but here is a summary in my own words:
kpatch-build constructs livepatch kernel modules by comparing a
reference build with a patched build, combing through ELF object
sections and extracting new and changed sections that include patched code.
An alternative approach is "source-based", in which a livepatch kernel
module is built (mostly entirely) using the kernel's build
infrastructure. Sources for a livepatch are gathered ahead of time and
then built like an ordinary kernel module.
In either approach, there lies the problem of symbol visibility: how can
a livepatch resolve symbols that a kernel module ordinarily can't. For
example, file local or simply unexported symbols in across the kernel
and other modules. Enter the concept of "livepatch symbols" described
in module-elf-format.txt.
kpatch-build already creates such "livepatch symbols" (see its
create_klp_relasecs_and_syms()) and the livepatching core kernel code
already knows how resolve such symbols at klp_object patch time (see
klp_write_object_relocations()).
The klp-convert tool and this supporting patchset would empower
source-based-constructed livepatch modules to do the same.
-- Joe
On 4/10/19 11:50 AM, Joe Lawrence wrote:
> Hi folks,
>
> This is the third installment of the klp-convert tool for generating and
> processing livepatch symbols for livepatch module builds. For those
> following along at home, archive links to previous versions:
>
> RFC:
> https://lore.kernel.org/lkml/[email protected]/
> v2:
> https://lore.kernel.org/lkml/[email protected]/
>
> (Note that I don't see v2 archived at lore, but that is a link to the
> most recent subthread that lore did catch.)
>
>
> Livepatches may use symbols which are not contained in its own scope,
> and, because of that, may end up compiled with relocations that will
> only be resolved during module load. Yet, when the referenced symbols are
> not exported, solving this relocation requires information on the object
> that holds the symbol (either vmlinux or modules) and its position inside
> the object, as an object may contain multiple symbols with the same name.
> Providing such information must be done accordingly to what is specified
> in Documentation/livepatch/module-elf-format.txt.
Hi Miroslav,
I noticed that some binutils programs like gdb, objdump, etc. don't like
the .ko kernel objects that we're generating from this patchset,
specifically those with the additional '.klp.rela.<obj>..text' livepatch
symbol relocation sections.
For reference, I opened a new bugzilla with more details here:
https://sourceware.org/bugzilla/show_bug.cgi?id=24456
And was about to ping the binutils mailing list about the assertion that
is tripping in bfd/elf.c. The thought occurred to me that you guys
might already be carrying a patch to workaround this issue?
-- Joe
On 4/11/19 3:04 PM, Miroslav Benes wrote:
>
>> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
>> index 2472ce39a18d..8b9b42a258ad 100644
>> --- a/samples/livepatch/Makefile
>> +++ b/samples/livepatch/Makefile
>> @@ -1,3 +1,4 @@
>> +LIVEPATCH_livepatch-sample := y
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
>> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
>
> We discussed this and I think there has been no conclusion yet. Shouldn't
> we do the same (add LIVEPATCH_) for the other samples here as well?
In v2 review, I mentioned moving this hunk to ("modpost: Add modinfo
flag to livepatch modules") along with the same change to the other
sample modules. I didn't apply that for v3, but can do so for the next
spin if it makes the commits easier to review.
-- Joe
On Tue, 16 Apr 2019, Joe Lawrence wrote:
> On 4/11/19 3:04 PM, Miroslav Benes wrote:
> >
> >> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> >> index 2472ce39a18d..8b9b42a258ad 100644
> >> --- a/samples/livepatch/Makefile
> >> +++ b/samples/livepatch/Makefile
> >> @@ -1,3 +1,4 @@
> >> +LIVEPATCH_livepatch-sample := y
> >> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
> >> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
> >> obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
> >
> > We discussed this and I think there has been no conclusion yet. Shouldn't
> > we do the same (add LIVEPATCH_) for the other samples here as well?
>
> In v2 review, I mentioned moving this hunk to ("modpost: Add modinfo flag to
> livepatch modules") along with the same change to the other sample modules. I
> didn't apply that for v3, but can do so for the next spin if it makes the
> commits easier to review.
I think it belongs here, but I may be missing something. Anyway, it is not
really important, so I'd leave it up to you.
Miroslav
On Tue, 16 Apr 2019, Joe Lawrence wrote:
> On 4/10/19 11:50 AM, Joe Lawrence wrote:
> > Hi folks,
> >
> > This is the third installment of the klp-convert tool for generating and
> > processing livepatch symbols for livepatch module builds. For those
> > following along at home, archive links to previous versions:
> >
> > RFC:
> > https://lore.kernel.org/lkml/[email protected]/
> > v2:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > (Note that I don't see v2 archived at lore, but that is a link to the
> > most recent subthread that lore did catch.)
> >
> >
> > Livepatches may use symbols which are not contained in its own scope,
> > and, because of that, may end up compiled with relocations that will
> > only be resolved during module load. Yet, when the referenced symbols are
> > not exported, solving this relocation requires information on the object
> > that holds the symbol (either vmlinux or modules) and its position inside
> > the object, as an object may contain multiple symbols with the same name.
> > Providing such information must be done accordingly to what is specified
> > in Documentation/livepatch/module-elf-format.txt.
>
> Hi Miroslav,
>
> I noticed that some binutils programs like gdb, objdump, etc. don't like the
> .ko kernel objects that we're generating from this patchset, specifically
> those with the additional '.klp.rela.<obj>..text' livepatch symbol relocation
> sections.
>
> For reference, I opened a new bugzilla with more details here:
> https://sourceware.org/bugzilla/show_bug.cgi?id=24456
Another great catch.
> And was about to ping the binutils mailing list about the assertion that is
> tripping in bfd/elf.c. The thought occurred to me that you guys might already
> be carrying a patch to workaround this issue?
No, unfortunately we don't. At least I don't know about anything and a
quick test on openSUSE Leap 15.0 confirms it (objdump -D gives "bad
value").
Thanks
Miroslav
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> TODO
> ----
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
Multiple object files
=====================
For v3, we tweaked the build scripts so that we could successfully build
a klp-converted livepatch module from multiple object files.
One interesting use-case is supporting two livepatch symbols of the same
name, but different object/position values.
An example target kernel module might be layed out like this:
test_klp_convert_mod.ko << target module is comprised of
two object files, each defining
test_klp_convert_mod_a.o their own local get_homonym_string()
get_homonym_string() function and homonym_string[]
homonym_string[] character arrays.
test_klp_convert_mod_b.o
get_homonym_string()
homonym_string[]
A look at interesting parts of the the module's symbol table:
% eu-readelf --symbols lib/livepatch/test_klp_convert_mod.ko | \
grep -e 'homonym' -e test_klp_convert_mod_
29: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_a.c
32: 0000000000000010 8 FUNC LOCAL DEFAULT 3 get_homonym_string
33: 0000000000000000 17 OBJECT LOCAL DEFAULT 5 homonym_string
37: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_b.c
38: 0000000000000020 8 FUNC LOCAL DEFAULT 3 get_homonym_string
39: 0000000000000000 17 OBJECT LOCAL DEFAULT 11 homonym_string
suggests that any livepatch module that wishes to resolve to
test_klp_convert_mod_a.c :: get_homonym_string() should include an
annotation like:
file_a.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_a[] = {
KLP_SYMPOS(get_homonym_string, 1),
};
and to resolve test_klp_convert_mod_b.c :: get_homonym_string():
file_b.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_b[] = {
KLP_SYMPOS(get_homonym_string, 2),
};
Unfortunately klp-convert v3 will fail to build such livepatch module,
regardless of sympos values:
klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,2 vs. vmlinux.state_show,1.
klp-convert: Unable to load user-provided sympos
make[1]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert_multi_objs.ko] Error 255
This abort message can be traced to sympos_sanity_check() as it verifies
that there no duplicate symbol names found in its list of user specified
symbols (ie, those needing to be converted).
Common sympos
-------------
New test case and related fixup can be found here:
https://github.com/joe-lawrence/linux/commits/klp-convert-v4-multiple-objs
To better debug this issue, I added another selftest that demonstrates
this configuration in isolation. "show_state" is a popular kernel
function name (my VM reported 5 instances in kallsyms) so I chose that
symbol name.
Initially I specified the *same* symbol position (1) in both .c file
KLP_SYMPOS annotations. At the very least, we can trivially patch
klp-convert v3 to support annotations for the the same symbol <object,
name, position> value across object files.
Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification: allow for duplicate <object, name, position>
instances. Now it will only complain when a supplied symbol references
the same <object, name> but a different <position>.
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..713835dfc9ec 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
}
}
-/* Checks if two or more elements in usr_symbols have the same name */
+/*
+ * Checks if two or more elements in usr_symbols have the same
+ * object and name, but different symbol position
+ */
static bool sympos_sanity_check(void)
{
bool sane = true;
@@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
list_for_each_entry(sp, &usr_symbols, list) {
aux = list_next_entry(sp, list);
list_for_each_entry_from(aux, &usr_symbols, list) {
- if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+ if (sp->pos != aux->pos &&
+ strcmp(sp->object_name, aux->object_name) == 0 &&
+ strcmp(sp->symbol_name, aux->symbol_name) == 0)
WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
sp->object_name, sp->symbol_name, sp->pos,
aux->object_name, aux->symbol_name, aux->pos);
Unique sympos
-------------
But even with the above workaround, specifying unique symbol positions
will (and should) result in a klp-convert complaint.
When modifying the test module with unique symbol position annotation
values (1 and 2 respectively):
test_klp_convert_multi_objs_a.c:
extern void *state_show;
...
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 1)
};
test_klp_convert_multi_objs_b.c:
extern void *state_show;
...
KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
KLP_SYMPOS(state_show, 2)
};
Each object file's symbol table contains a "state_show" instance:
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep '\<state_show\>'
30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
and the intermediate test_klp_convert_multi_objs.klp.o file contains a
combined .klp.module_relocs.vmlinux relocation section with two
KLP_SYMPOS structures, each with a unique <sympos> value:
% objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
0000000 0000 0000 0000 0000 0001 0000 0000 0000
0000010 0000 0000 0002 0000
but the symbol tables were merged, sorted and non-unique symbols
removed, leaving a *single* unresolved "state_show" instance:
% eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep '\<state_show\>'
53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
which means that even though we have separate relocations for each
"state_show" instance:
Relocation section [ 6] '.rela.text.unlikely' for section [ 5] '.text.unlikely' at offset 0x44510 contains 11 entries:
Offset Type Value Addend Name
0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show
...
0x0000000000000034 X86_64_32S 000000000000000000 +0 state_show
...
they share the same rela->sym and there is no way to distinguish which
one correlates to which KLP_SYMPOS structure.
Future Work
-----------
I don't see an easy way to support multiple homonym <object, name>
symbols with unique <position> values in the current livepatch module
Elf format. The only solutions that come to mind right now include
renaming homonym symbols somehow to retain the relocation->symbol
relationship when separate object files are combined. Perhaps an
intermediate linker step could make annotated symbols unique in some way
to achieve this. /thinking out loud
In the meantime, the unique symbol <position> case is already detected
and with a little tweaking we could support multiple common symbol
<position> values.
-- Joe
On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> +static bool convert_rela(struct section *oldsec, struct rela *r,
> + struct sympos *sp, struct elf *klp_elf)
> +{
> + struct section *sec;
> + struct rela *r1, *r2;
> +
> + sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
> + if (!sec) {
> + WARN("Can't create or access klp.rela section (%s.%s)\n",
> + sp->object_name, sp->symbol_name);
> + return false;
> + }
> +
> + if (!convert_klp_symbol(r->sym, sp)) {
> + WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
> + r->sym->name);
> + return false;
> + }
> +
> + /* Move the converted rela to klp rela section */
> + list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
> + if (r1->sym->name == r->sym->name) {
> + list_del(&r1->list);
> + list_add(&r1->list, &sec->relas);
> + }
> + }
> + return true;
> +}
This one took a while to find and debug, but I believe that
convert_rela()'s list removal is not as safe as it thinks it is.
Start with its calling context from main() below:
> + list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
> + if (!is_rela_section(sec))
> + continue;
> +
> + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> + if (!must_convert(rela->sym))
> + continue;
> +
> + if (!find_missing_position(rela->sym, &sp)) {
> + WARN("Unable to find missing symbol: %s",
> + rela->sym->name);
> + return -1;
> + }
> + if (!convert_rela(sec, rela, &sp, klp_elf)) {
> + WARN("Unable to convert relocation: %s",
> + rela->sym->name);
> + return -1;
> + }
> + }
> + }
AFAIK the *_safe list traversals, they cache the ->next value at the
beginning of each iteration, so that one could blow the current element
in position away. The cached ->next value is then assigned when moving
to the next iteration.
But notice how convert_rela() looks through the entire list of
relocations, moving each rela with a matching symbol?
Consider a slight tweak to samples/livepatch-annotated.c:
static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
{
+ if (saved_command_line)
+ saved_command_line[0] = '\0';
+
seq_printf(m, "%s livepatch=1\n", saved_command_line);
return 0;
}
On my system, this generates relocations like this:
% eu-readelf --relocs samples/livepatch/livepatch-annotated-sample.o
Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x98 contains 9 entries:
Offset Type Value Addend Name
0x0000000000000001 X86_64_PC32 000000000000000000 -4 __fentry__
0x0000000000000008 X86_64_PC32 000000000000000000 -4 saved_command_line
0x0000000000000017 X86_64_PC32 000000000000000000 -4 saved_command_line
0x000000000000001e X86_64_32S 000000000000000000 +0 .rodata.str1.1
0x0000000000000023 X86_64_PC32 000000000000000000 -4 seq_printf
0x0000000000000031 X86_64_PC32 000000000000000000 -4 __fentry__
0x0000000000000038 X86_64_32S 000000000000000000 +0 .data
0x0000000000000051 X86_64_PC32 000000000000000000 -4 __fentry__
0x000000000000003d X86_64_PC32 000000000000000000 -4 klp_enable_patch
We now have back-to-back rela's with sym->name = "saved_command_line".
When the first is converted, convert_rela() will move both of them to
the klp rela section. The linked list values may be consistent, but the
cached ->next value will be bogus and the in-flight-traversal will run
off the rails.
I think we can work around it with a combination of 1) only moving a
single rela symbol at a time in convert_rela and 2) processing the
second (third, etc.) a little bit more so that they are moved
individually.
I hacked this together and it works against the livepatch-annotate.c
test above so far...
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..126395f1c0cd 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
if (r1->sym->name == r->sym->name) {
list_del(&r1->list);
list_add(&r1->list, &sec->relas);
+ break;
}
}
return true;
@@ -549,8 +550,8 @@ static bool is_converted(char *sname)
}
/*
- * Checks if symbol must be converted (conditions):
- * not resolved, not already converted or isn't an exported symbol
+ * Checks if symbol must be or was already converted (conditions):
+ * not resolved or isn't an exported symbol
*/
static bool must_convert(struct symbol *sym)
{
@@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym)
if (strcmp(sym->name, ".TOC.") == 0)
return false;
- return (!(is_converted(sym->name) || is_exported(sym->name)));
+ return (!is_exported(sym->name));
}
/* Checks if a section is a klp rela section */
@@ -640,7 +641,8 @@ int main(int argc, const char **argv)
if (!must_convert(rela->sym))
continue;
- if (!find_missing_position(rela->sym, &sp)) {
+ if (!is_converted(rela->sym->name) &&
+ !find_missing_position(rela->sym, &sp)) {
WARN("Unable to find missing symbol: %s",
rela->sym->name);
return -1;
--
2.20.1
On Tue, 23 Apr 2019, Joe Lawrence wrote:
> On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
> >
> > [ ... snip ... ]
> >
> > +static bool convert_rela(struct section *oldsec, struct rela *r,
> > + struct sympos *sp, struct elf *klp_elf)
> > +{
> > + struct section *sec;
> > + struct rela *r1, *r2;
> > +
> > + sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
> > + if (!sec) {
> > + WARN("Can't create or access klp.rela section (%s.%s)\n",
> > + sp->object_name, sp->symbol_name);
> > + return false;
> > + }
> > +
> > + if (!convert_klp_symbol(r->sym, sp)) {
> > + WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
> > + r->sym->name);
> > + return false;
> > + }
> > +
> > + /* Move the converted rela to klp rela section */
> > + list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
> > + if (r1->sym->name == r->sym->name) {
> > + list_del(&r1->list);
> > + list_add(&r1->list, &sec->relas);
> > + }
> > + }
> > + return true;
> > +}
>
> This one took a while to find and debug, but I believe that
> convert_rela()'s list removal is not as safe as it thinks it is.
>
> Start with its calling context from main() below:
>
> > + list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
> > + if (!is_rela_section(sec))
> > + continue;
> > +
> > + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> > + if (!must_convert(rela->sym))
> > + continue;
> > +
> > + if (!find_missing_position(rela->sym, &sp)) {
> > + WARN("Unable to find missing symbol: %s",
> > + rela->sym->name);
> > + return -1;
> > + }
> > + if (!convert_rela(sec, rela, &sp, klp_elf)) {
> > + WARN("Unable to convert relocation: %s",
> > + rela->sym->name);
> > + return -1;
> > + }
> > + }
> > + }
>
> AFAIK the *_safe list traversals, they cache the ->next value at the
> beginning of each iteration, so that one could blow the current element
> in position away. The cached ->next value is then assigned when moving
> to the next iteration.
>
> But notice how convert_rela() looks through the entire list of
> relocations, moving each rela with a matching symbol?
>
> Consider a slight tweak to samples/livepatch-annotated.c:
>
>
> static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
> {
> + if (saved_command_line)
> + saved_command_line[0] = '\0';
> +
> seq_printf(m, "%s livepatch=1\n", saved_command_line);
> return 0;
> }
>
>
> On my system, this generates relocations like this:
>
> % eu-readelf --relocs samples/livepatch/livepatch-annotated-sample.o
> Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x98 contains 9 entries:
> Offset Type Value Addend Name
> 0x0000000000000001 X86_64_PC32 000000000000000000 -4 __fentry__
> 0x0000000000000008 X86_64_PC32 000000000000000000 -4 saved_command_line
> 0x0000000000000017 X86_64_PC32 000000000000000000 -4 saved_command_line
> 0x000000000000001e X86_64_32S 000000000000000000 +0 .rodata.str1.1
> 0x0000000000000023 X86_64_PC32 000000000000000000 -4 seq_printf
> 0x0000000000000031 X86_64_PC32 000000000000000000 -4 __fentry__
> 0x0000000000000038 X86_64_32S 000000000000000000 +0 .data
> 0x0000000000000051 X86_64_PC32 000000000000000000 -4 __fentry__
> 0x000000000000003d X86_64_PC32 000000000000000000 -4 klp_enable_patch
>
> We now have back-to-back rela's with sym->name = "saved_command_line".
> When the first is converted, convert_rela() will move both of them to
> the klp rela section. The linked list values may be consistent, but the
> cached ->next value will be bogus and the in-flight-traversal will run
> off the rails.
Yep, valid, if I'm reading the code correctly.
> I think we can work around it with a combination of 1) only moving a
> single rela symbol at a time in convert_rela and 2) processing the
> second (third, etc.) a little bit more so that they are moved
> individually.
>
> I hacked this together and it works against the livepatch-annotate.c
> test above so far...
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> index 82c27d219372..126395f1c0cd 100644
> --- a/scripts/livepatch/klp-convert.c
> +++ b/scripts/livepatch/klp-convert.c
> @@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
> if (r1->sym->name == r->sym->name) {
> list_del(&r1->list);
> list_add(&r1->list, &sec->relas);
> + break;
> }
> }
> return true;
Couldn't we remove the loop all together instead of breaking it?
list_del(&r->list);
list_add(&r->list, &sec->relas);
could be sufficient.
> @@ -549,8 +550,8 @@ static bool is_converted(char *sname)
> }
>
> /*
> - * Checks if symbol must be converted (conditions):
> - * not resolved, not already converted or isn't an exported symbol
> + * Checks if symbol must be or was already converted (conditions):
> + * not resolved or isn't an exported symbol
> */
> static bool must_convert(struct symbol *sym)
> {
> @@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym)
> if (strcmp(sym->name, ".TOC.") == 0)
> return false;
>
> - return (!(is_converted(sym->name) || is_exported(sym->name)));
> + return (!is_exported(sym->name));
> }
>
> /* Checks if a section is a klp rela section */
> @@ -640,7 +641,8 @@ int main(int argc, const char **argv)
> if (!must_convert(rela->sym))
> continue;
>
> - if (!find_missing_position(rela->sym, &sp)) {
> + if (!is_converted(rela->sym->name) &&
> + !find_missing_position(rela->sym, &sp)) {
> WARN("Unable to find missing symbol: %s",
> rela->sym->name);
> return -1;
Looks good.
Miroslav
[...]
> Result: a small tweak to sympos_sanity_check() to relax its symbol
> uniqueness verification: allow for duplicate <object, name, position>
> instances. Now it will only complain when a supplied symbol references
> the same <object, name> but a different <position>.
>
> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
> index 82c27d219372..713835dfc9ec 100644
> --- a/scripts/livepatch/klp-convert.c
> +++ b/scripts/livepatch/klp-convert.c
> @@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
> }
> }
>
> -/* Checks if two or more elements in usr_symbols have the same name */
> +/*
> + * Checks if two or more elements in usr_symbols have the same
> + * object and name, but different symbol position
> + */
> static bool sympos_sanity_check(void)
> {
> bool sane = true;
> @@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
> list_for_each_entry(sp, &usr_symbols, list) {
> aux = list_next_entry(sp, list);
> list_for_each_entry_from(aux, &usr_symbols, list) {
> - if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
> + if (sp->pos != aux->pos &&
> + strcmp(sp->object_name, aux->object_name) == 0 &&
> + strcmp(sp->symbol_name, aux->symbol_name) == 0)
> WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
> sp->object_name, sp->symbol_name, sp->pos,
> aux->object_name, aux->symbol_name, aux->pos);
Looks good and I'd definitely include it in v4.
> Unique sympos
> -------------
>
> But even with the above workaround, specifying unique symbol positions
> will (and should) result in a klp-convert complaint.
>
> When modifying the test module with unique symbol position annotation
> values (1 and 2 respectively):
>
> test_klp_convert_multi_objs_a.c:
>
> extern void *state_show;
> ...
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 1)
> };
>
> test_klp_convert_multi_objs_b.c:
>
> extern void *state_show;
> ...
> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
> KLP_SYMPOS(state_show, 2)
> };
>
>
> Each object file's symbol table contains a "state_show" instance:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep '\<state_show\>'
> 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
> 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>
> and the intermediate test_klp_convert_multi_objs.klp.o file contains a
> combined .klp.module_relocs.vmlinux relocation section with two
> KLP_SYMPOS structures, each with a unique <sympos> value:
>
> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
> lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
>
> 0000000 0000 0000 0000 0000 0001 0000 0000 0000
> 0000010 0000 0000 0002 0000
>
> but the symbol tables were merged, sorted and non-unique symbols
> removed, leaving a *single* unresolved "state_show" instance:
>
> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep '\<state_show\>'
> 53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>
> which means that even though we have separate relocations for each
> "state_show" instance:
>
> Relocation section [ 6] '.rela.text.unlikely' for section [ 5] '.text.unlikely' at offset 0x44510 contains 11 entries:
> Offset Type Value Addend Name
> 0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show
> ...
> 0x0000000000000034 X86_64_32S 000000000000000000 +0 state_show
> ...
>
> they share the same rela->sym and there is no way to distinguish which
> one correlates to which KLP_SYMPOS structure.
>
>
> Future Work
> -----------
>
> I don't see an easy way to support multiple homonym <object, name>
> symbols with unique <position> values in the current livepatch module
> Elf format. The only solutions that come to mind right now include
> renaming homonym symbols somehow to retain the relocation->symbol
> relationship when separate object files are combined. Perhaps an
> intermediate linker step could make annotated symbols unique in some way
> to achieve this. /thinking out loud
I'd set this aside for now and we can return to it later. I think it could
be quite rare in practice.
I was thinking about renaming the symbol too. We can extend the symbol
naming convention we have now and deal with it in klp_resolve_symbols(),
but maybe Josh will come up with something clever and cleaner.
Miroslav
On 4/24/19 3:19 PM, Miroslav Benes wrote:
> [...]
>
>> Result: a small tweak to sympos_sanity_check() to relax its symbol
>> uniqueness verification: allow for duplicate <object, name, position>
>> instances. Now it will only complain when a supplied symbol references
>> the same <object, name> but a different <position>.
>>
>> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
>> index 82c27d219372..713835dfc9ec 100644
>> --- a/scripts/livepatch/klp-convert.c
>> +++ b/scripts/livepatch/klp-convert.c
>> @@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
>> }
>> }
>>
>> -/* Checks if two or more elements in usr_symbols have the same name */
>> +/*
>> + * Checks if two or more elements in usr_symbols have the same
>> + * object and name, but different symbol position
>> + */
>> static bool sympos_sanity_check(void)
>> {
>> bool sane = true;
>> @@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
>> list_for_each_entry(sp, &usr_symbols, list) {
>> aux = list_next_entry(sp, list);
>> list_for_each_entry_from(aux, &usr_symbols, list) {
>> - if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
>> + if (sp->pos != aux->pos &&
>> + strcmp(sp->object_name, aux->object_name) == 0 &&
>> + strcmp(sp->symbol_name, aux->symbol_name) == 0)
>> WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
>> sp->object_name, sp->symbol_name, sp->pos,
>> aux->object_name, aux->symbol_name, aux->pos);
>
> Looks good and I'd definitely include it in v4.
>
>> Unique sympos
>> -------------
>>
>> But even with the above workaround, specifying unique symbol positions
>> will (and should) result in a klp-convert complaint.
>>
>> When modifying the test module with unique symbol position annotation
>> values (1 and 2 respectively):
>>
>> test_klp_convert_multi_objs_a.c:
>>
>> extern void *state_show;
>> ...
>> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
>> KLP_SYMPOS(state_show, 1)
>> };
>>
>> test_klp_convert_multi_objs_b.c:
>>
>> extern void *state_show;
>> ...
>> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
>> KLP_SYMPOS(state_show, 2)
>> };
>>
>>
>> Each object file's symbol table contains a "state_show" instance:
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep '\<state_show\>'
>> 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
>> 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> and the intermediate test_klp_convert_multi_objs.klp.o file contains a
>> combined .klp.module_relocs.vmlinux relocation section with two
>> KLP_SYMPOS structures, each with a unique <sympos> value:
>>
>> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
>> lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
>>
>> 0000000 0000 0000 0000 0000 0001 0000 0000 0000
>> 0000010 0000 0000 0002 0000
>>
>> but the symbol tables were merged, sorted and non-unique symbols
>> removed, leaving a *single* unresolved "state_show" instance:
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep '\<state_show\>'
>> 53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> which means that even though we have separate relocations for each
>> "state_show" instance:
>>
>> Relocation section [ 6] '.rela.text.unlikely' for section [ 5] '.text.unlikely' at offset 0x44510 contains 11 entries:
>> Offset Type Value Addend Name
>> 0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show
>> ...
>> 0x0000000000000034 X86_64_32S 000000000000000000 +0 state_show
>> ...
>>
>> they share the same rela->sym and there is no way to distinguish which
>> one correlates to which KLP_SYMPOS structure.
>>
>>
>> Future Work
>> -----------
>>
>> I don't see an easy way to support multiple homonym <object, name>
>> symbols with unique <position> values in the current livepatch module
>> Elf format. The only solutions that come to mind right now include
>> renaming homonym symbols somehow to retain the relocation->symbol
>> relationship when separate object files are combined. Perhaps an
>> intermediate linker step could make annotated symbols unique in some way
>> to achieve this. /thinking out loud
>
> I'd set this aside for now and we can return to it later. I think it could
> be quite rare in practice.
>
> I was thinking about renaming the symbol too. We can extend the symbol
> naming convention we have now and deal with it in klp_resolve_symbols(),
> but maybe Josh will come up with something clever and cleaner.
I think this could work well, but (sorry if I understood Joe's idea
wrongly) not as a linker step. Instead of modifying the linker, I think
we could create another tool and plug it into the kbuild pipeline prior
to the livepatch module linking. This way, we would parse the .o elf
files, check for homonyms and rename them based on a convention that is
later understood by klp-convert, as suggested.
If I am not missing something, this would fix the case where we have
homonyms pointing to the same or different positions, without additional
user intervention other then adding the SYMPOS annotations.
If you consider this to be useful I can start experiencing.
>
> Miroslav
>
On Wed, Apr 24, 2019 at 04:13:59PM -0300, Joao Moreira wrote:
> > > Future Work
> > > -----------
> > >
> > > I don't see an easy way to support multiple homonym <object, name>
> > > symbols with unique <position> values in the current livepatch module
> > > Elf format. The only solutions that come to mind right now include
> > > renaming homonym symbols somehow to retain the relocation->symbol
> > > relationship when separate object files are combined. Perhaps an
> > > intermediate linker step could make annotated symbols unique in some way
> > > to achieve this. /thinking out loud
> >
> > I'd set this aside for now and we can return to it later. I think it could
> > be quite rare in practice.
> >
> > I was thinking about renaming the symbol too. We can extend the symbol
> > naming convention we have now and deal with it in klp_resolve_symbols(),
> > but maybe Josh will come up with something clever and cleaner.
>
> I think this could work well, but (sorry if I understood Joe's idea wrongly)
> not as a linker step. Instead of modifying the linker, I think we could
> create another tool and plug it into the kbuild pipeline prior to the
> livepatch module linking. This way, we would parse the .o elf files, check
> for homonyms and rename them based on a convention that is later understood
> by klp-convert, as suggested.
>
> If I am not missing something, this would fix the case where we have
> homonyms pointing to the same or different positions, without additional
> user intervention other then adding the SYMPOS annotations.
>
> If you consider this to be useful I can start experiencing.
I tend to agree with Miroslav that it's probably ok to ignore this issue
for now, as long as klp-convert can detect it and spit out an error. I
assume the patch author could work around it with a kallsyms hack. If
we encounter it much in the real world then we could figure out a
solution at that point.
--
Josh
On 4/24/19 3:13 PM, Joao Moreira wrote:
>>> Future Work
>>> -----------
>>>
>>> I don't see an easy way to support multiple homonym <object, name>
>>> symbols with unique <position> values in the current livepatch module
>>> Elf format. The only solutions that come to mind right now include
>>> renaming homonym symbols somehow to retain the relocation->symbol
>>> relationship when separate object files are combined. Perhaps an
>>> intermediate linker step could make annotated symbols unique in some way
>>> to achieve this. /thinking out loud
>> I'd set this aside for now and we can return to it later. I think it could
>> be quite rare in practice.
I agree, especially since we can detect this corner case and abort the
translation.
>> I was thinking about renaming the symbol too. We can extend the symbol
>> naming convention we have now and deal with it in klp_resolve_symbols(),
>> but maybe Josh will come up with something clever and cleaner.
> I think this could work well, but (sorry if I understood Joe's idea
> wrongly) not as a linker step. Instead of modifying the linker, I think
> we could create another tool and plug it into the kbuild pipeline prior
> to the livepatch module linking. This way, we would parse the .o elf
> files, check for homonyms and rename them based on a convention that is
> later understood by klp-convert, as suggested.
My knowledge of the build tools is limited, so there was a bunch of
hand-waving you couldn't see when I wrote that paragraph :) But yes,
that is basically the idea: plugging into the kbuild pipeline to give
these some kinda of .o-unique prefix that klp-convert would interpret
and strip accordingly.
> If I am not missing something, this would fix the case where we have
> homonyms pointing to the same or different positions, without additional
> user intervention other then adding the SYMPOS annotations.
>
> If you consider this to be useful I can start experiencing.
>
It's not the highest priority, but even a prototype of how to insert a
script into the pipeline to achieve this would be massively time saving
for myself. If renaming looks easy, we could try to work into the
initial klp-convert patchset... if not, save it for a follow up enhancement.
Thanks,
-- Joe
On 4/24/19 1:47 PM, Miroslav Benes wrote:
> On Tue, 23 Apr 2019, Joe Lawrence wrote:
>> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
>> index 82c27d219372..126395f1c0cd 100644
>> --- a/scripts/livepatch/klp-convert.c
>> +++ b/scripts/livepatch/klp-convert.c
>> @@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
>> if (r1->sym->name == r->sym->name) {
>> list_del(&r1->list);
>> list_add(&r1->list, &sec->relas);
>> + break;
>> }
>> }
>> return true;
>
> Couldn't we remove the loop all together instead of breaking it?
>
> list_del(&r->list);
> list_add(&r->list, &sec->relas);
>
> could be sufficient.
Ah yea, good point.
>
>> @@ -549,8 +550,8 @@ static bool is_converted(char *sname)
>> }
>>
>> /*
>> - * Checks if symbol must be converted (conditions):
>> - * not resolved, not already converted or isn't an exported symbol
>> + * Checks if symbol must be or was already converted (conditions):
>> + * not resolved or isn't an exported symbol
>> */
>> static bool must_convert(struct symbol *sym)
>> {
>> @@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym)
>> if (strcmp(sym->name, ".TOC.") == 0)
>> return false;
>>
>> - return (!(is_converted(sym->name) || is_exported(sym->name)));
>> + return (!is_exported(sym->name));
>> }
>>
>> /* Checks if a section is a klp rela section */
>> @@ -640,7 +641,8 @@ int main(int argc, const char **argv)
>> if (!must_convert(rela->sym))
>> continue;
>>
>> - if (!find_missing_position(rela->sym, &sp)) {
>> + if (!is_converted(rela->sym->name) &&
>> + !find_missing_position(rela->sym, &sp)) {
>> WARN("Unable to find missing symbol: %s",
>> rela->sym->name);
>> return -1;
>
> Looks good.
There were a few additional spots I needed to update (like main's
section walk doesn't need to re-convert klp_rela_sections, and likewise,
convert_rela doesn't need to re-convert_klp_symbol), but I will include
with v4 for a real review.
Thanks,
-- Joe
On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote:
>
> [ ... snip ... ]
>
> Quick look, but it seems quite similar to the problem we had with
> apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
> introduced it.
That was an interesting diversion :) I think I grok the idea as:
The kernel supports a few different code-patching methods:
- SMP locks
- alternatives
- paravirt
- jump labels
and we need to ensure that they do not prematurely operate on unresolved
klp-relocations. The solution that led to arch/x86/kernel/livepatch.c
introduces "klp.arch" sections that rename such klp-relocations *and*
their associated special section data structures. Processing is then
deferred until after a relevant klp_object is loaded.
> I think, we should do the same for jump labels. Add
> jump_label_apply_nops() from module_finalize() to
> arch_klp_init_object_loaded() and convert jump_table ELF section so its
> processing is delayed.
Nod. Tthat sounds about right. There may be some more work yet in the
static keys API as well, but I'm not 100%.
> Which leads me another TODO... klp-convert does not convert even
> .altinstructions and .parainstructions sections, so it has that problem as
> well. If I remember, it was on Josh's TODO list when he first introduced
> klp-convert. See [email protected].
In the RFC, Josh highlights a somewhat difficult problem regarding these
special sections -- how to associate these special section data
structures and their relocations to a specific klp_object.
If I understand his suggestion, he proposed annotating livepatch module
replacement functions as to stuff them into specially named ELF sections
(which would include the klp_object name) and then bypass the existing
livepatch registration API. No minor change.
With that in mind, I'm starting to think of a game plan for klp-convert
like:
- phase 1: detect /abort unsupported sections
- phase 2: manual annotations in livepatch modules (like
KLP_MODULE_RELOC / SYMPOS, but for special sections) so
that klp-convert can start building "klp.arch" sections
- phase 3: livepatch API change above to support somewhat more
automatic generation of phase 2 annotations
> The selftest for the alternatives would be appreciated too. One day.
In the course of understanding the background behind
arch/x86/kernel/livepatch.c, I wrote a bunch of livepatch selftests that
try out simple examples of those special sections.
For alternatives, I did something like:
/* TODO: find reliably true/false features */
#define TRUE_FEATURE (X86_FEATURE_FPU)
#define FALSE_FEATURE (X86_FEATURE_VME)
...
klp_function1()
klp_function2()
klp_new_function()
asm (ALTERNATIVE("call klp_function1", "call klp_function2", TRUE_FEATURE));
asm (ALTERNATIVE("call klp_function1", "call klp_function2", FALSE_FEATURE));
asm (ALTERNATIVE("call mod_function1", "call mod_function2", TRUE_FEATURE));
asm (ALTERNATIVE("call mod_function1", "call mod_function2", FALSE_FEATURE));
asm (ALTERNATIVE("call mod_function2", "call mod_function1", TRUE_FEATURE));
asm (ALTERNATIVE("call mod_function2", "call mod_function1", FALSE_FEATURE));
so that I could see what kind of relocations were generated for default
and non-default instructions as well as module-local and then
unexported-extern functions.
Once we have klp-convert supporting these conversions, I think something
like that would suffice. In the meantime, I'm not sure how to create
"klp.arch" sectioned ELFs without something like kpatch-build.
> And of course we should look at the other supported architectures and
> their module_finalize() functions. I have it on my TODO list somewhere,
> but you know how it works with those :/. I am sure there are more hidden
> surprises there.
Hmm, well powerpc and s390 do appear to have processing for special
sections as well ... but for the moment, I'm going to focus on x86 as
that seems like enough work for now :)
> > Detection
> > ---------
> >
> > I can post ("livepatch/klp-convert: abort on static key conversion")
> > here as a follow commit if it looks reasonable and folks wish to review
> > it... or we can try and tackle static keys before merging klp-convert.
>
> Good idea. I'd rather fix it, but I think it could be a lot of work, so
> something like this patch seems to be a good idea.
I'm thinking of adding this in a commit so klp-convert can intercept
these sections:
static bool is_section_supported(char *sname)
{
if (strcmp(sname, ".rela.altinstructions") == 0)
return false;
if (strcmp(sname, ".rela.parainstructions") == 0)
return false;
if (strcmp(sname, ".rela__jump_table") == 0)
return false;
return true;
}
Right now my v4 collection has a bunch of small fixups and nitpick
corrections. It feels like a good resting place for now before
embarking on special section support, what do you think?
-- Joe
On Fri, 3 May 2019, Joe Lawrence wrote:
> On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote:
> >
> > [ ... snip ... ]
> >
> > Quick look, but it seems quite similar to the problem we had with
> > apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
> > introduced it.
>
> That was an interesting diversion :) I think I grok the idea as:
>
> The kernel supports a few different code-patching methods:
>
> - SMP locks
> - alternatives
> - paravirt
> - jump labels
>
> and we need to ensure that they do not prematurely operate on unresolved
> klp-relocations. The solution that led to arch/x86/kernel/livepatch.c
> introduces "klp.arch" sections that rename such klp-relocations *and*
> their associated special section data structures. Processing is then
> deferred until after a relevant klp_object is loaded.
Correct.
> > I think, we should do the same for jump labels. Add
> > jump_label_apply_nops() from module_finalize() to
> > arch_klp_init_object_loaded() and convert jump_table ELF section so its
> > processing is delayed.
>
> Nod. Tthat sounds about right. There may be some more work yet in the
> static keys API as well, but I'm not 100%.
>
> > Which leads me another TODO... klp-convert does not convert even
> > .altinstructions and .parainstructions sections, so it has that problem as
> > well. If I remember, it was on Josh's TODO list when he first introduced
> > klp-convert. See [email protected].
>
> In the RFC, Josh highlights a somewhat difficult problem regarding these
> special sections -- how to associate these special section data
> structures and their relocations to a specific klp_object.
>
> If I understand his suggestion, he proposed annotating livepatch module
> replacement functions as to stuff them into specially named ELF sections
> (which would include the klp_object name) and then bypass the existing
> livepatch registration API. No minor change.
>
> With that in mind, I'm starting to think of a game plan for klp-convert
> like:
>
> - phase 1: detect /abort unsupported sections
>
> - phase 2: manual annotations in livepatch modules (like
> KLP_MODULE_RELOC / SYMPOS, but for special sections) so
> that klp-convert can start building "klp.arch" sections
>
> - phase 3: livepatch API change above to support somewhat more
> automatic generation of phase 2 annotations
Looks good to me. First, I'd focus on something which covers (hopefully) a
vast majority cases and then we can do the rest. So yes, this seems to be
a good plan.
> > And of course we should look at the other supported architectures and
> > their module_finalize() functions. I have it on my TODO list somewhere,
> > but you know how it works with those :/. I am sure there are more hidden
> > surprises there.
>
> Hmm, well powerpc and s390 do appear to have processing for special
> sections as well ... but for the moment, I'm going to focus on x86 as
> that seems like enough work for now :)
Yes, please :).
> > > Detection
> > > ---------
> > >
> > > I can post ("livepatch/klp-convert: abort on static key conversion")
> > > here as a follow commit if it looks reasonable and folks wish to review
> > > it... or we can try and tackle static keys before merging klp-convert.
> >
> > Good idea. I'd rather fix it, but I think it could be a lot of work, so
> > something like this patch seems to be a good idea.
>
> I'm thinking of adding this in a commit so klp-convert can intercept
> these sections:
>
> static bool is_section_supported(char *sname)
> {
> if (strcmp(sname, ".rela.altinstructions") == 0)
> return false;
> if (strcmp(sname, ".rela.parainstructions") == 0)
> return false;
> if (strcmp(sname, ".rela__jump_table") == 0)
> return false;
> return true;
> }
>
> Right now my v4 collection has a bunch of small fixups and nitpick
> corrections. It feels like a good resting place for now before
> embarking on special section support, what do you think?
Yes.
Thanks
Miroslav