[RFC PATCH v2 0/6] (mostly) Arch-independent livepatch

[RFC PATCH v2 0/6] (mostly) Arch-independent livepatch

[Jessica Yu]

This patchset removes livepatch's need for architecture-specific relocation
code by leveraging existing code in the module loader to perform
arch-dependent work. Specifically, instead of duplicating code and
re-implementing what the apply_relocate_add() function in the module loader
already does in livepatch's klp_write_module_reloc(), we reuse
apply_relocate_add() to write relocations. The hope is that this will make
livepatch more easily portable to other architectures and greatly reduce
the amount of arch-specific code required to port livepatch to a particular
architecture.

Background: Why does livepatch need to write its own relocations?
==
A typical livepatch module contains patched versions of functions that can
reference non-exported global symbols and non-included local symbols.
Relocations referencing these types of symbols cannot be left in as-is
since the kernel module loader cannot resolve them and will therefore
reject the livepatch module. Furthermore, we cannot apply relocations that
affect modules not loaded yet at run time (e.g. a patch to a driver). The
current kpatch build system therefore solves this problem by embedding
special "dynrela" (dynamic reloc) sections in the resulting patch module
Elf output. Using these dynrela sections, livepatch can correctly resolve
symbols while taking into account its scope and what module the symbol
belongs to, and then manually apply the dynamic relocations.

Motivation: Why is having arch-dependent relocation code a problem?
==
The original motivation for this patchset stems from the increasing
roadblocks encountered while attempting to port livepatch to s390.
Specifically, there were problems dealing with s390 PLT and GOT relocation
types (R_390_{PLT,GOT}), which are handled differently from x86's
relocation types (which are much simpler to deal with, and a single
livepatch function (klp_write_module_reloc()) has been sufficient enough).
These s390 reloc types cannot be handled by simply performing a calculation
(as in the x86 case). For s390 modules with PLT/GOT relocations, the kernel
module loader allocates and fills in PLT+GOT table entries for every symbol
referenced by a PLT/GOT reloc in module core memory. So the problem of
porting livepatch to s390 became much more complicated than simply writing
an s390-specific klp_write_module_reloc() function. How can livepatch
handle these relocation types if the s390 module loader needs to allocate
and fill PLT/GOT entries ahead of time? The potential solutions were: 1)
have livepatch possibly allocate and maintain its own PLT/GOT tables for
every patch module (requiring even more arch-specific code), 2) modify the
s390 module loader heavily to accommodate livepatch modules (i.e. allocate
all the needed PLT/GOT entries for livepatch in advance but refrain from
applying relocations for to-be-patched modules), or 3) eliminate this
potential mess by leveraging module loader code to do all the relocation
work, letting livepatch off the hook completely. Solution #3 is what this
patchset implements.

How does this patchset remedy these problems?
==
Reusing the module loader code to perform livepatch relocations means that
livepatch no longer needs arch-specific reloc code and the aforementioned
problems with s390 PLT/GOT reloc types disappear (because we let the module
loader do all the relocation work for us). It will enable livepatch to be
more easily ported to other architectures.

Summary of proposed changes
==
This patch series enables livepatch to use the module loader's
apply_relocate_add() function to apply livepatch relocations (i.e. what
used to be dynrelas). apply_relocate_add() requires access to a patch
module's section headers, symbol table, reloc section indices, etc., and all
of these are accessible through the load_info struct used in the module
loader. Therefore we persist module Elf information (copied from load_info)
for livepatch modules.

The ELF-related changes enable livepatch to patch modules that are not yet
loaded (as well as patch vmlinux when kaslr is enabled). In order to use
apply_relocate_add(), we need real SHT_RELA sections to pass in. A
complication here is that relocations for not-yet-loaded modules should not
be applied when the patch module loads; they should only be applied once
the target module is loaded. Thus kpatch build scripts were modified to
output a livepatch module that contains special __klp_rela sections that
are managed by livepatch and are applied at the appropriate time (i.e. when
target module loads). They are marked with a special SHF_RELA_LIVEPATCH
section flag to indicate to the module loader that livepatch will handle
them. The SHN_LIVEPATCH shndx marks symbols that need to be resolved
once their respective target module loads. So, the module loader ignores
these symbols and does not attempt to resolve them. Finally, the
STB_LIVEPATCH_EXT symbol bind marks the scope of certain livepatch symbols,
so that livepatch can find the symbol in the right place. These ELF
constants were selected from OS-specific ranges according to the
definitions from glibc.

v2:
 - Copy only the minimum required Elf information for livepatch modules to
   make the call to apply_relocate_add(), not the entire load_info struct
   and the redundant copy of the module in memory
 - Add module->klp flag for simple identification of livepatch modules
 - s390: remove redundant vfree() and preserve mod_arch_specific if
   livepatch module
 - Use array format instead of a linked list for klp_reloc_secs
 - Add new documentation describing the format of a livepatch module in
   Documentation/livepatch

Jessica Yu (6):
  Elf: add livepatch-specific Elf constants
  module: preserve Elf information for livepatch modules
  module: s390: keep mod_arch_specific for livepatch modules
  livepatch: reuse module loader code to write relocations
  samples: livepatch: init reloc section array and mark as klp module
  Documentation: livepatch: outline the Elf format of a livepatch module

 Documentation/livepatch/patch-module-format.txt | 117 ++++++++++++++++++++++++
 arch/s390/kernel/module.c                       |  13 ++-
 arch/x86/include/asm/livepatch.h                |   2 -
 arch/x86/kernel/Makefile                        |   1 -
 arch/x86/kernel/livepatch.c                     |  91 ------------------
 include/linux/livepatch.h                       |  30 +++---
 include/linux/module.h                          |  15 +++
 include/uapi/linux/elf.h                        |  17 ++--
 kernel/livepatch/core.c                         |  94 ++++++++++---------
 kernel/module.c                                 |  98 +++++++++++++++++++-
 samples/livepatch/livepatch-sample.c            |   6 ++
 11 files changed, 319 insertions(+), 165 deletions(-)
 create mode 100644 Documentation/livepatch/patch-module-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

[RFC PATCH v2 0/6] (mostly) Arch-independent livepatch

[Jessica Yu]

This patchset removes livepatch's need for architecture-specific relocation
code by leveraging existing code in the module loader to perform
arch-dependent work. Specifically, instead of duplicating code and
re-implementing what the apply_relocate_add() function in the module loader
already does in livepatch's klp_write_module_reloc(), we reuse
apply_relocate_add() to write relocations. The hope is that this will make
livepatch more easily portable to other architectures and greatly reduce
the amount of arch-specific code required to port livepatch to a particular
architecture.

Background: Why does livepatch need to write its own relocations?
==
A typical livepatch module contains patched versions of functions that can
reference non-exported global symbols and non-included local symbols.
Relocations referencing these types of symbols cannot be left in as-is
since the kernel module loader cannot resolve them and will therefore
reject the livepatch module. Furthermore, we cannot apply relocations that
affect modules not loaded yet at run time (e.g. a patch to a driver). The
current kpatch build system therefore solves this problem by embedding
special "dynrela" (dynamic reloc) sections in the resulting patch module
Elf output. Using these dynrela sections, livepatch can correctly resolve
symbols while taking into account its scope and what module the symbol
belongs to, and then manually apply the dynamic relocations.

Motivation: Why is having arch-dependent relocation code a problem?
==
The original motivation for this patchset stems from the increasing
roadblocks encountered while attempting to port livepatch to s390.
Specifically, there were problems dealing with s390 PLT and GOT relocation
types (R_390_{PLT,GOT}), which are handled differently from x86's
relocation types (which are much simpler to deal with, and a single
livepatch function (klp_write_module_reloc()) has been sufficient enough).
These s390 reloc types cannot be handled by simply performing a calculation
(as in the x86 case). For s390 modules with PLT/GOT relocations, the kernel
module loader allocates and fills in PLT+GOT table entries for every symbol
referenced by a PLT/GOT reloc in module core memory. So the problem of
porting livepatch to s390 became much more complicated than simply writing
an s390-specific klp_write_module_reloc() function. How can livepatch
handle these relocation types if the s390 module loader needs to allocate
and fill PLT/GOT entries ahead of time? The potential solutions were: 1)
have livepatch possibly allocate and maintain its own PLT/GOT tables for
every patch module (requiring even more arch-specific code), 2) modify the
s390 module loader heavily to accommodate livepatch modules (i.e. allocate
all the needed PLT/GOT entries for livepatch in advance but refrain from
applying relocations for to-be-patched modules), or 3) eliminate this
potential mess by leveraging module loader code to do all the relocation
work, letting livepatch off the hook completely. Solution #3 is what this
patchset implements.

How does this patchset remedy these problems?
==
Reusing the module loader code to perform livepatch relocations means that
livepatch no longer needs arch-specific reloc code and the aforementioned
problems with s390 PLT/GOT reloc types disappear (because we let the module
loader do all the relocation work for us). It will enable livepatch to be
more easily ported to other architectures.

Summary of proposed changes
==
This patch series enables livepatch to use the module loader's
apply_relocate_add() function to apply livepatch relocations (i.e. what
used to be dynrelas). apply_relocate_add() requires access to a patch
module's section headers, symbol table, reloc section indices, etc., and all
of these are accessible through the load_info struct used in the module
loader. Therefore we persist module Elf information (copied from load_info)
for livepatch modules.

The ELF-related changes enable livepatch to patch modules that are not yet
loaded (as well as patch vmlinux when kaslr is enabled). In order to use
apply_relocate_add(), we need real SHT_RELA sections to pass in. A
complication here is that relocations for not-yet-loaded modules should not
be applied when the patch module loads; they should only be applied once
the target module is loaded. Thus kpatch build scripts were modified to
output a livepatch module that contains special __klp_rela sections that
are managed by livepatch and are applied at the appropriate time (i.e. when
target module loads). They are marked with a special SHF_RELA_LIVEPATCH
section flag to indicate to the module loader that livepatch will handle
them. The SHN_LIVEPATCH shndx marks symbols that need to be resolved
once their respective target module loads. So, the module loader ignores
these symbols and does not attempt to resolve them. Finally, the
STB_LIVEPATCH_EXT symbol bind marks the scope of certain livepatch symbols,
so that livepatch can find the symbol in the right place. These ELF
constants were selected from OS-specific ranges according to the
definitions from glibc.

v2:
 - Copy only the minimum required Elf information for livepatch modules to
   make the call to apply_relocate_add(), not the entire load_info struct
   and the redundant copy of the module in memory
 - Add module->klp flag for simple identification of livepatch modules
 - s390: remove redundant vfree() and preserve mod_arch_specific if
   livepatch module
 - Use array format instead of a linked list for klp_reloc_secs
 - Add new documentation describing the format of a livepatch module in
   Documentation/livepatch

Jessica Yu (6):
  Elf: add livepatch-specific Elf constants
  module: preserve Elf information for livepatch modules
  module: s390: keep mod_arch_specific for livepatch modules
  livepatch: reuse module loader code to write relocations
  samples: livepatch: init reloc section array and mark as klp module
  Documentation: livepatch: outline the Elf format of a livepatch module

 Documentation/livepatch/patch-module-format.txt | 117 ++++++++++++++++++++++++
 arch/s390/kernel/module.c                       |  13 ++-
 arch/x86/include/asm/livepatch.h                |   2 -
 arch/x86/kernel/Makefile                        |   1 -
 arch/x86/kernel/livepatch.c                     |  91 ------------------
 include/linux/livepatch.h                       |  30 +++---
 include/linux/module.h                          |  15 +++
 include/uapi/linux/elf.h                        |  17 ++--
 kernel/livepatch/core.c                         |  94 ++++++++++---------
 kernel/module.c                                 |  98 +++++++++++++++++++-
 samples/livepatch/livepatch-sample.c            |   6 ++
 11 files changed, 319 insertions(+), 165 deletions(-)
 create mode 100644 Documentation/livepatch/patch-module-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

[RFC PATCH v2 1/6] Elf: add livepatch-specific Elf constants

[Jessica Yu]

Add livepatch Elf relocation section flag (SHF_RELA_LIVEPATCH), livepatch
symbol bind (STB_LIVEPATCH_EXT) and section index (SHN_LIVEPATCH).
The values of these Elf constants were selected from OS-specific ranges
according to the definitions from glibc.

Livepatch relocation sections are marked with SHF_RELA_LIVEPATCH to
indicate to the module loader that it should not apply that relocation
section and that livepatch will handle them.

The SHN_LIVEPATCH shndx marks symbols that will be resolved by livepatch.
The module loader ignores these symbols and does not attempt to resolve
them.

The STB_LIVEPATCH_EXT symbol bind marks the scope of certain livepatch
symbols, so that livepatch can appropriately resolve them.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/uapi/linux/elf.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 71e1d0e..95ff318 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -115,9 +115,10 @@ typedef __s64	Elf64_Sxword;
 #define DT_HIPROC	0x7fffffff
 
 /* This info is needed when parsing the symbol table */
-#define STB_LOCAL  0
-#define STB_GLOBAL 1
-#define STB_WEAK   2
+#define STB_LOCAL		0
+#define STB_GLOBAL		1
+#define STB_WEAK		2
+#define STB_LIVEPATCH_EXT	11
 
 #define STT_NOTYPE  0
 #define STT_OBJECT  1
@@ -282,16 +283,18 @@ typedef struct elf64_phdr {
 #define SHT_HIUSER	0xffffffff
 
 /* sh_flags */
-#define SHF_WRITE	0x1
-#define SHF_ALLOC	0x2
-#define SHF_EXECINSTR	0x4
-#define SHF_MASKPROC	0xf0000000
+#define SHF_WRITE		0x1
+#define SHF_ALLOC		0x2
+#define SHF_EXECINSTR		0x4
+#define SHF_RELA_LIVEPATCH	0x00100000
+#define SHF_MASKPROC		0xf0000000
 
 /* special section indexes */
 #define SHN_UNDEF	0
 #define SHN_LORESERVE	0xff00
 #define SHN_LOPROC	0xff00
 #define SHN_HIPROC	0xff1f
+#define SHN_LIVEPATCH	0xff20
 #define SHN_ABS		0xfff1
 #define SHN_COMMON	0xfff2
 #define SHN_HIRESERVE	0xffff
-- 
2.4.3

[RFC PATCH v2 2/6] module: preserve Elf information for livepatch modules

[Jessica Yu]

For livepatch modules, copy Elf section, symbol, and string information
from the load_info struct in the module loader.

Livepatch uses special relocation sections in order to be able to patch
modules that are not yet loaded, as well as apply patches to the kernel
when the addresses of symbols cannot be determined at compile time (for
example, when kaslr is enabled). Livepatch modules must preserve Elf
information such as section indices in order to apply the remaining
relocation sections at the appropriate time (i.e. when the target module
loads).

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/module.h |  9 +++++
 kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..9b46256 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -425,6 +425,14 @@ struct module {
 
 	/* Notes attributes */
 	struct module_notes_attrs *notes_attrs;
+
+	/* Elf information (optionally saved) */
+	Elf_Ehdr *hdr;
+	Elf_Shdr *sechdrs;
+	char *secstrings;
+	struct {
+		unsigned int sym, str, mod, vers, info, pcpu;
+	} index;
 #endif
 
 	/* The command line arguments (may be mangled).  People like
@@ -461,6 +469,7 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+	bool klp; /* Is this a livepatch module? */
 	bool klp_alive;
 #endif
 
diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..433c2d6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
 
+static void free_module_elf(struct module *mod)
+{
+	kfree(mod->hdr);
+	kfree(mod->sechdrs);
+	kfree(mod->secstrings);
+}
+
 void __weak module_memfree(void *module_region)
 {
 	vfree(module_region);
@@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
 	/* Free any allocated parameters. */
 	destroy_params(mod->kp, mod->num_kp);
 
+	/* Free Elf information if it was saved */
+	free_module_elf(mod);
+
 	/* Now we can delete it from the lists */
 	mutex_lock(&module_mutex);
 	/* Unlink carefully: kallsyms could be walking list. */
@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			       (long)sym[i].st_value);
 			break;
 
+		case SHN_LIVEPATCH:
+			/* klp symbols are resolved by livepatch */
+			break;
+
 		case SHN_UNDEF:
 			ksym = resolve_symbol_wait(mod, info, name);
 			/* Ok if resolved.  */
@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
+		/* klp relocation sections are applied by livepatch */
+		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+			continue;
+
 		if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
 {
 	const Elf_Shdr *sechdrs = info->sechdrs;
 
+	if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
+		return 'K';
+	if (sym->st_shndx == SHN_LIVEPATCH)
+		return 'k';
+
 	if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
 		if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
 			return 'v';
@@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 
 	/* Compute total space required for the core symbols' strtab. */
 	for (ndst = i = 0; i < nsrc; i++) {
-		if (i == 0 ||
+		if (i == 0 || mod->klp ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
 			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
 			ndst++;
@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->core_strtab = s = mod->module_core + info->stroffs;
 	src = mod->symtab;
 	for (ndst = i = 0; i < mod->num_symtab; i++) {
-		if (i == 0 ||
+		if (i == 0 || mod->klp ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
 			dst[ndst] = src[i];
 			dst[ndst++].st_name = s - mod->core_strtab;
@@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
 	return 0;
 }
 
+/*
+ * copy_module_elf - preserve Elf information about a module
+ */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	unsigned int size;
+	int ret = 0;
+	Elf_Shdr *symsect;
+
+	/* Elf header */
+	size = sizeof(Elf_Ehdr);
+	mod->hdr = kzalloc(size, GFP_KERNEL);
+	if (mod->hdr == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	memcpy(mod->hdr, info->hdr, size);
+
+	/* Elf section header table */
+	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
+	mod->sechdrs = kzalloc(size, GFP_KERNEL);
+	if (mod->sechdrs == NULL) {
+		ret = -ENOMEM;
+		goto free_hdr;
+	}
+	memcpy(mod->sechdrs, info->sechdrs, size);
+
+	/* Elf section name string table */
+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+	mod->secstrings = kzalloc(size, GFP_KERNEL);
+	if (mod->secstrings == NULL) {
+		ret = -ENOMEM;
+		goto free_sechdrs;
+	}
+	memcpy(mod->secstrings, info->secstrings, size);
+
+	/* Elf section indices */
+	memcpy(&mod->index, &info->index, sizeof(info->index));
+
+	/*
+	 * Update symtab's sh_addr to point to a valid
+	 * symbol table, as the temporary symtab in module
+	 * init memory will be freed
+	 */
+	symsect = mod->sechdrs + mod->index.sym;
+	symsect->sh_addr = (unsigned long)mod->core_symtab;
+
+	return ret;
+
+free_sechdrs:
+	kfree(mod->sechdrs);
+free_hdr:
+	kfree(mod->hdr);
+out:
+	return ret;
+}
+
+
 #define COPY_CHUNK_SIZE (16*PAGE_SIZE)
 
 static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned long len)
@@ -2866,6 +2947,9 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+	if (get_modinfo(info, "livepatch"))
+		mod->klp = true;
+
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
 
@@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto bug_cleanup;
 
+	/*
+	 * Save sechdrs, indices, and other data from info
+	 * in order to patch to-be-loaded modules.
+	 * Do not call free_copy() for livepatch modules.
+	 */
+	if (mod->klp)
+		err = copy_module_elf(mod, info);
+	if (err < 0)
+		goto bug_cleanup;
+
 	/* Get rid of temporary copy. */
 	free_copy(info);
 
-- 
2.4.3

[RFC PATCH v2 3/6] module: s390: keep mod_arch_specific for livepatch modules

[Jessica Yu]

Livepatch needs to utilize the symbol information contained in the
mod_arch_specific struct in order to be able to call the s390
apply_relocate_add() function to apply relocations. Remove the redundant
vfree() in module_finalize() since module_arch_freeing_init() (which also frees
said structures) is called in do_init_module(). Keep a reference to syminfo if
the module is a livepatch module and free the structures in
module_arch_cleanup(). If the module isn't a livepatch module, we free the
structures in module_arch_freeing_init() as usual.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 arch/s390/kernel/module.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index 0c1a679..17a1979 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -51,6 +51,9 @@ void *module_alloc(unsigned long size)
 
 void module_arch_freeing_init(struct module *mod)
 {
+	if (mod->klp)
+		return;
+
 	vfree(mod->arch.syminfo);
 	mod->arch.syminfo = NULL;
 }
@@ -420,12 +423,18 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	return 0;
 }
 
+void module_arch_cleanup(struct module *mod)
+{
+	if (mod->klp) {
+		vfree(mod->arch.syminfo);
+		mod->arch.syminfo = NULL;
+	}
+}
+
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
 	jump_label_apply_nops(me);
-	vfree(me->arch.syminfo);
-	me->arch.syminfo = NULL;
 	return 0;
 }
-- 
2.4.3

[RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Jessica Yu]

Reuse module loader code to write relocations, thereby eliminating the need
for architecture specific relocation code in livepatch. Namely, we reuse
apply_relocate_add() in the module loader to write relocations instead of
duplicating functionality in livepatch's klp_write_module_reloc(). To apply
relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
are resolved and then apply_relocate_add() is called to apply those
relocations.

In addition, remove x86 livepatch relocation code. It is no longer needed
since symbol resolution and relocation work have been offloaded to module
loader.   

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 arch/x86/include/asm/livepatch.h |  2 -
 arch/x86/kernel/Makefile         |  1 -
 arch/x86/kernel/livepatch.c      | 91 --------------------------------------
 include/linux/livepatch.h        | 30 ++++++-------
 include/linux/module.h           |  6 +++
 kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
 6 files changed, 70 insertions(+), 154 deletions(-)
 delete mode 100644 arch/x86/kernel/livepatch.c

diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index 19c099a..7312e25 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
 #endif
 	return 0;
 }
-int klp_write_module_reloc(struct module *mod, unsigned long type,
-			   unsigned long loc, unsigned long value);
 
 static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 {
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index b1b78ff..c5e9a5c 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
 obj-$(CONFIG_X86_TSC)		+= trace_clock.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
deleted file mode 100644
index d1d35cc..0000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * livepatch.c - x86-specific Kernel Live Patching Core
- *
- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
- * Copyright (C) 2014 SUSE
- *
- * 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/>.
- */
-
-#include <linux/module.h>
-#include <linux/uaccess.h>
-#include <asm/cacheflush.h>
-#include <asm/page_types.h>
-#include <asm/elf.h>
-#include <asm/livepatch.h>
-
-/**
- * klp_write_module_reloc() - write a relocation in a module
- * @mod:	module in which the section to be modified is found
- * @type:	ELF relocation type (see asm/elf.h)
- * @loc:	address that the relocation should be written to
- * @value:	relocation value (sym address + addend)
- *
- * This function writes a relocation to the specified location for
- * a particular module.
- */
-int klp_write_module_reloc(struct module *mod, unsigned long type,
-			   unsigned long loc, unsigned long value)
-{
-	int ret, numpages, size = 4;
-	bool readonly;
-	unsigned long val;
-	unsigned long core = (unsigned long)mod->module_core;
-	unsigned long core_size = mod->core_size;
-
-	switch (type) {
-	case R_X86_64_NONE:
-		return 0;
-	case R_X86_64_64:
-		val = value;
-		size = 8;
-		break;
-	case R_X86_64_32:
-		val = (u32)value;
-		break;
-	case R_X86_64_32S:
-		val = (s32)value;
-		break;
-	case R_X86_64_PC32:
-		val = (u32)(value - loc);
-		break;
-	default:
-		/* unsupported relocation type */
-		return -EINVAL;
-	}
-
-	if (loc < core || loc >= core + core_size)
-		/* loc does not point to any symbol inside the module */
-		return -EINVAL;
-
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
-}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 31db7a0..54f62a6 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -64,28 +64,21 @@ struct klp_func {
 };
 
 /**
- * struct klp_reloc - relocation structure for live patching
- * @loc:	address where the relocation will be written
- * @val:	address of the referenced symbol (optional,
- *		vmlinux	patches only)
- * @type:	ELF relocation type
- * @name:	name of the referenced symbol (for lookup/verification)
- * @addend:	offset from the referenced symbol
- * @external:	symbol is either exported or within the live patch module itself
+ * struct klp_reloc_sec - relocation section struct for live patching
+ * @index:	Elf section index of the relocation section
+ * @name:	name of the relocation section
+ * @objname:	name of the object associated with the klp reloc section
  */
-struct klp_reloc {
-	unsigned long loc;
-	unsigned long val;
-	unsigned long type;
-	const char *name;
-	int addend;
-	int external;
+struct klp_reloc_sec {
+	unsigned int index;
+	char *name;
+	char *objname;
 };
 
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
- * @relocs:	relocation entries to be applied at load time
+ * @reloc_secs:	relocation sections to be applied at load time
  * @funcs:	function entries for functions to be patched in the object
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
@@ -95,7 +88,7 @@ struct klp_reloc {
 struct klp_object {
 	/* external */
 	const char *name;
-	struct klp_reloc *relocs;
+	struct klp_reloc_sec *reloc_secs;
 	struct klp_func *funcs;
 
 	/* internal */
@@ -129,6 +122,9 @@ struct klp_patch {
 #define klp_for_each_func(obj, func) \
 	for (func = obj->funcs; func->old_name; func++)
 
+#define klp_for_each_reloc_sec(obj, reloc_sec) \
+	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/include/linux/module.h b/include/linux/module.h
index 9b46256..ea61147 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
 #ifdef CONFIG_DEBUG_SET_MODULE_RONX
 extern void set_all_modules_text_rw(void);
 extern void set_all_modules_text_ro(void);
+extern void
+set_page_attributes(void *start, void *end,
+		    int (*set)(unsigned long start, int num_pages));
 #else
 static inline void set_all_modules_text_rw(void) { }
 static inline void set_all_modules_text_ro(void) { }
+static inline void
+set_page_attributes(void *start, void *end,
+		    int (*set)(unsigned long start, int num_pages)) { }
 #endif
 
 #ifdef CONFIG_GENERIC_BUG
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index db545cb..17b7278 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,6 +28,9 @@
 #include <linux/list.h>
 #include <linux/kallsyms.h>
 #include <linux/livepatch.h>
+#include <linux/elf.h>
+#include <asm/cacheflush.h>
+#include <linux/moduleloader.h>
 
 /**
  * struct klp_ops - structure for tracking registered ftrace ops structs
@@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
 }
 
 static int klp_write_object_relocations(struct module *pmod,
-					struct klp_object *obj)
+					struct klp_object *obj,
+					struct klp_patch *patch)
 {
-	int ret;
-	struct klp_reloc *reloc;
+	int relindex, num_relas;
+	int i, ret = 0;
+	unsigned long addr;
+	char *symname;
+	struct klp_reloc_sec *reloc_sec;
+	Elf_Rela *rela;
+	Elf_Sym *sym, *symtab;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	if (WARN_ON(!obj->relocs))
-		return -EINVAL;
-
-	for (reloc = obj->relocs; reloc->name; reloc++) {
-		if (!klp_is_module(obj)) {
-
-#if defined(CONFIG_RANDOMIZE_BASE)
-			/* If KASLR has been enabled, adjust old value accordingly */
-			if (kaslr_enabled())
-				reloc->val += kaslr_offset();
-#endif
-			ret = klp_verify_vmlinux_symbol(reloc->name,
-							reloc->val);
-			if (ret)
-				return ret;
-		} else {
-			/* module, reloc->val needs to be discovered */
-			if (reloc->external)
-				ret = klp_find_external_symbol(pmod,
-							       reloc->name,
-							       &reloc->val);
-			else
-				ret = klp_find_object_symbol(obj->mod->name,
-							     reloc->name,
-							     &reloc->val);
-			if (ret)
-				return ret;
-		}
-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
-					     reloc->val + reloc->addend);
-		if (ret) {
-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
-			       reloc->name, reloc->val, ret);
-			return ret;
+	symtab = (void *)pmod->core_symtab;
+
+	/* For each __klp_rela section for this object */
+	klp_for_each_reloc_sec(obj, reloc_sec) {
+		relindex = reloc_sec->index;
+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
+
+		/* For each rela in this __klp_rela section */
+		for (i = 0; i < num_relas; i++, rela++) {
+			sym = symtab + ELF_R_SYM(rela->r_info);
+			symname = pmod->core_strtab + sym->st_name;
+
+			if (sym->st_shndx == SHN_LIVEPATCH) {
+				if (sym->st_info == 'K')
+					ret = klp_find_external_symbol(pmod, symname, &addr);
+				else
+					ret = klp_find_object_symbol(obj->name, symname, &addr);
+				if (ret)
+					return ret;
+				sym->st_value = addr;
+			}
 		}
+		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
+					 pmod->index.sym, relindex, pmod);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void notrace klp_ftrace_handler(unsigned long ip,
@@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct module *pmod;
 	int ret;
 
-	if (obj->relocs) {
-		ret = klp_write_object_relocations(patch->mod, obj);
-		if (ret)
-			return ret;
-	}
+	pmod = patch->mod;
+
+	set_page_attributes(pmod->module_core,
+			    pmod->module_core + pmod->core_text_size,
+			    set_memory_rw);
+
+	ret = klp_write_object_relocations(pmod, obj, patch);
+	if (ret)
+		return ret;
+
+	set_page_attributes(pmod->module_core,
+			    pmod->module_core + pmod->core_text_size,
+			    set_memory_ro);
 
 	klp_for_each_func(obj, func) {
 		ret = klp_find_verify_func_addr(obj, func);
-- 
2.4.3

[RFC PATCH v2 5/6] samples: livepatch: init reloc section array and mark as klp module

[Jessica Yu]

Create the array of klp relocation sections in the sample
klp_object (even if the array is empty in this case).

In addition, mark the module as a livepatch module so that
the module loader can appropriately identify and initialize it.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 samples/livepatch/livepatch-sample.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index fb8c861..298dc21 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -53,10 +53,15 @@ static struct klp_func funcs[] = {
 	}, { }
 };
 
+static struct klp_reloc_sec reloc_secs[] = {
+	{ }
+};
+
 static struct klp_object objs[] = {
 	{
 		/* name being NULL means vmlinux */
 		.funcs = funcs,
+		.reloc_secs = reloc_secs,
 	}, { }
 };
 
@@ -89,3 +94,4 @@ static void livepatch_exit(void)
 module_init(livepatch_init);
 module_exit(livepatch_exit);
 MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
2.4.3

[RFC PATCH v2 6/6] Documentation: livepatch: outline the Elf format of a livepatch module

[Jessica Yu]

Document the special Elf sections and constants livepatch modules use.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 Documentation/livepatch/patch-module-format.txt | 117 ++++++++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100644 Documentation/livepatch/patch-module-format.txt

diff --git a/Documentation/livepatch/patch-module-format.txt b/Documentation/livepatch/patch-module-format.txt
new file mode 100644
index 0000000..6118e5d
--- /dev/null
+++ b/Documentation/livepatch/patch-module-format.txt
@@ -0,0 +1,116 @@
+---------------------------
+Livepatch module Elf format
+---------------------------
+
+This document outlines the special Elf constants and sections livepatch
+uses to patch both modules and the kernel (vmlinux).
+
+--------------------------
+1. Livepatch modinfo field
+--------------------------
+
+Livepatch modules can be identified by users by using the 'modinfo' command
+and looking for the presence of the "livepatch" field. This field is also
+used by the kernel module loader to identify livepatch modules.
+
+Example modinfo output:
+
+% modinfo kpatch-meminfo.ko
+filename:		kpatch-meminfo.ko
+livepatch:		Y
+license:		GPL
+depends:
+vermagic:		4.3.0+ SMP mod_unload
+
+--------------------
+2. Livepatch symbols
+--------------------
+
+These are symbols marked with SHN_LIVEPATCH and are manually resolved by
+livepatch. This is useful in cases where we cannot immediately know the
+address of a symbol because the to-be-patched module is not loaded yet.
+livepatch modules keep these symbols in their original symbol tables, and
+the symbol table is made accessible through module->core_symtab.
+
+-----------------------------------
+3. Livepatch "external" symbol bind
+-----------------------------------
+
+The STB_LIVEPATCH_EXT symbol bind is used to help livepatch resolve global
+symbols referenced by klp relocations. After the module is copied into
+memory the module loader actually overwrites each symbol's bind with a
+character (see add_kallsyms() in kernel/module.c), so STB_LIVEPATCH_EXT
+symbols are represented with a capital 'K'.
+
+-----------------------------------
+4. "__klp_rela" relocation sections
+-----------------------------------
+
+A livepatch module uses special Elf relocation sections to apply
+relocations both for regular vmlinux patches as well as those that should
+be applied as soon as the to-be-patched module is loaded. For example, if a
+patch module patches a driver that is not currently loaded, livepatch will
+apply its corresponding klp relocation section(s) to the driver once it
+loads.
+
+The names of these livepatch relocation sections are formatted
+"__klp_rela_${objname}", where ${objname} is the name of the "object" being
+patched (e.g. vmlinux or name of module). Each object within a patch module
+may have multiple klp sections (e.g. patches to multiple functions within
+the same object). There is a 1-1 correspondence between a klp relocation
+section and the target section (usually the text section for a function) to
+which the relocation(s) apply.
+
+Here's a sample readelf output for a livepatch module that patches vmlinux and
+modules 9p, btrfs, ext4:
+  ...
+  [29] __klp_rela_9p.text.caches_show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
+  [30] __klp_rela_btrfs.text.btrfs_feature_attr_show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
+  ...
+  [34] __klp_rela_ext4.text.ext4_attr_store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
+  [35] __klp_rela_ext4.text.ext4_attr_show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
+  [36] __klp_rela_vmlinux.text.cmdline_proc_show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
+  [37] __klp_rela_vmlinux.text.meminfo_proc_show RELA         0000000000000000 003218 0000f0 18 AIo 64  19  8
+  ...
+
+klp relocation sections are SHT_RELA sections but with a few special
+characteristics. Notice that they are marked SHF_ALLOC ("A") so that they
+will not be discarded when the module is loaded into memory, as well as
+with the SHF_RELA_LIVEPATCH flag ("o" - for OS-specific) so the module
+loader can identify them and avoid treating them as regular SHT_RELA
+sections, since they are manually managed by livepatch.
+
+---------------------------------------------------------------
+4.1 How klp relocation sections are represented and initialized
+---------------------------------------------------------------
+
+Livepatch modules must initialize a klp_patch structure to pass in to
+klp_register_patch(). A klp_patch struct contains an array of klp_objects,
+and each klp_object contains an array of klp_reloc_sec structs that
+represent the klp relocation sections that must be applied to that object.
+Each klp_reloc_sec struct is allocated and initialized by the patch module
+code before the call to klp_register_patch().
+
+The klp_reloc_sec structure contains useful metadata about a klp relocation
+section such as its section index and name. Since Elf information is
+preserved for livepatch modules (see Section 5), a klp relocation section
+can be applied simply by passing in the saved section index to
+apply_relocate_add() (in the module loader code), which then uses it to
+access the actual Elf relocation section and apply the relocations.
+
+--------------------------------------------------------
+5. How a livepatch module accesses its symbol table and
+its klp relocation sections
+--------------------------------------------------------
+
+The kernel module loader checks whether the module being loaded is a
+livepatch module. If so, it then makes a copy of the module's Elf header,
+section headers, section name string table, and some noteworthy section
+indices (for example, the symtab's section index). It adjusts the symtab's
+sh_addr to point to mod->core_symtab, since the original mod->symtab lies
+in init memory and gets freed once the module finishes initializing. For
+livepatch modules, the core_symtab will be an exact copy of its original
+symbol table (where normally, only "core" symbols are included in this
+symbol table. See is_core_symbol() in kernel/module.c). livepatch requires
+that the symbols retain their original indices in the symbol table so that
+the klp relocation sections can be applied correctly.
-- 
2.4.3

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:
>Reuse module loader code to write relocations, thereby eliminating the need
>for architecture specific relocation code in livepatch. Namely, we reuse
>apply_relocate_add() in the module loader to write relocations instead of
>duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>are resolved and then apply_relocate_add() is called to apply those
>relocations.
>
>In addition, remove x86 livepatch relocation code. It is no longer needed
>since symbol resolution and relocation work have been offloaded to module
>loader.
>
>Signed-off-by: Jessica Yu <jeyu@redhat.com>
>---
> arch/x86/include/asm/livepatch.h |  2 -
> arch/x86/kernel/Makefile         |  1 -
> arch/x86/kernel/livepatch.c      | 91 --------------------------------------
> include/linux/livepatch.h        | 30 ++++++-------
> include/linux/module.h           |  6 +++
> kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
> 6 files changed, 70 insertions(+), 154 deletions(-)
> delete mode 100644 arch/x86/kernel/livepatch.c
>
>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>index 19c099a..7312e25 100644
>--- a/arch/x86/include/asm/livepatch.h
>+++ b/arch/x86/include/asm/livepatch.h
>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
> #endif
> 	return 0;
> }
>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>-			   unsigned long loc, unsigned long value);
>
> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> {
>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>index b1b78ff..c5e9a5c 100644
>--- a/arch/x86/kernel/Makefile
>+++ b/arch/x86/kernel/Makefile
>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
> obj-y				+= apic/
> obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
> obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
> obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
> obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>deleted file mode 100644
>index d1d35cc..0000000
>--- a/arch/x86/kernel/livepatch.c
>+++ /dev/null
>@@ -1,91 +0,0 @@
>-/*
>- * livepatch.c - x86-specific Kernel Live Patching Core
>- *
>- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
>- * Copyright (C) 2014 SUSE
>- *
>- * 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/>.
>- */
>-
>-#include <linux/module.h>
>-#include <linux/uaccess.h>
>-#include <asm/cacheflush.h>
>-#include <asm/page_types.h>
>-#include <asm/elf.h>
>-#include <asm/livepatch.h>
>-
>-/**
>- * klp_write_module_reloc() - write a relocation in a module
>- * @mod:	module in which the section to be modified is found
>- * @type:	ELF relocation type (see asm/elf.h)
>- * @loc:	address that the relocation should be written to
>- * @value:	relocation value (sym address + addend)
>- *
>- * This function writes a relocation to the specified location for
>- * a particular module.
>- */
>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>-			   unsigned long loc, unsigned long value)
>-{
>-	int ret, numpages, size = 4;
>-	bool readonly;
>-	unsigned long val;
>-	unsigned long core = (unsigned long)mod->module_core;
>-	unsigned long core_size = mod->core_size;
>-
>-	switch (type) {
>-	case R_X86_64_NONE:
>-		return 0;
>-	case R_X86_64_64:
>-		val = value;
>-		size = 8;
>-		break;
>-	case R_X86_64_32:
>-		val = (u32)value;
>-		break;
>-	case R_X86_64_32S:
>-		val = (s32)value;
>-		break;
>-	case R_X86_64_PC32:
>-		val = (u32)(value - loc);
>-		break;
>-	default:
>-		/* unsupported relocation type */
>-		return -EINVAL;
>-	}
>-
>-	if (loc < core || loc >= core + core_size)
>-		/* loc does not point to any symbol inside the module */
>-		return -EINVAL;
>-
>-	readonly = false;
>-
>-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>-	if (loc < core + mod->core_ro_size)
>-		readonly = true;
>-#endif
>-
>-	/* determine if the relocation spans a page boundary */
>-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>-
>-	if (readonly)
>-		set_memory_rw(loc & PAGE_MASK, numpages);
>-
>-	ret = probe_kernel_write((void *)loc, &val, size);
>-
>-	if (readonly)
>-		set_memory_ro(loc & PAGE_MASK, numpages);
>-
>-	return ret;
>-}
>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>index 31db7a0..54f62a6 100644
>--- a/include/linux/livepatch.h
>+++ b/include/linux/livepatch.h
>@@ -64,28 +64,21 @@ struct klp_func {
> };
>
> /**
>- * struct klp_reloc - relocation structure for live patching
>- * @loc:	address where the relocation will be written
>- * @val:	address of the referenced symbol (optional,
>- *		vmlinux	patches only)
>- * @type:	ELF relocation type
>- * @name:	name of the referenced symbol (for lookup/verification)
>- * @addend:	offset from the referenced symbol
>- * @external:	symbol is either exported or within the live patch module itself
>+ * struct klp_reloc_sec - relocation section struct for live patching
>+ * @index:	Elf section index of the relocation section
>+ * @name:	name of the relocation section
>+ * @objname:	name of the object associated with the klp reloc section
>  */
>-struct klp_reloc {
>-	unsigned long loc;
>-	unsigned long val;
>-	unsigned long type;
>-	const char *name;
>-	int addend;
>-	int external;
>+struct klp_reloc_sec {
>+	unsigned int index;
>+	char *name;
>+	char *objname;
> };
>
> /**
>  * struct klp_object - kernel object structure for live patching
>  * @name:	module name (or NULL for vmlinux)
>- * @relocs:	relocation entries to be applied at load time
>+ * @reloc_secs:	relocation sections to be applied at load time
>  * @funcs:	function entries for functions to be patched in the object
>  * @kobj:	kobject for sysfs resources
>  * @mod:	kernel module associated with the patched object
>@@ -95,7 +88,7 @@ struct klp_reloc {
> struct klp_object {
> 	/* external */
> 	const char *name;
>-	struct klp_reloc *relocs;
>+	struct klp_reloc_sec *reloc_secs;
> 	struct klp_func *funcs;
>
> 	/* internal */
>@@ -129,6 +122,9 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> 	for (func = obj->funcs; func->old_name; func++)
>
>+#define klp_for_each_reloc_sec(obj, reloc_sec) \
>+	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>+
> int klp_register_patch(struct klp_patch *);
> int klp_unregister_patch(struct klp_patch *);
> int klp_enable_patch(struct klp_patch *);
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 9b46256..ea61147 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
> #ifdef CONFIG_DEBUG_SET_MODULE_RONX
> extern void set_all_modules_text_rw(void);
> extern void set_all_modules_text_ro(void);
>+extern void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages));
> #else
> static inline void set_all_modules_text_rw(void) { }
> static inline void set_all_modules_text_ro(void) { }
>+static inline void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages)) { }
> #endif
>
> #ifdef CONFIG_GENERIC_BUG
>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>index db545cb..17b7278 100644
>--- a/kernel/livepatch/core.c
>+++ b/kernel/livepatch/core.c
>@@ -28,6 +28,9 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
>+#include <linux/elf.h>
>+#include <asm/cacheflush.h>
>+#include <linux/moduleloader.h>
>
> /**
>  * struct klp_ops - structure for tracking registered ftrace ops structs
>@@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
> }
>
> static int klp_write_object_relocations(struct module *pmod,
>-					struct klp_object *obj)
>+					struct klp_object *obj,
>+					struct klp_patch *patch)
> {
>-	int ret;
>-	struct klp_reloc *reloc;
>+	int relindex, num_relas;
>+	int i, ret = 0;
>+	unsigned long addr;
>+	char *symname;
>+	struct klp_reloc_sec *reloc_sec;
>+	Elf_Rela *rela;
>+	Elf_Sym *sym, *symtab;
>
> 	if (WARN_ON(!klp_is_object_loaded(obj)))
> 		return -EINVAL;
>
>-	if (WARN_ON(!obj->relocs))
>-		return -EINVAL;
>-
>-	for (reloc = obj->relocs; reloc->name; reloc++) {
>-		if (!klp_is_module(obj)) {
>-
>-#if defined(CONFIG_RANDOMIZE_BASE)
>-			/* If KASLR has been enabled, adjust old value accordingly */
>-			if (kaslr_enabled())
>-				reloc->val += kaslr_offset();
>-#endif
>-			ret = klp_verify_vmlinux_symbol(reloc->name,
>-							reloc->val);
>-			if (ret)
>-				return ret;
>-		} else {
>-			/* module, reloc->val needs to be discovered */
>-			if (reloc->external)
>-				ret = klp_find_external_symbol(pmod,
>-							       reloc->name,
>-							       &reloc->val);
>-			else
>-				ret = klp_find_object_symbol(obj->mod->name,
>-							     reloc->name,
>-							     &reloc->val);
>-			if (ret)
>-				return ret;
>-		}
>-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>-					     reloc->val + reloc->addend);
>-		if (ret) {
>-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>-			       reloc->name, reloc->val, ret);
>-			return ret;
>+	symtab = (void *)pmod->core_symtab;
>+
>+	/* For each __klp_rela section for this object */
>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>+		relindex = reloc_sec->index;
>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>+
>+		/* For each rela in this __klp_rela section */
>+		for (i = 0; i < num_relas; i++, rela++) {
>+			sym = symtab + ELF_R_SYM(rela->r_info);
>+			symname = pmod->core_strtab + sym->st_name;
>+
>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>+				if (sym->st_info == 'K')
>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>+				else
>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>+				if (ret)
>+					return ret;
>+				sym->st_value = addr;

I'll note here that this patchset has the same problem that current
livepatch has in that it will be unable to handle duplicate symbols
within the same object (i.e. sysfs will be unable to create entries
with the same name and livepatch errors out). After the Chris's sympos
patchset gets merged I should be able to rebase this patchset on top
of it to resolve the duplicate symbol issue.

>+			}
> 		}
>+		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>+					 pmod->index.sym, relindex, pmod);
> 	}
>
>-	return 0;
>+	return ret;
> }
>
> static void notrace klp_ftrace_handler(unsigned long ip,
>@@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> 				  struct klp_object *obj)
> {
> 	struct klp_func *func;
>+	struct module *pmod;
> 	int ret;
>
>-	if (obj->relocs) {
>-		ret = klp_write_object_relocations(patch->mod, obj);
>-		if (ret)
>-			return ret;
>-	}
>+	pmod = patch->mod;
>+
>+	set_page_attributes(pmod->module_core,
>+			    pmod->module_core + pmod->core_text_size,
>+			    set_memory_rw);

Note #2: When Josh's module cleanup patches get merged, I'll swap out these
set_page_attribute() calls for (un)set_module_core_ro_nx().

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:
>Reuse module loader code to write relocations, thereby eliminating the need
>for architecture specific relocation code in livepatch. Namely, we reuse
>apply_relocate_add() in the module loader to write relocations instead of
>duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>are resolved and then apply_relocate_add() is called to apply those
>relocations.
>
>In addition, remove x86 livepatch relocation code. It is no longer needed
>since symbol resolution and relocation work have been offloaded to module
>loader.
>
>Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>---
> arch/x86/include/asm/livepatch.h |  2 -
> arch/x86/kernel/Makefile         |  1 -
> arch/x86/kernel/livepatch.c      | 91 --------------------------------------
> include/linux/livepatch.h        | 30 ++++++-------
> include/linux/module.h           |  6 +++
> kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
> 6 files changed, 70 insertions(+), 154 deletions(-)
> delete mode 100644 arch/x86/kernel/livepatch.c
>
>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>index 19c099a..7312e25 100644
>--- a/arch/x86/include/asm/livepatch.h
>+++ b/arch/x86/include/asm/livepatch.h
>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
> #endif
> 	return 0;
> }
>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>-			   unsigned long loc, unsigned long value);
>
> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> {
>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>index b1b78ff..c5e9a5c 100644
>--- a/arch/x86/kernel/Makefile
>+++ b/arch/x86/kernel/Makefile
>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
> obj-y				+= apic/
> obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
> obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
> obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
> obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>deleted file mode 100644
>index d1d35cc..0000000
>--- a/arch/x86/kernel/livepatch.c
>+++ /dev/null
>@@ -1,91 +0,0 @@
>-/*
>- * livepatch.c - x86-specific Kernel Live Patching Core
>- *
>- * Copyright (C) 2014 Seth Jennings <sjenning-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>- * Copyright (C) 2014 SUSE
>- *
>- * 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/>.
>- */
>-
>-#include <linux/module.h>
>-#include <linux/uaccess.h>
>-#include <asm/cacheflush.h>
>-#include <asm/page_types.h>
>-#include <asm/elf.h>
>-#include <asm/livepatch.h>
>-
>-/**
>- * klp_write_module_reloc() - write a relocation in a module
>- * @mod:	module in which the section to be modified is found
>- * @type:	ELF relocation type (see asm/elf.h)
>- * @loc:	address that the relocation should be written to
>- * @value:	relocation value (sym address + addend)
>- *
>- * This function writes a relocation to the specified location for
>- * a particular module.
>- */
>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>-			   unsigned long loc, unsigned long value)
>-{
>-	int ret, numpages, size = 4;
>-	bool readonly;
>-	unsigned long val;
>-	unsigned long core = (unsigned long)mod->module_core;
>-	unsigned long core_size = mod->core_size;
>-
>-	switch (type) {
>-	case R_X86_64_NONE:
>-		return 0;
>-	case R_X86_64_64:
>-		val = value;
>-		size = 8;
>-		break;
>-	case R_X86_64_32:
>-		val = (u32)value;
>-		break;
>-	case R_X86_64_32S:
>-		val = (s32)value;
>-		break;
>-	case R_X86_64_PC32:
>-		val = (u32)(value - loc);
>-		break;
>-	default:
>-		/* unsupported relocation type */
>-		return -EINVAL;
>-	}
>-
>-	if (loc < core || loc >= core + core_size)
>-		/* loc does not point to any symbol inside the module */
>-		return -EINVAL;
>-
>-	readonly = false;
>-
>-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>-	if (loc < core + mod->core_ro_size)
>-		readonly = true;
>-#endif
>-
>-	/* determine if the relocation spans a page boundary */
>-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>-
>-	if (readonly)
>-		set_memory_rw(loc & PAGE_MASK, numpages);
>-
>-	ret = probe_kernel_write((void *)loc, &val, size);
>-
>-	if (readonly)
>-		set_memory_ro(loc & PAGE_MASK, numpages);
>-
>-	return ret;
>-}
>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>index 31db7a0..54f62a6 100644
>--- a/include/linux/livepatch.h
>+++ b/include/linux/livepatch.h
>@@ -64,28 +64,21 @@ struct klp_func {
> };
>
> /**
>- * struct klp_reloc - relocation structure for live patching
>- * @loc:	address where the relocation will be written
>- * @val:	address of the referenced symbol (optional,
>- *		vmlinux	patches only)
>- * @type:	ELF relocation type
>- * @name:	name of the referenced symbol (for lookup/verification)
>- * @addend:	offset from the referenced symbol
>- * @external:	symbol is either exported or within the live patch module itself
>+ * struct klp_reloc_sec - relocation section struct for live patching
>+ * @index:	Elf section index of the relocation section
>+ * @name:	name of the relocation section
>+ * @objname:	name of the object associated with the klp reloc section
>  */
>-struct klp_reloc {
>-	unsigned long loc;
>-	unsigned long val;
>-	unsigned long type;
>-	const char *name;
>-	int addend;
>-	int external;
>+struct klp_reloc_sec {
>+	unsigned int index;
>+	char *name;
>+	char *objname;
> };
>
> /**
>  * struct klp_object - kernel object structure for live patching
>  * @name:	module name (or NULL for vmlinux)
>- * @relocs:	relocation entries to be applied at load time
>+ * @reloc_secs:	relocation sections to be applied at load time
>  * @funcs:	function entries for functions to be patched in the object
>  * @kobj:	kobject for sysfs resources
>  * @mod:	kernel module associated with the patched object
>@@ -95,7 +88,7 @@ struct klp_reloc {
> struct klp_object {
> 	/* external */
> 	const char *name;
>-	struct klp_reloc *relocs;
>+	struct klp_reloc_sec *reloc_secs;
> 	struct klp_func *funcs;
>
> 	/* internal */
>@@ -129,6 +122,9 @@ struct klp_patch {
> #define klp_for_each_func(obj, func) \
> 	for (func = obj->funcs; func->old_name; func++)
>
>+#define klp_for_each_reloc_sec(obj, reloc_sec) \
>+	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>+
> int klp_register_patch(struct klp_patch *);
> int klp_unregister_patch(struct klp_patch *);
> int klp_enable_patch(struct klp_patch *);
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 9b46256..ea61147 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
> #ifdef CONFIG_DEBUG_SET_MODULE_RONX
> extern void set_all_modules_text_rw(void);
> extern void set_all_modules_text_ro(void);
>+extern void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages));
> #else
> static inline void set_all_modules_text_rw(void) { }
> static inline void set_all_modules_text_ro(void) { }
>+static inline void
>+set_page_attributes(void *start, void *end,
>+		    int (*set)(unsigned long start, int num_pages)) { }
> #endif
>
> #ifdef CONFIG_GENERIC_BUG
>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>index db545cb..17b7278 100644
>--- a/kernel/livepatch/core.c
>+++ b/kernel/livepatch/core.c
>@@ -28,6 +28,9 @@
> #include <linux/list.h>
> #include <linux/kallsyms.h>
> #include <linux/livepatch.h>
>+#include <linux/elf.h>
>+#include <asm/cacheflush.h>
>+#include <linux/moduleloader.h>
>
> /**
>  * struct klp_ops - structure for tracking registered ftrace ops structs
>@@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
> }
>
> static int klp_write_object_relocations(struct module *pmod,
>-					struct klp_object *obj)
>+					struct klp_object *obj,
>+					struct klp_patch *patch)
> {
>-	int ret;
>-	struct klp_reloc *reloc;
>+	int relindex, num_relas;
>+	int i, ret = 0;
>+	unsigned long addr;
>+	char *symname;
>+	struct klp_reloc_sec *reloc_sec;
>+	Elf_Rela *rela;
>+	Elf_Sym *sym, *symtab;
>
> 	if (WARN_ON(!klp_is_object_loaded(obj)))
> 		return -EINVAL;
>
>-	if (WARN_ON(!obj->relocs))
>-		return -EINVAL;
>-
>-	for (reloc = obj->relocs; reloc->name; reloc++) {
>-		if (!klp_is_module(obj)) {
>-
>-#if defined(CONFIG_RANDOMIZE_BASE)
>-			/* If KASLR has been enabled, adjust old value accordingly */
>-			if (kaslr_enabled())
>-				reloc->val += kaslr_offset();
>-#endif
>-			ret = klp_verify_vmlinux_symbol(reloc->name,
>-							reloc->val);
>-			if (ret)
>-				return ret;
>-		} else {
>-			/* module, reloc->val needs to be discovered */
>-			if (reloc->external)
>-				ret = klp_find_external_symbol(pmod,
>-							       reloc->name,
>-							       &reloc->val);
>-			else
>-				ret = klp_find_object_symbol(obj->mod->name,
>-							     reloc->name,
>-							     &reloc->val);
>-			if (ret)
>-				return ret;
>-		}
>-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>-					     reloc->val + reloc->addend);
>-		if (ret) {
>-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>-			       reloc->name, reloc->val, ret);
>-			return ret;
>+	symtab = (void *)pmod->core_symtab;
>+
>+	/* For each __klp_rela section for this object */
>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>+		relindex = reloc_sec->index;
>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>+
>+		/* For each rela in this __klp_rela section */
>+		for (i = 0; i < num_relas; i++, rela++) {
>+			sym = symtab + ELF_R_SYM(rela->r_info);
>+			symname = pmod->core_strtab + sym->st_name;
>+
>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>+				if (sym->st_info == 'K')
>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>+				else
>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>+				if (ret)
>+					return ret;
>+				sym->st_value = addr;

I'll note here that this patchset has the same problem that current
livepatch has in that it will be unable to handle duplicate symbols
within the same object (i.e. sysfs will be unable to create entries
with the same name and livepatch errors out). After the Chris's sympos
patchset gets merged I should be able to rebase this patchset on top
of it to resolve the duplicate symbol issue.

>+			}
> 		}
>+		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>+					 pmod->index.sym, relindex, pmod);
> 	}
>
>-	return 0;
>+	return ret;
> }
>
> static void notrace klp_ftrace_handler(unsigned long ip,
>@@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> 				  struct klp_object *obj)
> {
> 	struct klp_func *func;
>+	struct module *pmod;
> 	int ret;
>
>-	if (obj->relocs) {
>-		ret = klp_write_object_relocations(patch->mod, obj);
>-		if (ret)
>-			return ret;
>-	}
>+	pmod = patch->mod;
>+
>+	set_page_attributes(pmod->module_core,
>+			    pmod->module_core + pmod->core_text_size,
>+			    set_memory_rw);

Note #2: When Josh's module cleanup patches get merged, I'll swap out these
set_page_attribute() calls for (un)set_module_core_ro_nx().

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:
>For livepatch modules, copy Elf section, symbol, and string information
>from the load_info struct in the module loader.
>
>Livepatch uses special relocation sections in order to be able to patch
>modules that are not yet loaded, as well as apply patches to the kernel
>when the addresses of symbols cannot be determined at compile time (for
>example, when kaslr is enabled). Livepatch modules must preserve Elf
>information such as section indices in order to apply the remaining
>relocation sections at the appropriate time (i.e. when the target module
>loads).
>
>Signed-off-by: Jessica Yu <jeyu@redhat.com>
>---
> include/linux/module.h |  9 +++++
> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 105 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 3a19c79..9b46256 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -425,6 +425,14 @@ struct module {
>
> 	/* Notes attributes */
> 	struct module_notes_attrs *notes_attrs;
>+
>+	/* Elf information (optionally saved) */
>+	Elf_Ehdr *hdr;
>+	Elf_Shdr *sechdrs;
>+	char *secstrings;
>+	struct {
>+		unsigned int sym, str, mod, vers, info, pcpu;
>+	} index;
> #endif
>
> 	/* The command line arguments (may be mangled).  People like
>@@ -461,6 +469,7 @@ struct module {
> #endif
>
> #ifdef CONFIG_LIVEPATCH
>+	bool klp; /* Is this a livepatch module? */

Gah. I believe this field should be outside the #ifdef. 

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:
>For livepatch modules, copy Elf section, symbol, and string information
>from the load_info struct in the module loader.
>
>Livepatch uses special relocation sections in order to be able to patch
>modules that are not yet loaded, as well as apply patches to the kernel
>when the addresses of symbols cannot be determined at compile time (for
>example, when kaslr is enabled). Livepatch modules must preserve Elf
>information such as section indices in order to apply the remaining
>relocation sections at the appropriate time (i.e. when the target module
>loads).
>
>Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>---
> include/linux/module.h |  9 +++++
> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 105 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 3a19c79..9b46256 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -425,6 +425,14 @@ struct module {
>
> 	/* Notes attributes */
> 	struct module_notes_attrs *notes_attrs;
>+
>+	/* Elf information (optionally saved) */
>+	Elf_Ehdr *hdr;
>+	Elf_Shdr *sechdrs;
>+	char *secstrings;
>+	struct {
>+		unsigned int sym, str, mod, vers, info, pcpu;
>+	} index;
> #endif
>
> 	/* The command line arguments (may be mangled).  People like
>@@ -461,6 +469,7 @@ struct module {
> #endif
>
> #ifdef CONFIG_LIVEPATCH
>+	bool klp; /* Is this a livepatch module? */

Gah. I believe this field should be outside the #ifdef. 

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Jessica Yu [30/11/15 23:21 -0500]:
>For livepatch modules, copy Elf section, symbol, and string information
>from the load_info struct in the module loader.
>
>Livepatch uses special relocation sections in order to be able to patch
>modules that are not yet loaded, as well as apply patches to the kernel
>when the addresses of symbols cannot be determined at compile time (for
>example, when kaslr is enabled). Livepatch modules must preserve Elf
>information such as section indices in order to apply the remaining
>relocation sections at the appropriate time (i.e. when the target module
>loads).
>
>Signed-off-by: Jessica Yu <jeyu@redhat.com>
>---
> include/linux/module.h |  9 +++++
> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 105 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 3a19c79..9b46256 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -425,6 +425,14 @@ struct module {
>
> 	/* Notes attributes */
> 	struct module_notes_attrs *notes_attrs;
>+
>+	/* Elf information (optionally saved) */
>+	Elf_Ehdr *hdr;
>+	Elf_Shdr *sechdrs;
>+	char *secstrings;
>+	struct {
>+		unsigned int sym, str, mod, vers, info, pcpu;
>+	} index;
> #endif

This particular patch unforunately breaks !CONFIG_KALLSYMS kernel
builds, as I've just discovered. These fields should move out of the
CONFIG_KALLSYMS block. And..

> 	/* The command line arguments (may be mangled).  People like
>@@ -461,6 +469,7 @@ struct module {
> #endif
>
> #ifdef CONFIG_LIVEPATCH
>+	bool klp; /* Is this a livepatch module? */
> 	bool klp_alive;
> #endif
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 8f051a1..433c2d6 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
> static void unset_module_init_ro_nx(struct module *mod) { }
> #endif
>
>+static void free_module_elf(struct module *mod)
>+{
>+	kfree(mod->hdr);
>+	kfree(mod->sechdrs);
>+	kfree(mod->secstrings);
>+}
>+
> void __weak module_memfree(void *module_region)
> {
> 	vfree(module_region);
>@@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
> 	/* Free any allocated parameters. */
> 	destroy_params(mod->kp, mod->num_kp);
>
>+	/* Free Elf information if it was saved */
>+	free_module_elf(mod);
>+
> 	/* Now we can delete it from the lists */
> 	mutex_lock(&module_mutex);
> 	/* Unlink carefully: kallsyms could be walking list. */
>@@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
> 			       (long)sym[i].st_value);
> 			break;
>
>+		case SHN_LIVEPATCH:
>+			/* klp symbols are resolved by livepatch */
>+			break;
>+
> 		case SHN_UNDEF:
> 			ksym = resolve_symbol_wait(mod, info, name);
> 			/* Ok if resolved.  */
>@@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> 			continue;
>
>+		/* klp relocation sections are applied by livepatch */
>+		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
>+			continue;
>+
> 		if (info->sechdrs[i].sh_type == SHT_REL)
> 			err = apply_relocate(info->sechdrs, info->strtab,
> 					     info->index.sym, i, mod);
>@@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
> {
> 	const Elf_Shdr *sechdrs = info->sechdrs;
>
>+	if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
>+		return 'K';
>+	if (sym->st_shndx == SHN_LIVEPATCH)
>+		return 'k';
>+
> 	if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
> 		if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
> 			return 'v';
>@@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>
> 	/* Compute total space required for the core symbols' strtab. */
> 	for (ndst = i = 0; i < nsrc; i++) {
>-		if (i == 0 ||
>+		if (i == 0 || mod->klp ||
> 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
> 			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
> 			ndst++;
>@@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> 	mod->core_strtab = s = mod->module_core + info->stroffs;
> 	src = mod->symtab;
> 	for (ndst = i = 0; i < mod->num_symtab; i++) {
>-		if (i == 0 ||
>+		if (i == 0 || mod->klp ||
> 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
> 			dst[ndst] = src[i];
> 			dst[ndst++].st_name = s - mod->core_strtab;
>@@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
> 	return 0;
> }
>
>+/*
>+ * copy_module_elf - preserve Elf information about a module
>+ */
>+static int copy_module_elf(struct module *mod, struct load_info *info)
>+{
>+	unsigned int size;
>+	int ret = 0;
>+	Elf_Shdr *symsect;
>+
>+	/* Elf header */
>+	size = sizeof(Elf_Ehdr);
>+	mod->hdr = kzalloc(size, GFP_KERNEL);
>+	if (mod->hdr == NULL) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>+	memcpy(mod->hdr, info->hdr, size);
>+
>+	/* Elf section header table */
>+	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
>+	mod->sechdrs = kzalloc(size, GFP_KERNEL);
>+	if (mod->sechdrs == NULL) {
>+		ret = -ENOMEM;
>+		goto free_hdr;
>+	}
>+	memcpy(mod->sechdrs, info->sechdrs, size);
>+
>+	/* Elf section name string table */
>+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
>+	mod->secstrings = kzalloc(size, GFP_KERNEL);
>+	if (mod->secstrings == NULL) {
>+		ret = -ENOMEM;
>+		goto free_sechdrs;
>+	}
>+	memcpy(mod->secstrings, info->secstrings, size);
>+
>+	/* Elf section indices */
>+	memcpy(&mod->index, &info->index, sizeof(info->index));
>+
>+	/*
>+	 * Update symtab's sh_addr to point to a valid
>+	 * symbol table, as the temporary symtab in module
>+	 * init memory will be freed
>+	 */
>+	symsect = mod->sechdrs + mod->index.sym;
>+	symsect->sh_addr = (unsigned long)mod->core_symtab;

..instead of relying on CONFIG_KALLSYMS being set, check for
CONFIG_KALLSYMS before the sh_addr assignment above (mod->core_symtab
only exists when CONFIG_KALLSYMS is set). Then we can leave
{free,copy}_module_elf() outside any #ifdef blocks.

Jessica

Re: [RFC PATCH v2 2/6] module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/module.h |  9 +++++
>  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>  	/* Notes attributes */
>  	struct module_notes_attrs *notes_attrs;
> +
> +	/* Elf information (optionally saved) */
> +	Elf_Ehdr *hdr;

I would rename "hdr" to "elf_hdr" to make its purpose clearer.

> +	Elf_Shdr *sechdrs;
> +	char *secstrings;

Probably a good idea to add underscores to the names ("sec_hdrs" and
"sec_strings") to be consistent with most of the other fields in the
struct.

> +	struct {
> +		unsigned int sym, str, mod, vers, info, pcpu;
> +	} index;

I might be contradicting myself from what I said before.  But I'm
thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
Then below, there could be two versions of copy_module_elf(), the real
one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
story for free_module_elf().

>  #endif
>  
>  	/* The command line arguments (may be mangled).  People like
> @@ -461,6 +469,7 @@ struct module {
>  #endif
>  
>  #ifdef CONFIG_LIVEPATCH
> +	bool klp; /* Is this a livepatch module? */
>  	bool klp_alive;
>  #endif
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..433c2d6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
>  static void unset_module_init_ro_nx(struct module *mod) { }
>  #endif
>  
> +static void free_module_elf(struct module *mod)
> +{
> +	kfree(mod->hdr);
> +	kfree(mod->sechdrs);
> +	kfree(mod->secstrings);
> +}
> +
>  void __weak module_memfree(void *module_region)
>  {
>  	vfree(module_region);
> @@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
>  	/* Free any allocated parameters. */
>  	destroy_params(mod->kp, mod->num_kp);
>  
> +	/* Free Elf information if it was saved */
> +	free_module_elf(mod);
> +
>  	/* Now we can delete it from the lists */
>  	mutex_lock(&module_mutex);
>  	/* Unlink carefully: kallsyms could be walking list. */
> @@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			       (long)sym[i].st_value);
>  			break;
>  
> +		case SHN_LIVEPATCH:
> +			/* klp symbols are resolved by livepatch */
> +			break;
> +
>  		case SHN_UNDEF:
>  			ksym = resolve_symbol_wait(mod, info, name);
>  			/* Ok if resolved.  */
> @@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>  			continue;
>  
> +		/* klp relocation sections are applied by livepatch */
> +		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> +			continue;
> +
>  		if (info->sechdrs[i].sh_type == SHT_REL)
>  			err = apply_relocate(info->sechdrs, info->strtab,
>  					     info->index.sym, i, mod);
> @@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
>  {
>  	const Elf_Shdr *sechdrs = info->sechdrs;
>  
> +	if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
> +		return 'K';
> +	if (sym->st_shndx == SHN_LIVEPATCH)
> +		return 'k';
> +
>  	if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
>  		if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
>  			return 'v';
> @@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>  
>  	/* Compute total space required for the core symbols' strtab. */
>  	for (ndst = i = 0; i < nsrc; i++) {
> -		if (i == 0 ||
> +		if (i == 0 || mod->klp ||
>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>  			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
>  			ndst++;

Instead of accessing mod->klp directly, how about an
'is_livepatch_module(mod)' function.  There could be two versions, with
the !LIVEPATCH version always returning false and the LIVEPATCH version
checking mod->klp.  Then mod->klp itself can stay inside the LIVEPATCH
ifdef in the module struct.

> @@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>  	mod->core_strtab = s = mod->module_core + info->stroffs;
>  	src = mod->symtab;
>  	for (ndst = i = 0; i < mod->num_symtab; i++) {
> -		if (i == 0 ||
> +		if (i == 0 || mod->klp ||
>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>  			dst[ndst] = src[i];
>  			dst[ndst++].st_name = s - mod->core_strtab;
> @@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
>  	return 0;
>  }
>  
> +/*
> + * copy_module_elf - preserve Elf information about a module
> + */
> +static int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	unsigned int size;
> +	int ret = 0;

No need to initialize ret to zero here since it's never used
uninitalized.

> +	Elf_Shdr *symsect;
> +
> +	/* Elf header */
> +	size = sizeof(Elf_Ehdr);
> +	mod->hdr = kzalloc(size, GFP_KERNEL);

No need to zero the memory here with kzalloc() since it will all be
memcpy()'d anyway.  kmalloc() can be used instead (and the same for the
other kzalloc()s below).

> +	if (mod->hdr == NULL) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	memcpy(mod->hdr, info->hdr, size);
> +
> +	/* Elf section header table */
> +	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
> +	mod->sechdrs = kzalloc(size, GFP_KERNEL);
> +	if (mod->sechdrs == NULL) {
> +		ret = -ENOMEM;
> +		goto free_hdr;
> +	}
> +	memcpy(mod->sechdrs, info->sechdrs, size);
> +
> +	/* Elf section name string table */
> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> +	mod->secstrings = kzalloc(size, GFP_KERNEL);
> +	if (mod->secstrings == NULL) {
> +		ret = -ENOMEM;
> +		goto free_sechdrs;
> +	}
> +	memcpy(mod->secstrings, info->secstrings, size);
> +
> +	/* Elf section indices */
> +	memcpy(&mod->index, &info->index, sizeof(info->index));
> +
> +	/*
> +	 * Update symtab's sh_addr to point to a valid
> +	 * symbol table, as the temporary symtab in module
> +	 * init memory will be freed
> +	 */
> +	symsect = mod->sechdrs + mod->index.sym;
> +	symsect->sh_addr = (unsigned long)mod->core_symtab;
> +
> +	return ret;
> +
> +free_sechdrs:
> +	kfree(mod->sechdrs);
> +free_hdr:
> +	kfree(mod->hdr);
> +out:
> +	return ret;
> +}
> +
> +
>  #define COPY_CHUNK_SIZE (16*PAGE_SIZE)
>  
>  static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned long len)
> @@ -2866,6 +2947,9 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>  			"is unknown, you have been warned.\n", mod->name);
>  	}
>  
> +	if (get_modinfo(info, "livepatch"))
> +		mod->klp = true;
> +

Similar to the is_livepatch_module() function I suggested, this can be
put in a function so that mod->klp can be abstracted away for the
!LIVEPATCH case.  Maybe there should be a check_livepatch_modinfo()
function:

1. the !LIVEPATCH version of the function could return an error if
modinfo has "livepatch"

2. the LIVEPATCH version could simply set mod->klp = true.

>  	/* Set up license info based on the info section */
>  	set_license(mod, get_modinfo(info, "license"));
>  
> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto bug_cleanup;
>  
> +	/*
> +	 * Save sechdrs, indices, and other data from info
> +	 * in order to patch to-be-loaded modules.
> +	 * Do not call free_copy() for livepatch modules.

I think the last line of this comment isn't right, since free_copy() is
called below regardless.

> +	 */
> +	if (mod->klp)
> +		err = copy_module_elf(mod, info);
> +	if (err < 0)
> +		goto bug_cleanup;

Not strictly necessary, but I think it would be a little cleaner to only
check the err if copy_module_elf() was called.

> +
>  	/* Get rid of temporary copy. */
>  	free_copy(info);

-- 
Josh

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Mon, Nov 30, 2015 at 11:21:17PM -0500, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the need
> for architecture specific relocation code in livepatch. Namely, we reuse
> apply_relocate_add() in the module loader to write relocations instead of
> duplicating functionality in livepatch's klp_write_module_reloc(). To apply
> relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
> are resolved and then apply_relocate_add() is called to apply those
> relocations.
> 
> In addition, remove x86 livepatch relocation code. It is no longer needed
> since symbol resolution and relocation work have been offloaded to module
> loader.   
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  arch/x86/include/asm/livepatch.h |  2 -
>  arch/x86/kernel/Makefile         |  1 -
>  arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>  include/linux/livepatch.h        | 30 ++++++-------
>  include/linux/module.h           |  6 +++
>  kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>  6 files changed, 70 insertions(+), 154 deletions(-)
>  delete mode 100644 arch/x86/kernel/livepatch.c
> 
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 19c099a..7312e25 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>  #endif
>  	return 0;
>  }
> -int klp_write_module_reloc(struct module *mod, unsigned long type,
> -			   unsigned long loc, unsigned long value);
>  
>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>  {
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index b1b78ff..c5e9a5c 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>  obj-y				+= apic/
>  obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
> -obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>  obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>  obj-$(CONFIG_X86_TSC)		+= trace_clock.o
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> deleted file mode 100644
> index d1d35cc..0000000
> --- a/arch/x86/kernel/livepatch.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * livepatch.c - x86-specific Kernel Live Patching Core
> - *
> - * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
> - * Copyright (C) 2014 SUSE
> - *
> - * 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/>.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/uaccess.h>
> -#include <asm/cacheflush.h>
> -#include <asm/page_types.h>
> -#include <asm/elf.h>
> -#include <asm/livepatch.h>
> -
> -/**
> - * klp_write_module_reloc() - write a relocation in a module
> - * @mod:	module in which the section to be modified is found
> - * @type:	ELF relocation type (see asm/elf.h)
> - * @loc:	address that the relocation should be written to
> - * @value:	relocation value (sym address + addend)
> - *
> - * This function writes a relocation to the specified location for
> - * a particular module.
> - */
> -int klp_write_module_reloc(struct module *mod, unsigned long type,
> -			   unsigned long loc, unsigned long value)
> -{
> -	int ret, numpages, size = 4;
> -	bool readonly;
> -	unsigned long val;
> -	unsigned long core = (unsigned long)mod->module_core;
> -	unsigned long core_size = mod->core_size;
> -
> -	switch (type) {
> -	case R_X86_64_NONE:
> -		return 0;
> -	case R_X86_64_64:
> -		val = value;
> -		size = 8;
> -		break;
> -	case R_X86_64_32:
> -		val = (u32)value;
> -		break;
> -	case R_X86_64_32S:
> -		val = (s32)value;
> -		break;
> -	case R_X86_64_PC32:
> -		val = (u32)(value - loc);
> -		break;
> -	default:
> -		/* unsupported relocation type */
> -		return -EINVAL;
> -	}
> -
> -	if (loc < core || loc >= core + core_size)
> -		/* loc does not point to any symbol inside the module */
> -		return -EINVAL;
> -
> -	readonly = false;
> -
> -#ifdef CONFIG_DEBUG_SET_MODULE_RONX
> -	if (loc < core + mod->core_ro_size)
> -		readonly = true;
> -#endif
> -
> -	/* determine if the relocation spans a page boundary */
> -	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
> -
> -	if (readonly)
> -		set_memory_rw(loc & PAGE_MASK, numpages);
> -
> -	ret = probe_kernel_write((void *)loc, &val, size);
> -
> -	if (readonly)
> -		set_memory_ro(loc & PAGE_MASK, numpages);
> -
> -	return ret;
> -}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 31db7a0..54f62a6 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -64,28 +64,21 @@ struct klp_func {
>  };
>  
>  /**
> - * struct klp_reloc - relocation structure for live patching
> - * @loc:	address where the relocation will be written
> - * @val:	address of the referenced symbol (optional,
> - *		vmlinux	patches only)
> - * @type:	ELF relocation type
> - * @name:	name of the referenced symbol (for lookup/verification)
> - * @addend:	offset from the referenced symbol
> - * @external:	symbol is either exported or within the live patch module itself
> + * struct klp_reloc_sec - relocation section struct for live patching
> + * @index:	Elf section index of the relocation section
> + * @name:	name of the relocation section
> + * @objname:	name of the object associated with the klp reloc section
>   */
> -struct klp_reloc {
> -	unsigned long loc;
> -	unsigned long val;
> -	unsigned long type;
> -	const char *name;
> -	int addend;
> -	int external;
> +struct klp_reloc_sec {
> +	unsigned int index;
> +	char *name;
> +	char *objname;
>  };
>  
>  /**
>   * struct klp_object - kernel object structure for live patching
>   * @name:	module name (or NULL for vmlinux)
> - * @relocs:	relocation entries to be applied at load time
> + * @reloc_secs:	relocation sections to be applied at load time
>   * @funcs:	function entries for functions to be patched in the object
>   * @kobj:	kobject for sysfs resources
>   * @mod:	kernel module associated with the patched object
> @@ -95,7 +88,7 @@ struct klp_reloc {
>  struct klp_object {
>  	/* external */
>  	const char *name;
> -	struct klp_reloc *relocs;
> +	struct klp_reloc_sec *reloc_secs;

There was a lot of discussion for v1, so I'm not sure, but I thought we
ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
think the symbol table can be walked directly looking for klp rela
sections?

The benefits of doing so would be to make the interface simpler -- no
need to "cache" the section metadata when it's already easily and
quickly available in the module struct elf section headers.

>  	struct klp_func *funcs;
>  
>  	/* internal */
> @@ -129,6 +122,9 @@ struct klp_patch {
>  #define klp_for_each_func(obj, func) \
>  	for (func = obj->funcs; func->old_name; func++)
>  
> +#define klp_for_each_reloc_sec(obj, reloc_sec) \
> +	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
> +
>  int klp_register_patch(struct klp_patch *);
>  int klp_unregister_patch(struct klp_patch *);
>  int klp_enable_patch(struct klp_patch *);
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9b46256..ea61147 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
>  #ifdef CONFIG_DEBUG_SET_MODULE_RONX
>  extern void set_all_modules_text_rw(void);
>  extern void set_all_modules_text_ro(void);
> +extern void
> +set_page_attributes(void *start, void *end,
> +		    int (*set)(unsigned long start, int num_pages));
>  #else
>  static inline void set_all_modules_text_rw(void) { }
>  static inline void set_all_modules_text_ro(void) { }
> +static inline void
> +set_page_attributes(void *start, void *end,
> +		    int (*set)(unsigned long start, int num_pages)) { }
>  #endif
>  
>  #ifdef CONFIG_GENERIC_BUG
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index db545cb..17b7278 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,9 @@
>  #include <linux/list.h>
>  #include <linux/kallsyms.h>
>  #include <linux/livepatch.h>
> +#include <linux/elf.h>
> +#include <asm/cacheflush.h>
> +#include <linux/moduleloader.h>
>  
>  /**
>   * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> -					struct klp_object *obj)
> +					struct klp_object *obj,
> +					struct klp_patch *patch)
>  {
> -	int ret;
> -	struct klp_reloc *reloc;
> +	int relindex, num_relas;
> +	int i, ret = 0;
> +	unsigned long addr;
> +	char *symname;
> +	struct klp_reloc_sec *reloc_sec;
> +	Elf_Rela *rela;
> +	Elf_Sym *sym, *symtab;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> -
> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		if (!klp_is_module(obj)) {
> -
> -#if defined(CONFIG_RANDOMIZE_BASE)
> -			/* If KASLR has been enabled, adjust old value accordingly */
> -			if (kaslr_enabled())
> -				reloc->val += kaslr_offset();
> -#endif
> -			ret = klp_verify_vmlinux_symbol(reloc->name,
> -							reloc->val);
> -			if (ret)
> -				return ret;
> -		} else {
> -			/* module, reloc->val needs to be discovered */
> -			if (reloc->external)
> -				ret = klp_find_external_symbol(pmod,
> -							       reloc->name,
> -							       &reloc->val);
> -			else
> -				ret = klp_find_object_symbol(obj->mod->name,
> -							     reloc->name,
> -							     &reloc->val);
> -			if (ret)
> -				return ret;
> -		}
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     reloc->val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, reloc->val, ret);
> -			return ret;
> +	symtab = (void *)pmod->core_symtab;
> +
> +	/* For each __klp_rela section for this object */
> +	klp_for_each_reloc_sec(obj, reloc_sec) {
> +		relindex = reloc_sec->index;
> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> +
> +		/* For each rela in this __klp_rela section */
> +		for (i = 0; i < num_relas; i++, rela++) {
> +			sym = symtab + ELF_R_SYM(rela->r_info);
> +			symname = pmod->core_strtab + sym->st_name;
> +
> +			if (sym->st_shndx == SHN_LIVEPATCH) {
> +				if (sym->st_info == 'K')
> +					ret = klp_find_external_symbol(pmod, symname, &addr);
> +				else
> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> +				if (ret)
> +					return ret;
> +				sym->st_value = addr;

So I think you're also working on removing the 'external' stuff.  Any
idea how this code will handle that?  Specifically I'm wondering how the
objname and sympos of the rela's symbol will be specified.  Maybe it can
be encoded somehow in one of the symbol fields (st_value)?

> +			}
>  		}
> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> +					 pmod->index.sym, relindex, pmod);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void notrace klp_ftrace_handler(unsigned long ip,
> @@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  				  struct klp_object *obj)
>  {
>  	struct klp_func *func;
> +	struct module *pmod;
>  	int ret;
>  
> -	if (obj->relocs) {
> -		ret = klp_write_object_relocations(patch->mod, obj);
> -		if (ret)
> -			return ret;
> -	}
> +	pmod = patch->mod;
> +
> +	set_page_attributes(pmod->module_core,
> +			    pmod->module_core + pmod->core_text_size,
> +			    set_memory_rw);
> +
> +	ret = klp_write_object_relocations(pmod, obj, patch);
> +	if (ret)
> +		return ret;
> +
> +	set_page_attributes(pmod->module_core,
> +			    pmod->module_core + pmod->core_text_size,
> +			    set_memory_ro);
>  
>  	klp_for_each_func(obj, func) {
>  		ret = klp_find_verify_func_addr(obj, func);
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Josh

Re: [RFC PATCH v2 5/6] samples: livepatch: init reloc section array and mark as klp module

[Josh Poimboeuf]

On Mon, Nov 30, 2015 at 11:21:18PM -0500, Jessica Yu wrote:
> Create the array of klp relocation sections in the sample
> klp_object (even if the array is empty in this case).
> 
> In addition, mark the module as a livepatch module so that
> the module loader can appropriately identify and initialize it.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  samples/livepatch/livepatch-sample.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index fb8c861..298dc21 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -53,10 +53,15 @@ static struct klp_func funcs[] = {
>  	}, { }
>  };
>  
> +static struct klp_reloc_sec reloc_secs[] = {
> +	{ }
> +};
> +

I think it's not possible to specify the relocations when building the
patch module in this way, right?  If so, I think this is confusing, and
it would be better to just leave out any mention of reloc_secs in the
sample module.

>  static struct klp_object objs[] = {
>  	{
>  		/* name being NULL means vmlinux */
>  		.funcs = funcs,
> +		.reloc_secs = reloc_secs,
>  	}, { }
>  };
>  
> @@ -89,3 +94,4 @@ static void livepatch_exit(void)
>  module_init(livepatch_init);
>  module_exit(livepatch_exit);
>  MODULE_LICENSE("GPL");
> +MODULE_INFO(livepatch, "Y");
> -- 
> 2.4.3
> 

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>On Mon, Nov 30, 2015 at 11:21:17PM -0500, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the need
>> for architecture specific relocation code in livepatch. Namely, we reuse
>> apply_relocate_add() in the module loader to write relocations instead of
>> duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>> relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>> are resolved and then apply_relocate_add() is called to apply those
>> relocations.
>>
>> In addition, remove x86 livepatch relocation code. It is no longer needed
>> since symbol resolution and relocation work have been offloaded to module
>> loader.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  arch/x86/include/asm/livepatch.h |  2 -
>>  arch/x86/kernel/Makefile         |  1 -
>>  arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>>  include/linux/livepatch.h        | 30 ++++++-------
>>  include/linux/module.h           |  6 +++
>>  kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>>  6 files changed, 70 insertions(+), 154 deletions(-)
>>  delete mode 100644 arch/x86/kernel/livepatch.c
>>
>> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>> index 19c099a..7312e25 100644
>> --- a/arch/x86/include/asm/livepatch.h
>> +++ b/arch/x86/include/asm/livepatch.h
>> @@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>>  #endif
>>  	return 0;
>>  }
>> -int klp_write_module_reloc(struct module *mod, unsigned long type,
>> -			   unsigned long loc, unsigned long value);
>>
>>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>  {
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index b1b78ff..c5e9a5c 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>>  obj-y				+= apic/
>>  obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>> -obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>>  obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>>  obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>> deleted file mode 100644
>> index d1d35cc..0000000
>> --- a/arch/x86/kernel/livepatch.c
>> +++ /dev/null
>> @@ -1,91 +0,0 @@
>> -/*
>> - * livepatch.c - x86-specific Kernel Live Patching Core
>> - *
>> - * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
>> - * Copyright (C) 2014 SUSE
>> - *
>> - * 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/>.
>> - */
>> -
>> -#include <linux/module.h>
>> -#include <linux/uaccess.h>
>> -#include <asm/cacheflush.h>
>> -#include <asm/page_types.h>
>> -#include <asm/elf.h>
>> -#include <asm/livepatch.h>
>> -
>> -/**
>> - * klp_write_module_reloc() - write a relocation in a module
>> - * @mod:	module in which the section to be modified is found
>> - * @type:	ELF relocation type (see asm/elf.h)
>> - * @loc:	address that the relocation should be written to
>> - * @value:	relocation value (sym address + addend)
>> - *
>> - * This function writes a relocation to the specified location for
>> - * a particular module.
>> - */
>> -int klp_write_module_reloc(struct module *mod, unsigned long type,
>> -			   unsigned long loc, unsigned long value)
>> -{
>> -	int ret, numpages, size = 4;
>> -	bool readonly;
>> -	unsigned long val;
>> -	unsigned long core = (unsigned long)mod->module_core;
>> -	unsigned long core_size = mod->core_size;
>> -
>> -	switch (type) {
>> -	case R_X86_64_NONE:
>> -		return 0;
>> -	case R_X86_64_64:
>> -		val = value;
>> -		size = 8;
>> -		break;
>> -	case R_X86_64_32:
>> -		val = (u32)value;
>> -		break;
>> -	case R_X86_64_32S:
>> -		val = (s32)value;
>> -		break;
>> -	case R_X86_64_PC32:
>> -		val = (u32)(value - loc);
>> -		break;
>> -	default:
>> -		/* unsupported relocation type */
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (loc < core || loc >= core + core_size)
>> -		/* loc does not point to any symbol inside the module */
>> -		return -EINVAL;
>> -
>> -	readonly = false;
>> -
>> -#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>> -	if (loc < core + mod->core_ro_size)
>> -		readonly = true;
>> -#endif
>> -
>> -	/* determine if the relocation spans a page boundary */
>> -	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>> -
>> -	if (readonly)
>> -		set_memory_rw(loc & PAGE_MASK, numpages);
>> -
>> -	ret = probe_kernel_write((void *)loc, &val, size);
>> -
>> -	if (readonly)
>> -		set_memory_ro(loc & PAGE_MASK, numpages);
>> -
>> -	return ret;
>> -}
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..54f62a6 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -64,28 +64,21 @@ struct klp_func {
>>  };
>>
>>  /**
>> - * struct klp_reloc - relocation structure for live patching
>> - * @loc:	address where the relocation will be written
>> - * @val:	address of the referenced symbol (optional,
>> - *		vmlinux	patches only)
>> - * @type:	ELF relocation type
>> - * @name:	name of the referenced symbol (for lookup/verification)
>> - * @addend:	offset from the referenced symbol
>> - * @external:	symbol is either exported or within the live patch module itself
>> + * struct klp_reloc_sec - relocation section struct for live patching
>> + * @index:	Elf section index of the relocation section
>> + * @name:	name of the relocation section
>> + * @objname:	name of the object associated with the klp reloc section
>>   */
>> -struct klp_reloc {
>> -	unsigned long loc;
>> -	unsigned long val;
>> -	unsigned long type;
>> -	const char *name;
>> -	int addend;
>> -	int external;
>> +struct klp_reloc_sec {
>> +	unsigned int index;
>> +	char *name;
>> +	char *objname;
>>  };
>>
>>  /**
>>   * struct klp_object - kernel object structure for live patching
>>   * @name:	module name (or NULL for vmlinux)
>> - * @relocs:	relocation entries to be applied at load time
>> + * @reloc_secs:	relocation sections to be applied at load time
>>   * @funcs:	function entries for functions to be patched in the object
>>   * @kobj:	kobject for sysfs resources
>>   * @mod:	kernel module associated with the patched object
>> @@ -95,7 +88,7 @@ struct klp_reloc {
>>  struct klp_object {
>>  	/* external */
>>  	const char *name;
>> -	struct klp_reloc *relocs;
>> +	struct klp_reloc_sec *reloc_secs;
>
>There was a lot of discussion for v1, so I'm not sure, but I thought we
>ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>think the symbol table can be walked directly looking for klp rela
>sections?
>
>The benefits of doing so would be to make the interface simpler -- no
>need to "cache" the section metadata when it's already easily and
>quickly available in the module struct elf section headers.

Ah, I might have interpreted the conclusions of that last discussion
incorrectly.

In that case, I think I can just get rid of my klp_for_each_reloc_sec
macro as well as the lreloc scaffolding code from kpatch. The only
potentially "ugly" part of this change is that I'll have to move the
string parsing stuff here to extract the objname of the __klp_rela
section (which may not actually look that bad, we'll see how that
turns out).

>>  	struct klp_func *funcs;
>>
>>  	/* internal */
>> @@ -129,6 +122,9 @@ struct klp_patch {
>>  #define klp_for_each_func(obj, func) \
>>  	for (func = obj->funcs; func->old_name; func++)
>>
>> +#define klp_for_each_reloc_sec(obj, reloc_sec) \
>> +	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>> +
>>  int klp_register_patch(struct klp_patch *);
>>  int klp_unregister_patch(struct klp_patch *);
>>  int klp_enable_patch(struct klp_patch *);
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 9b46256..ea61147 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
>>  #ifdef CONFIG_DEBUG_SET_MODULE_RONX
>>  extern void set_all_modules_text_rw(void);
>>  extern void set_all_modules_text_ro(void);
>> +extern void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages));
>>  #else
>>  static inline void set_all_modules_text_rw(void) { }
>>  static inline void set_all_modules_text_ro(void) { }
>> +static inline void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages)) { }
>>  #endif
>>
>>  #ifdef CONFIG_GENERIC_BUG
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index db545cb..17b7278 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -28,6 +28,9 @@
>>  #include <linux/list.h>
>>  #include <linux/kallsyms.h>
>>  #include <linux/livepatch.h>
>> +#include <linux/elf.h>
>> +#include <asm/cacheflush.h>
>> +#include <linux/moduleloader.h>
>>
>>  /**
>>   * struct klp_ops - structure for tracking registered ftrace ops structs
>> @@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>>  }
>>
>>  static int klp_write_object_relocations(struct module *pmod,
>> -					struct klp_object *obj)
>> +					struct klp_object *obj,
>> +					struct klp_patch *patch)
>>  {
>> -	int ret;
>> -	struct klp_reloc *reloc;
>> +	int relindex, num_relas;
>> +	int i, ret = 0;
>> +	unsigned long addr;
>> +	char *symname;
>> +	struct klp_reloc_sec *reloc_sec;
>> +	Elf_Rela *rela;
>> +	Elf_Sym *sym, *symtab;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> -
>> -	for (reloc = obj->relocs; reloc->name; reloc++) {
>> -		if (!klp_is_module(obj)) {
>> -
>> -#if defined(CONFIG_RANDOMIZE_BASE)
>> -			/* If KASLR has been enabled, adjust old value accordingly */
>> -			if (kaslr_enabled())
>> -				reloc->val += kaslr_offset();
>> -#endif
>> -			ret = klp_verify_vmlinux_symbol(reloc->name,
>> -							reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		} else {
>> -			/* module, reloc->val needs to be discovered */
>> -			if (reloc->external)
>> -				ret = klp_find_external_symbol(pmod,
>> -							       reloc->name,
>> -							       &reloc->val);
>> -			else
>> -				ret = klp_find_object_symbol(obj->mod->name,
>> -							     reloc->name,
>> -							     &reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		}
>> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>> -					     reloc->val + reloc->addend);
>> -		if (ret) {
>> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>> -			       reloc->name, reloc->val, ret);
>> -			return ret;
>> +	symtab = (void *)pmod->core_symtab;
>> +
>> +	/* For each __klp_rela section for this object */
>> +	klp_for_each_reloc_sec(obj, reloc_sec) {
>> +		relindex = reloc_sec->index;
>> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> +
>> +		/* For each rela in this __klp_rela section */
>> +		for (i = 0; i < num_relas; i++, rela++) {
>> +			sym = symtab + ELF_R_SYM(rela->r_info);
>> +			symname = pmod->core_strtab + sym->st_name;
>> +
>> +			if (sym->st_shndx == SHN_LIVEPATCH) {
>> +				if (sym->st_info == 'K')
>> +					ret = klp_find_external_symbol(pmod, symname, &addr);
>> +				else
>> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> +				if (ret)
>> +					return ret;
>> +				sym->st_value = addr;
>
>So I think you're also working on removing the 'external' stuff.  Any
>idea how this code will handle that?  Specifically I'm wondering how the
>objname and sympos of the rela's symbol will be specified.  Maybe it can
>be encoded somehow in one of the symbol fields (st_value)?

Thanks for bringing this up. I think we can just encode the symbol's
position in kallsyms in the symbol's st_other field. It isn't used
anywhere and has size char, which is plenty of bits to represent the
small ints.

For objname, the simplest solution might be to append ".klp.objname"
to the symbol name, and extract out this suffix when resolving the
symbol. Another way might be to have st_value contain the index into
the strtab (or .kpatch.strings) that contains the objname. Then we'd
access the objname just like how we access the symbol's name (strtab +
sym->st_name). After we find the objname we can then overwrite
st_value with the real address. I think this second method is cleaner.
Thoughts?

>> +			}
>>  		}
>> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>> +					 pmod->index.sym, relindex, pmod);
>>  	}
>>
>> -	return 0;
>> +	return ret;
>>  }
>>
>>  static void notrace klp_ftrace_handler(unsigned long ip,
>> @@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>>  				  struct klp_object *obj)
>>  {
>>  	struct klp_func *func;
>> +	struct module *pmod;
>>  	int ret;
>>
>> -	if (obj->relocs) {
>> -		ret = klp_write_object_relocations(patch->mod, obj);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	pmod = patch->mod;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_rw);
>> +
>> +	ret = klp_write_object_relocations(pmod, obj, patch);
>> +	if (ret)
>> +		return ret;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_ro);
>>
>>  	klp_for_each_func(obj, func) {
>>  		ret = klp_find_verify_func_addr(obj, func);
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe live-patching" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>-- 
>Josh

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>On Mon, Nov 30, 2015 at 11:21:17PM -0500, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the need
>> for architecture specific relocation code in livepatch. Namely, we reuse
>> apply_relocate_add() in the module loader to write relocations instead of
>> duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>> relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>> are resolved and then apply_relocate_add() is called to apply those
>> relocations.
>>
>> In addition, remove x86 livepatch relocation code. It is no longer needed
>> since symbol resolution and relocation work have been offloaded to module
>> loader.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/x86/include/asm/livepatch.h |  2 -
>>  arch/x86/kernel/Makefile         |  1 -
>>  arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>>  include/linux/livepatch.h        | 30 ++++++-------
>>  include/linux/module.h           |  6 +++
>>  kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>>  6 files changed, 70 insertions(+), 154 deletions(-)
>>  delete mode 100644 arch/x86/kernel/livepatch.c
>>
>> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>> index 19c099a..7312e25 100644
>> --- a/arch/x86/include/asm/livepatch.h
>> +++ b/arch/x86/include/asm/livepatch.h
>> @@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>>  #endif
>>  	return 0;
>>  }
>> -int klp_write_module_reloc(struct module *mod, unsigned long type,
>> -			   unsigned long loc, unsigned long value);
>>
>>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>  {
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index b1b78ff..c5e9a5c 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>>  obj-y				+= apic/
>>  obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>> -obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>>  obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>>  obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>> deleted file mode 100644
>> index d1d35cc..0000000
>> --- a/arch/x86/kernel/livepatch.c
>> +++ /dev/null
>> @@ -1,91 +0,0 @@
>> -/*
>> - * livepatch.c - x86-specific Kernel Live Patching Core
>> - *
>> - * Copyright (C) 2014 Seth Jennings <sjenning-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> - * Copyright (C) 2014 SUSE
>> - *
>> - * 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/>.
>> - */
>> -
>> -#include <linux/module.h>
>> -#include <linux/uaccess.h>
>> -#include <asm/cacheflush.h>
>> -#include <asm/page_types.h>
>> -#include <asm/elf.h>
>> -#include <asm/livepatch.h>
>> -
>> -/**
>> - * klp_write_module_reloc() - write a relocation in a module
>> - * @mod:	module in which the section to be modified is found
>> - * @type:	ELF relocation type (see asm/elf.h)
>> - * @loc:	address that the relocation should be written to
>> - * @value:	relocation value (sym address + addend)
>> - *
>> - * This function writes a relocation to the specified location for
>> - * a particular module.
>> - */
>> -int klp_write_module_reloc(struct module *mod, unsigned long type,
>> -			   unsigned long loc, unsigned long value)
>> -{
>> -	int ret, numpages, size = 4;
>> -	bool readonly;
>> -	unsigned long val;
>> -	unsigned long core = (unsigned long)mod->module_core;
>> -	unsigned long core_size = mod->core_size;
>> -
>> -	switch (type) {
>> -	case R_X86_64_NONE:
>> -		return 0;
>> -	case R_X86_64_64:
>> -		val = value;
>> -		size = 8;
>> -		break;
>> -	case R_X86_64_32:
>> -		val = (u32)value;
>> -		break;
>> -	case R_X86_64_32S:
>> -		val = (s32)value;
>> -		break;
>> -	case R_X86_64_PC32:
>> -		val = (u32)(value - loc);
>> -		break;
>> -	default:
>> -		/* unsupported relocation type */
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (loc < core || loc >= core + core_size)
>> -		/* loc does not point to any symbol inside the module */
>> -		return -EINVAL;
>> -
>> -	readonly = false;
>> -
>> -#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>> -	if (loc < core + mod->core_ro_size)
>> -		readonly = true;
>> -#endif
>> -
>> -	/* determine if the relocation spans a page boundary */
>> -	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>> -
>> -	if (readonly)
>> -		set_memory_rw(loc & PAGE_MASK, numpages);
>> -
>> -	ret = probe_kernel_write((void *)loc, &val, size);
>> -
>> -	if (readonly)
>> -		set_memory_ro(loc & PAGE_MASK, numpages);
>> -
>> -	return ret;
>> -}
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 31db7a0..54f62a6 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -64,28 +64,21 @@ struct klp_func {
>>  };
>>
>>  /**
>> - * struct klp_reloc - relocation structure for live patching
>> - * @loc:	address where the relocation will be written
>> - * @val:	address of the referenced symbol (optional,
>> - *		vmlinux	patches only)
>> - * @type:	ELF relocation type
>> - * @name:	name of the referenced symbol (for lookup/verification)
>> - * @addend:	offset from the referenced symbol
>> - * @external:	symbol is either exported or within the live patch module itself
>> + * struct klp_reloc_sec - relocation section struct for live patching
>> + * @index:	Elf section index of the relocation section
>> + * @name:	name of the relocation section
>> + * @objname:	name of the object associated with the klp reloc section
>>   */
>> -struct klp_reloc {
>> -	unsigned long loc;
>> -	unsigned long val;
>> -	unsigned long type;
>> -	const char *name;
>> -	int addend;
>> -	int external;
>> +struct klp_reloc_sec {
>> +	unsigned int index;
>> +	char *name;
>> +	char *objname;
>>  };
>>
>>  /**
>>   * struct klp_object - kernel object structure for live patching
>>   * @name:	module name (or NULL for vmlinux)
>> - * @relocs:	relocation entries to be applied at load time
>> + * @reloc_secs:	relocation sections to be applied at load time
>>   * @funcs:	function entries for functions to be patched in the object
>>   * @kobj:	kobject for sysfs resources
>>   * @mod:	kernel module associated with the patched object
>> @@ -95,7 +88,7 @@ struct klp_reloc {
>>  struct klp_object {
>>  	/* external */
>>  	const char *name;
>> -	struct klp_reloc *relocs;
>> +	struct klp_reloc_sec *reloc_secs;
>
>There was a lot of discussion for v1, so I'm not sure, but I thought we
>ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>think the symbol table can be walked directly looking for klp rela
>sections?
>
>The benefits of doing so would be to make the interface simpler -- no
>need to "cache" the section metadata when it's already easily and
>quickly available in the module struct elf section headers.

Ah, I might have interpreted the conclusions of that last discussion
incorrectly.

In that case, I think I can just get rid of my klp_for_each_reloc_sec
macro as well as the lreloc scaffolding code from kpatch. The only
potentially "ugly" part of this change is that I'll have to move the
string parsing stuff here to extract the objname of the __klp_rela
section (which may not actually look that bad, we'll see how that
turns out).

>>  	struct klp_func *funcs;
>>
>>  	/* internal */
>> @@ -129,6 +122,9 @@ struct klp_patch {
>>  #define klp_for_each_func(obj, func) \
>>  	for (func = obj->funcs; func->old_name; func++)
>>
>> +#define klp_for_each_reloc_sec(obj, reloc_sec) \
>> +	for (reloc_sec = obj->reloc_secs; reloc_sec->name; reloc_sec++)
>> +
>>  int klp_register_patch(struct klp_patch *);
>>  int klp_unregister_patch(struct klp_patch *);
>>  int klp_enable_patch(struct klp_patch *);
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 9b46256..ea61147 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -777,9 +777,15 @@ extern int module_sysfs_initialized;
>>  #ifdef CONFIG_DEBUG_SET_MODULE_RONX
>>  extern void set_all_modules_text_rw(void);
>>  extern void set_all_modules_text_ro(void);
>> +extern void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages));
>>  #else
>>  static inline void set_all_modules_text_rw(void) { }
>>  static inline void set_all_modules_text_ro(void) { }
>> +static inline void
>> +set_page_attributes(void *start, void *end,
>> +		    int (*set)(unsigned long start, int num_pages)) { }
>>  #endif
>>
>>  #ifdef CONFIG_GENERIC_BUG
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index db545cb..17b7278 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -28,6 +28,9 @@
>>  #include <linux/list.h>
>>  #include <linux/kallsyms.h>
>>  #include <linux/livepatch.h>
>> +#include <linux/elf.h>
>> +#include <asm/cacheflush.h>
>> +#include <linux/moduleloader.h>
>>
>>  /**
>>   * struct klp_ops - structure for tracking registered ftrace ops structs
>> @@ -281,52 +284,48 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>>  }
>>
>>  static int klp_write_object_relocations(struct module *pmod,
>> -					struct klp_object *obj)
>> +					struct klp_object *obj,
>> +					struct klp_patch *patch)
>>  {
>> -	int ret;
>> -	struct klp_reloc *reloc;
>> +	int relindex, num_relas;
>> +	int i, ret = 0;
>> +	unsigned long addr;
>> +	char *symname;
>> +	struct klp_reloc_sec *reloc_sec;
>> +	Elf_Rela *rela;
>> +	Elf_Sym *sym, *symtab;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> -
>> -	for (reloc = obj->relocs; reloc->name; reloc++) {
>> -		if (!klp_is_module(obj)) {
>> -
>> -#if defined(CONFIG_RANDOMIZE_BASE)
>> -			/* If KASLR has been enabled, adjust old value accordingly */
>> -			if (kaslr_enabled())
>> -				reloc->val += kaslr_offset();
>> -#endif
>> -			ret = klp_verify_vmlinux_symbol(reloc->name,
>> -							reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		} else {
>> -			/* module, reloc->val needs to be discovered */
>> -			if (reloc->external)
>> -				ret = klp_find_external_symbol(pmod,
>> -							       reloc->name,
>> -							       &reloc->val);
>> -			else
>> -				ret = klp_find_object_symbol(obj->mod->name,
>> -							     reloc->name,
>> -							     &reloc->val);
>> -			if (ret)
>> -				return ret;
>> -		}
>> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>> -					     reloc->val + reloc->addend);
>> -		if (ret) {
>> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>> -			       reloc->name, reloc->val, ret);
>> -			return ret;
>> +	symtab = (void *)pmod->core_symtab;
>> +
>> +	/* For each __klp_rela section for this object */
>> +	klp_for_each_reloc_sec(obj, reloc_sec) {
>> +		relindex = reloc_sec->index;
>> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> +
>> +		/* For each rela in this __klp_rela section */
>> +		for (i = 0; i < num_relas; i++, rela++) {
>> +			sym = symtab + ELF_R_SYM(rela->r_info);
>> +			symname = pmod->core_strtab + sym->st_name;
>> +
>> +			if (sym->st_shndx == SHN_LIVEPATCH) {
>> +				if (sym->st_info == 'K')
>> +					ret = klp_find_external_symbol(pmod, symname, &addr);
>> +				else
>> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> +				if (ret)
>> +					return ret;
>> +				sym->st_value = addr;
>
>So I think you're also working on removing the 'external' stuff.  Any
>idea how this code will handle that?  Specifically I'm wondering how the
>objname and sympos of the rela's symbol will be specified.  Maybe it can
>be encoded somehow in one of the symbol fields (st_value)?

Thanks for bringing this up. I think we can just encode the symbol's
position in kallsyms in the symbol's st_other field. It isn't used
anywhere and has size char, which is plenty of bits to represent the
small ints.

For objname, the simplest solution might be to append ".klp.objname"
to the symbol name, and extract out this suffix when resolving the
symbol. Another way might be to have st_value contain the index into
the strtab (or .kpatch.strings) that contains the objname. Then we'd
access the objname just like how we access the symbol's name (strtab +
sym->st_name). After we find the objname we can then overwrite
st_value with the real address. I think this second method is cleaner.
Thoughts?

>> +			}
>>  		}
>> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
>> +					 pmod->index.sym, relindex, pmod);
>>  	}
>>
>> -	return 0;
>> +	return ret;
>>  }
>>
>>  static void notrace klp_ftrace_handler(unsigned long ip,
>> @@ -747,13 +746,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>>  				  struct klp_object *obj)
>>  {
>>  	struct klp_func *func;
>> +	struct module *pmod;
>>  	int ret;
>>
>> -	if (obj->relocs) {
>> -		ret = klp_write_object_relocations(patch->mod, obj);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	pmod = patch->mod;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_rw);
>> +
>> +	ret = klp_write_object_relocations(pmod, obj, patch);
>> +	if (ret)
>> +		return ret;
>> +
>> +	set_page_attributes(pmod->module_core,
>> +			    pmod->module_core + pmod->core_text_size,
>> +			    set_memory_ro);
>>
>>  	klp_for_each_func(obj, func) {
>>  		ret = klp_find_verify_func_addr(obj, func);
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe live-patching" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>-- 
>Josh

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:32 -0600]:
>On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader.
>>
>> Livepatch uses special relocation sections in order to be able to patch
>> modules that are not yet loaded, as well as apply patches to the kernel
>> when the addresses of symbols cannot be determined at compile time (for
>> example, when kaslr is enabled). Livepatch modules must preserve Elf
>> information such as section indices in order to apply the remaining
>> relocation sections at the appropriate time (i.e. when the target module
>> loads).
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  include/linux/module.h |  9 +++++
>>  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 105 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 3a19c79..9b46256 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -425,6 +425,14 @@ struct module {
>>
>>  	/* Notes attributes */
>>  	struct module_notes_attrs *notes_attrs;
>> +
>> +	/* Elf information (optionally saved) */
>> +	Elf_Ehdr *hdr;
>
>I would rename "hdr" to "elf_hdr" to make its purpose clearer.
>
>> +	Elf_Shdr *sechdrs;
>> +	char *secstrings;
>
>Probably a good idea to add underscores to the names ("sec_hdrs" and
>"sec_strings") to be consistent with most of the other fields in the
>struct.
>
>> +	struct {
>> +		unsigned int sym, str, mod, vers, info, pcpu;
>> +	} index;
>
>I might be contradicting myself from what I said before.  But I'm
>thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
>Then below, there could be two versions of copy_module_elf(), the real
>one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
>story for free_module_elf().

I think in the v1 discussion we were leaning more towards making this
generic. We could potentially just have the Elf module fields
available in the generic case, independent of whether CONFIG_LIVEPATCH
is set, whereas the mod->klp field should probably be only available
when LIVEPATCH is set. I think this makes sense since the Elf fields
aren't dependent on livepatch (although livepatch would be the only
user of these fields at the moment). I don't know if there would be
any users in the future that would be interested in using this Elf
information. Thoughts on this?

>>  #endif
>>
>>  	/* The command line arguments (may be mangled).  People like
>> @@ -461,6 +469,7 @@ struct module {
>>  #endif
>>
>>  #ifdef CONFIG_LIVEPATCH
>> +	bool klp; /* Is this a livepatch module? */
>>  	bool klp_alive;
>>  #endif
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 8f051a1..433c2d6 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
>>  static void unset_module_init_ro_nx(struct module *mod) { }
>>  #endif
>>
>> +static void free_module_elf(struct module *mod)
>> +{
>> +	kfree(mod->hdr);
>> +	kfree(mod->sechdrs);
>> +	kfree(mod->secstrings);
>> +}
>> +
>>  void __weak module_memfree(void *module_region)
>>  {
>>  	vfree(module_region);
>> @@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
>>  	/* Free any allocated parameters. */
>>  	destroy_params(mod->kp, mod->num_kp);
>>
>> +	/* Free Elf information if it was saved */
>> +	free_module_elf(mod);
>> +
>>  	/* Now we can delete it from the lists */
>>  	mutex_lock(&module_mutex);
>>  	/* Unlink carefully: kallsyms could be walking list. */
>> @@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>>  			       (long)sym[i].st_value);
>>  			break;
>>
>> +		case SHN_LIVEPATCH:
>> +			/* klp symbols are resolved by livepatch */
>> +			break;
>> +
>>  		case SHN_UNDEF:
>>  			ksym = resolve_symbol_wait(mod, info, name);
>>  			/* Ok if resolved.  */
>> @@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>>  			continue;
>>
>> +		/* klp relocation sections are applied by livepatch */
>> +		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
>> +			continue;
>> +
>>  		if (info->sechdrs[i].sh_type == SHT_REL)
>>  			err = apply_relocate(info->sechdrs, info->strtab,
>>  					     info->index.sym, i, mod);
>> @@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
>>  {
>>  	const Elf_Shdr *sechdrs = info->sechdrs;
>>
>> +	if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
>> +		return 'K';
>> +	if (sym->st_shndx == SHN_LIVEPATCH)
>> +		return 'k';
>> +
>>  	if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
>>  		if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
>>  			return 'v';
>> @@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>>
>>  	/* Compute total space required for the core symbols' strtab. */
>>  	for (ndst = i = 0; i < nsrc; i++) {
>> -		if (i == 0 ||
>> +		if (i == 0 || mod->klp ||
>>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>>  			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
>>  			ndst++;
>
>Instead of accessing mod->klp directly, how about an
>'is_livepatch_module(mod)' function.  There could be two versions, with
>the !LIVEPATCH version always returning false and the LIVEPATCH version
>checking mod->klp.  Then mod->klp itself can stay inside the LIVEPATCH
>ifdef in the module struct.

OK, that sounds good. Makes more sense for the klp field to exist only
in the LIVEPATCH case anyway.

>> @@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>>  	mod->core_strtab = s = mod->module_core + info->stroffs;
>>  	src = mod->symtab;
>>  	for (ndst = i = 0; i < mod->num_symtab; i++) {
>> -		if (i == 0 ||
>> +		if (i == 0 || mod->klp ||
>>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>>  			dst[ndst] = src[i];
>>  			dst[ndst++].st_name = s - mod->core_strtab;
>> @@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
>>  	return 0;
>>  }
>>
>> +/*
>> + * copy_module_elf - preserve Elf information about a module
>> + */
>> +static int copy_module_elf(struct module *mod, struct load_info *info)
>> +{
>> +	unsigned int size;
>> +	int ret = 0;
>
>No need to initialize ret to zero here since it's never used
>uninitalized.
>
>> +	Elf_Shdr *symsect;
>> +
>> +	/* Elf header */
>> +	size = sizeof(Elf_Ehdr);
>> +	mod->hdr = kzalloc(size, GFP_KERNEL);
>
>No need to zero the memory here with kzalloc() since it will all be
>memcpy()'d anyway.  kmalloc() can be used instead (and the same for the
>other kzalloc()s below).
>
>> +	if (mod->hdr == NULL) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +	memcpy(mod->hdr, info->hdr, size);
>> +
>> +	/* Elf section header table */
>> +	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
>> +	mod->sechdrs = kzalloc(size, GFP_KERNEL);
>> +	if (mod->sechdrs == NULL) {
>> +		ret = -ENOMEM;
>> +		goto free_hdr;
>> +	}
>> +	memcpy(mod->sechdrs, info->sechdrs, size);
>> +
>> +	/* Elf section name string table */
>> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
>> +	mod->secstrings = kzalloc(size, GFP_KERNEL);
>> +	if (mod->secstrings == NULL) {
>> +		ret = -ENOMEM;
>> +		goto free_sechdrs;
>> +	}
>> +	memcpy(mod->secstrings, info->secstrings, size);
>> +
>> +	/* Elf section indices */
>> +	memcpy(&mod->index, &info->index, sizeof(info->index));
>> +
>> +	/*
>> +	 * Update symtab's sh_addr to point to a valid
>> +	 * symbol table, as the temporary symtab in module
>> +	 * init memory will be freed
>> +	 */
>> +	symsect = mod->sechdrs + mod->index.sym;
>> +	symsect->sh_addr = (unsigned long)mod->core_symtab;
>> +
>> +	return ret;
>> +
>> +free_sechdrs:
>> +	kfree(mod->sechdrs);
>> +free_hdr:
>> +	kfree(mod->hdr);
>> +out:
>> +	return ret;
>> +}
>> +
>> +
>>  #define COPY_CHUNK_SIZE (16*PAGE_SIZE)
>>
>>  static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned long len)
>> @@ -2866,6 +2947,9 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>>  			"is unknown, you have been warned.\n", mod->name);
>>  	}
>>
>> +	if (get_modinfo(info, "livepatch"))
>> +		mod->klp = true;
>> +
>
>Similar to the is_livepatch_module() function I suggested, this can be
>put in a function so that mod->klp can be abstracted away for the
>!LIVEPATCH case.  Maybe there should be a check_livepatch_modinfo()
>function:
>
>1. the !LIVEPATCH version of the function could return an error if
>modinfo has "livepatch"
>
>2. the LIVEPATCH version could simply set mod->klp = true.
>
>>  	/* Set up license info based on the info section */
>>  	set_license(mod, get_modinfo(info, "license"));
>>
>> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>  	if (err < 0)
>>  		goto bug_cleanup;
>>
>> +	/*
>> +	 * Save sechdrs, indices, and other data from info
>> +	 * in order to patch to-be-loaded modules.
>> +	 * Do not call free_copy() for livepatch modules.
>
>I think the last line of this comment isn't right, since free_copy() is
>called below regardless.

Yes you're right, forgot to update that comment.

>> +	 */
>> +	if (mod->klp)
>> +		err = copy_module_elf(mod, info);
>> +	if (err < 0)
>> +		goto bug_cleanup;
>
>Not strictly necessary, but I think it would be a little cleaner to only
>check the err if copy_module_elf() was called.
>
>> +
>>  	/* Get rid of temporary copy. */
>>  	free_copy(info);
>
>-- 
>Josh

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/12/15 12:32 -0600]:
>On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader.
>>
>> Livepatch uses special relocation sections in order to be able to patch
>> modules that are not yet loaded, as well as apply patches to the kernel
>> when the addresses of symbols cannot be determined at compile time (for
>> example, when kaslr is enabled). Livepatch modules must preserve Elf
>> information such as section indices in order to apply the remaining
>> relocation sections at the appropriate time (i.e. when the target module
>> loads).
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  include/linux/module.h |  9 +++++
>>  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 105 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 3a19c79..9b46256 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -425,6 +425,14 @@ struct module {
>>
>>  	/* Notes attributes */
>>  	struct module_notes_attrs *notes_attrs;
>> +
>> +	/* Elf information (optionally saved) */
>> +	Elf_Ehdr *hdr;
>
>I would rename "hdr" to "elf_hdr" to make its purpose clearer.
>
>> +	Elf_Shdr *sechdrs;
>> +	char *secstrings;
>
>Probably a good idea to add underscores to the names ("sec_hdrs" and
>"sec_strings") to be consistent with most of the other fields in the
>struct.
>
>> +	struct {
>> +		unsigned int sym, str, mod, vers, info, pcpu;
>> +	} index;
>
>I might be contradicting myself from what I said before.  But I'm
>thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
>Then below, there could be two versions of copy_module_elf(), the real
>one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
>story for free_module_elf().

I think in the v1 discussion we were leaning more towards making this
generic. We could potentially just have the Elf module fields
available in the generic case, independent of whether CONFIG_LIVEPATCH
is set, whereas the mod->klp field should probably be only available
when LIVEPATCH is set. I think this makes sense since the Elf fields
aren't dependent on livepatch (although livepatch would be the only
user of these fields at the moment). I don't know if there would be
any users in the future that would be interested in using this Elf
information. Thoughts on this?

>>  #endif
>>
>>  	/* The command line arguments (may be mangled).  People like
>> @@ -461,6 +469,7 @@ struct module {
>>  #endif
>>
>>  #ifdef CONFIG_LIVEPATCH
>> +	bool klp; /* Is this a livepatch module? */
>>  	bool klp_alive;
>>  #endif
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 8f051a1..433c2d6 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1984,6 +1984,13 @@ static void unset_module_core_ro_nx(struct module *mod) { }
>>  static void unset_module_init_ro_nx(struct module *mod) { }
>>  #endif
>>
>> +static void free_module_elf(struct module *mod)
>> +{
>> +	kfree(mod->hdr);
>> +	kfree(mod->sechdrs);
>> +	kfree(mod->secstrings);
>> +}
>> +
>>  void __weak module_memfree(void *module_region)
>>  {
>>  	vfree(module_region);
>> @@ -2022,6 +2029,9 @@ static void free_module(struct module *mod)
>>  	/* Free any allocated parameters. */
>>  	destroy_params(mod->kp, mod->num_kp);
>>
>> +	/* Free Elf information if it was saved */
>> +	free_module_elf(mod);
>> +
>>  	/* Now we can delete it from the lists */
>>  	mutex_lock(&module_mutex);
>>  	/* Unlink carefully: kallsyms could be walking list. */
>> @@ -2137,6 +2147,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>>  			       (long)sym[i].st_value);
>>  			break;
>>
>> +		case SHN_LIVEPATCH:
>> +			/* klp symbols are resolved by livepatch */
>> +			break;
>> +
>>  		case SHN_UNDEF:
>>  			ksym = resolve_symbol_wait(mod, info, name);
>>  			/* Ok if resolved.  */
>> @@ -2185,6 +2199,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>>  			continue;
>>
>> +		/* klp relocation sections are applied by livepatch */
>> +		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
>> +			continue;
>> +
>>  		if (info->sechdrs[i].sh_type == SHT_REL)
>>  			err = apply_relocate(info->sechdrs, info->strtab,
>>  					     info->index.sym, i, mod);
>> @@ -2393,6 +2411,11 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
>>  {
>>  	const Elf_Shdr *sechdrs = info->sechdrs;
>>
>> +	if (ELF_ST_BIND(sym->st_info) == STB_LIVEPATCH_EXT)
>> +		return 'K';
>> +	if (sym->st_shndx == SHN_LIVEPATCH)
>> +		return 'k';
>> +
>>  	if (ELF_ST_BIND(sym->st_info) == STB_WEAK) {
>>  		if (ELF_ST_TYPE(sym->st_info) == STT_OBJECT)
>>  			return 'v';
>> @@ -2475,7 +2498,7 @@ static void layout_symtab(struct module *mod, struct load_info *info)
>>
>>  	/* Compute total space required for the core symbols' strtab. */
>>  	for (ndst = i = 0; i < nsrc; i++) {
>> -		if (i == 0 ||
>> +		if (i == 0 || mod->klp ||
>>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>>  			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
>>  			ndst++;
>
>Instead of accessing mod->klp directly, how about an
>'is_livepatch_module(mod)' function.  There could be two versions, with
>the !LIVEPATCH version always returning false and the LIVEPATCH version
>checking mod->klp.  Then mod->klp itself can stay inside the LIVEPATCH
>ifdef in the module struct.

OK, that sounds good. Makes more sense for the klp field to exist only
in the LIVEPATCH case anyway.

>> @@ -2517,7 +2540,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>>  	mod->core_strtab = s = mod->module_core + info->stroffs;
>>  	src = mod->symtab;
>>  	for (ndst = i = 0; i < mod->num_symtab; i++) {
>> -		if (i == 0 ||
>> +		if (i == 0 || mod->klp ||
>>  		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum)) {
>>  			dst[ndst] = src[i];
>>  			dst[ndst++].st_name = s - mod->core_strtab;
>> @@ -2638,6 +2661,64 @@ static int elf_header_check(struct load_info *info)
>>  	return 0;
>>  }
>>
>> +/*
>> + * copy_module_elf - preserve Elf information about a module
>> + */
>> +static int copy_module_elf(struct module *mod, struct load_info *info)
>> +{
>> +	unsigned int size;
>> +	int ret = 0;
>
>No need to initialize ret to zero here since it's never used
>uninitalized.
>
>> +	Elf_Shdr *symsect;
>> +
>> +	/* Elf header */
>> +	size = sizeof(Elf_Ehdr);
>> +	mod->hdr = kzalloc(size, GFP_KERNEL);
>
>No need to zero the memory here with kzalloc() since it will all be
>memcpy()'d anyway.  kmalloc() can be used instead (and the same for the
>other kzalloc()s below).
>
>> +	if (mod->hdr == NULL) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +	memcpy(mod->hdr, info->hdr, size);
>> +
>> +	/* Elf section header table */
>> +	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
>> +	mod->sechdrs = kzalloc(size, GFP_KERNEL);
>> +	if (mod->sechdrs == NULL) {
>> +		ret = -ENOMEM;
>> +		goto free_hdr;
>> +	}
>> +	memcpy(mod->sechdrs, info->sechdrs, size);
>> +
>> +	/* Elf section name string table */
>> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
>> +	mod->secstrings = kzalloc(size, GFP_KERNEL);
>> +	if (mod->secstrings == NULL) {
>> +		ret = -ENOMEM;
>> +		goto free_sechdrs;
>> +	}
>> +	memcpy(mod->secstrings, info->secstrings, size);
>> +
>> +	/* Elf section indices */
>> +	memcpy(&mod->index, &info->index, sizeof(info->index));
>> +
>> +	/*
>> +	 * Update symtab's sh_addr to point to a valid
>> +	 * symbol table, as the temporary symtab in module
>> +	 * init memory will be freed
>> +	 */
>> +	symsect = mod->sechdrs + mod->index.sym;
>> +	symsect->sh_addr = (unsigned long)mod->core_symtab;
>> +
>> +	return ret;
>> +
>> +free_sechdrs:
>> +	kfree(mod->sechdrs);
>> +free_hdr:
>> +	kfree(mod->hdr);
>> +out:
>> +	return ret;
>> +}
>> +
>> +
>>  #define COPY_CHUNK_SIZE (16*PAGE_SIZE)
>>
>>  static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned long len)
>> @@ -2866,6 +2947,9 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>>  			"is unknown, you have been warned.\n", mod->name);
>>  	}
>>
>> +	if (get_modinfo(info, "livepatch"))
>> +		mod->klp = true;
>> +
>
>Similar to the is_livepatch_module() function I suggested, this can be
>put in a function so that mod->klp can be abstracted away for the
>!LIVEPATCH case.  Maybe there should be a check_livepatch_modinfo()
>function:
>
>1. the !LIVEPATCH version of the function could return an error if
>modinfo has "livepatch"
>
>2. the LIVEPATCH version could simply set mod->klp = true.
>
>>  	/* Set up license info based on the info section */
>>  	set_license(mod, get_modinfo(info, "license"));
>>
>> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>  	if (err < 0)
>>  		goto bug_cleanup;
>>
>> +	/*
>> +	 * Save sechdrs, indices, and other data from info
>> +	 * in order to patch to-be-loaded modules.
>> +	 * Do not call free_copy() for livepatch modules.
>
>I think the last line of this comment isn't right, since free_copy() is
>called below regardless.

Yes you're right, forgot to update that comment.

>> +	 */
>> +	if (mod->klp)
>> +		err = copy_module_elf(mod, info);
>> +	if (err < 0)
>> +		goto bug_cleanup;
>
>Not strictly necessary, but I think it would be a little cleaner to only
>check the err if copy_module_elf() was called.
>
>> +
>>  	/* Get rid of temporary copy. */
>>  	free_copy(info);
>
>-- 
>Josh

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Minfei Huang]

On 11/30/15 at 11:21P, Jessica Yu wrote:
> +	klp_for_each_reloc_sec(obj, reloc_sec) {
> +		relindex = reloc_sec->index;
> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> +
> +		/* For each rela in this __klp_rela section */
> +		for (i = 0; i < num_relas; i++, rela++) {
> +			sym = symtab + ELF_R_SYM(rela->r_info);
> +			symname = pmod->core_strtab + sym->st_name;
> +
> +			if (sym->st_shndx == SHN_LIVEPATCH) {
> +				if (sym->st_info == 'K')
> +					ret = klp_find_external_symbol(pmod, symname, &addr);
> +				else
> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> +				if (ret)
> +					return ret;
> +				sym->st_value = addr;
> +			}
>  		}
> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> +					 pmod->index.sym, relindex, pmod);

It is more appropiate to test the ret.

Thanks
Minfei

>  	}
>  
> -	return 0;
> +	return ret;

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Minfei Huang]

On 11/30/15 at 11:21P, Jessica Yu wrote:
> +	klp_for_each_reloc_sec(obj, reloc_sec) {
> +		relindex = reloc_sec->index;
> +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> +
> +		/* For each rela in this __klp_rela section */
> +		for (i = 0; i < num_relas; i++, rela++) {
> +			sym = symtab + ELF_R_SYM(rela->r_info);
> +			symname = pmod->core_strtab + sym->st_name;
> +
> +			if (sym->st_shndx == SHN_LIVEPATCH) {
> +				if (sym->st_info == 'K')
> +					ret = klp_find_external_symbol(pmod, symname, &addr);
> +				else
> +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> +				if (ret)
> +					return ret;
> +				sym->st_value = addr;
> +			}
>  		}
> +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> +					 pmod->index.sym, relindex, pmod);

It is more appropiate to test the ret.

Thanks
Minfei

>  	}
>  
> -	return 0;
> +	return ret;

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>+	/* For each __klp_rela section for this object */
> >>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>+		relindex = reloc_sec->index;
> >>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>+
> >>+		/* For each rela in this __klp_rela section */
> >>+		for (i = 0; i < num_relas; i++, rela++) {
> >>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>+			symname = pmod->core_strtab + sym->st_name;
> >>+
> >>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>+				if (sym->st_info == 'K')
> >>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>+				else
> >>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>+				if (ret)
> >>+					return ret;
> >>+				sym->st_value = addr;
> >
> >So I think you're also working on removing the 'external' stuff.  Any
> >idea how this code will handle that?  Specifically I'm wondering how the
> >objname and sympos of the rela's symbol will be specified.  Maybe it can
> >be encoded somehow in one of the symbol fields (st_value)?
> 
> Thanks for bringing this up. I think we can just encode the symbol's
> position in kallsyms in the symbol's st_other field. It isn't used
> anywhere and has size char, which is plenty of bits to represent the
> small ints.

st_other does seem to at least have some trivial usage in the kernel,
see print_absolute_symbols() and sym_visibility() in
arch/x86/tools/relocs.c.  Two of the bits are used to specify the
"visibility" of a symbol.  Also readelf shows a "Vis" column in the
symbol table.

> For objname, the simplest solution might be to append ".klp.objname"
> to the symbol name, and extract out this suffix when resolving the
> symbol. Another way might be to have st_value contain the index into
> the strtab (or .kpatch.strings) that contains the objname. Then we'd
> access the objname just like how we access the symbol's name (strtab +
> sym->st_name). After we find the objname we can then overwrite
> st_value with the real address. I think this second method is cleaner.
> Thoughts?

Yeah, I guess there are a lot of possibilities for ways to encode it.

Personally I think it would be nice if the encoding were something that
could easily be seen when dumping the symbol table with readelf.  So,
for example, the objname could be encoded in the symbol name (as you
suggested), and the sympos could be in st_value.

If we do that, it'd probably be good to keep the naming consistent with
the '__klp_rela_objname.symname' thing.  So something like
'_klp_sym_objname.symname'.

But... would there be any side effects associated with renaming it?  For
example, would it cause problems with the s390 PLT?

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>+	/* For each __klp_rela section for this object */
> >>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>+		relindex = reloc_sec->index;
> >>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>+
> >>+		/* For each rela in this __klp_rela section */
> >>+		for (i = 0; i < num_relas; i++, rela++) {
> >>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>+			symname = pmod->core_strtab + sym->st_name;
> >>+
> >>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>+				if (sym->st_info == 'K')
> >>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>+				else
> >>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>+				if (ret)
> >>+					return ret;
> >>+				sym->st_value = addr;
> >
> >So I think you're also working on removing the 'external' stuff.  Any
> >idea how this code will handle that?  Specifically I'm wondering how the
> >objname and sympos of the rela's symbol will be specified.  Maybe it can
> >be encoded somehow in one of the symbol fields (st_value)?
> 
> Thanks for bringing this up. I think we can just encode the symbol's
> position in kallsyms in the symbol's st_other field. It isn't used
> anywhere and has size char, which is plenty of bits to represent the
> small ints.

st_other does seem to at least have some trivial usage in the kernel,
see print_absolute_symbols() and sym_visibility() in
arch/x86/tools/relocs.c.  Two of the bits are used to specify the
"visibility" of a symbol.  Also readelf shows a "Vis" column in the
symbol table.

> For objname, the simplest solution might be to append ".klp.objname"
> to the symbol name, and extract out this suffix when resolving the
> symbol. Another way might be to have st_value contain the index into
> the strtab (or .kpatch.strings) that contains the objname. Then we'd
> access the objname just like how we access the symbol's name (strtab +
> sym->st_name). After we find the objname we can then overwrite
> st_value with the real address. I think this second method is cleaner.
> Thoughts?

Yeah, I guess there are a lot of possibilities for ways to encode it.

Personally I think it would be nice if the encoding were something that
could easily be seen when dumping the symbol table with readelf.  So,
for example, the objname could be encoded in the symbol name (as you
suggested), and the sympos could be in st_value.

If we do that, it'd probably be good to keep the naming consistent with
the '__klp_rela_objname.symname' thing.  So something like
'_klp_sym_objname.symname'.

But... would there be any side effects associated with renaming it?  For
example, would it cause problems with the s390 PLT?

-- 
Josh

Re: module: preserve Elf information for livepatch modules

[Josh Poimboeuf]

On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> >>For livepatch modules, copy Elf section, symbol, and string information
> >>from the load_info struct in the module loader.
> >>
> >>Livepatch uses special relocation sections in order to be able to patch
> >>modules that are not yet loaded, as well as apply patches to the kernel
> >>when the addresses of symbols cannot be determined at compile time (for
> >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> >>information such as section indices in order to apply the remaining
> >>relocation sections at the appropriate time (i.e. when the target module
> >>loads).
> >>
> >>Signed-off-by: Jessica Yu <jeyu@redhat.com>
> >>---
> >> include/linux/module.h |  9 +++++
> >> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >> 2 files changed, 105 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 3a19c79..9b46256 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -425,6 +425,14 @@ struct module {
> >>
> >> 	/* Notes attributes */
> >> 	struct module_notes_attrs *notes_attrs;
> >>+
> >>+	/* Elf information (optionally saved) */
> >>+	Elf_Ehdr *hdr;
> >
> >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> >
> >>+	Elf_Shdr *sechdrs;
> >>+	char *secstrings;
> >
> >Probably a good idea to add underscores to the names ("sec_hdrs" and
> >"sec_strings") to be consistent with most of the other fields in the
> >struct.
> >
> >>+	struct {
> >>+		unsigned int sym, str, mod, vers, info, pcpu;
> >>+	} index;
> >
> >I might be contradicting myself from what I said before.  But I'm
> >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> >Then below, there could be two versions of copy_module_elf(), the real
> >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> >story for free_module_elf().
> 
> I think in the v1 discussion we were leaning more towards making this
> generic. We could potentially just have the Elf module fields
> available in the generic case, independent of whether CONFIG_LIVEPATCH
> is set, whereas the mod->klp field should probably be only available
> when LIVEPATCH is set. I think this makes sense since the Elf fields
> aren't dependent on livepatch (although livepatch would be the only
> user of these fields at the moment). I don't know if there would be
> any users in the future that would be interested in using this Elf
> information. Thoughts on this?

IIRC, I think I made the suggestion to always save the elf fields
because otherwise it was looking like we were going to need a lot of
spaghetti code.

But if we can find a way to wrap the elf fields in LIVEPATCH while
keeping the code simple, then there's no real downside and I think
that's the way to go.  If somebody else wants to use the fields later,
then they can remove or change the ifdefs as needed.

-- 
Josh

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Jiri Kosina]

On Thu, 10 Dec 2015, Minfei Huang wrote:

> > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > +		relindex = reloc_sec->index;
> > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > +
> > +		/* For each rela in this __klp_rela section */
> > +		for (i = 0; i < num_relas; i++, rela++) {
> > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > +			symname = pmod->core_strtab + sym->st_name;
> > +
> > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > +				if (sym->st_info == 'K')
> > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > +				else
> > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > +				if (ret)
> > +					return ret;
> > +				sym->st_value = addr;
> > +			}
> >  		}
> > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > +					 pmod->index.sym, relindex, pmod);
> 
> It is more appropiate to test the ret.

And if error is encountered ... then you'd propose to do what?

The code is correct as is; the return value is propagated properly to 
caller, and all the existing callers have proper error handling.

-- 
Jiri Kosina
SUSE Labs

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Jiri Kosina]

On Thu, 10 Dec 2015, Minfei Huang wrote:

> > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > +		relindex = reloc_sec->index;
> > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > +
> > +		/* For each rela in this __klp_rela section */
> > +		for (i = 0; i < num_relas; i++, rela++) {
> > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > +			symname = pmod->core_strtab + sym->st_name;
> > +
> > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > +				if (sym->st_info == 'K')
> > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > +				else
> > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > +				if (ret)
> > +					return ret;
> > +				sym->st_value = addr;
> > +			}
> >  		}
> > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > +					 pmod->index.sym, relindex, pmod);
> 
> It is more appropiate to test the ret.

And if error is encountered ... then you'd propose to do what?

The code is correct as is; the return value is propagated properly to 
caller, and all the existing callers have proper error handling.

-- 
Jiri Kosina
SUSE Labs

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 08:56:27PM +0100, Jiri Kosina wrote:
> On Thu, 10 Dec 2015, Minfei Huang wrote:
> 
> > > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > > +		relindex = reloc_sec->index;
> > > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > > +
> > > +		/* For each rela in this __klp_rela section */
> > > +		for (i = 0; i < num_relas; i++, rela++) {
> > > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > > +			symname = pmod->core_strtab + sym->st_name;
> > > +
> > > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > > +				if (sym->st_info == 'K')
> > > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > > +				else
> > > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > > +				if (ret)
> > > +					return ret;
> > > +				sym->st_value = addr;
> > > +			}
> > >  		}
> > > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > > +					 pmod->index.sym, relindex, pmod);
> > 
> > It is more appropiate to test the ret.
> 
> And if error is encountered ... then you'd propose to do what?
> 
> The code is correct as is; the return value is propagated properly to 
> caller, and all the existing callers have proper error handling.

apply_relocate_add() is called from inside a loop, so if there's an
error, it should return immediately instead of continuing the next
iteration.

-- 
Josh

Re: [RFC PATCH v2 4/6] livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 08:56:27PM +0100, Jiri Kosina wrote:
> On Thu, 10 Dec 2015, Minfei Huang wrote:
> 
> > > +	klp_for_each_reloc_sec(obj, reloc_sec) {
> > > +		relindex = reloc_sec->index;
> > > +		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> > > +		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> > > +
> > > +		/* For each rela in this __klp_rela section */
> > > +		for (i = 0; i < num_relas; i++, rela++) {
> > > +			sym = symtab + ELF_R_SYM(rela->r_info);
> > > +			symname = pmod->core_strtab + sym->st_name;
> > > +
> > > +			if (sym->st_shndx == SHN_LIVEPATCH) {
> > > +				if (sym->st_info == 'K')
> > > +					ret = klp_find_external_symbol(pmod, symname, &addr);
> > > +				else
> > > +					ret = klp_find_object_symbol(obj->name, symname, &addr);
> > > +				if (ret)
> > > +					return ret;
> > > +				sym->st_value = addr;
> > > +			}
> > >  		}
> > > +		ret = apply_relocate_add(pmod->sechdrs, pmod->core_strtab,
> > > +					 pmod->index.sym, relindex, pmod);
> > 
> > It is more appropiate to test the ret.
> 
> And if error is encountered ... then you'd propose to do what?
> 
> The code is correct as is; the return value is propagated properly to 
> caller, and all the existing callers have proper error handling.

apply_relocate_add() is called from inside a loop, so if there's an
error, it should return immediately instead of continuing the next
iteration.

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [10/12/15 08:28 -0600]:
>On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> >>+	/* For each __klp_rela section for this object */
>> >>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>> >>+		relindex = reloc_sec->index;
>> >>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> >>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> >>+
>> >>+		/* For each rela in this __klp_rela section */
>> >>+		for (i = 0; i < num_relas; i++, rela++) {
>> >>+			sym = symtab + ELF_R_SYM(rela->r_info);
>> >>+			symname = pmod->core_strtab + sym->st_name;
>> >>+
>> >>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>> >>+				if (sym->st_info == 'K')
>> >>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>> >>+				else
>> >>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> >>+				if (ret)
>> >>+					return ret;
>> >>+				sym->st_value = addr;
>> >
>> >So I think you're also working on removing the 'external' stuff.  Any
>> >idea how this code will handle that?  Specifically I'm wondering how the
>> >objname and sympos of the rela's symbol will be specified.  Maybe it can
>> >be encoded somehow in one of the symbol fields (st_value)?
>>
>> Thanks for bringing this up. I think we can just encode the symbol's
>> position in kallsyms in the symbol's st_other field. It isn't used
>> anywhere and has size char, which is plenty of bits to represent the
>> small ints.
>
>st_other does seem to at least have some trivial usage in the kernel,
>see print_absolute_symbols() and sym_visibility() in
>arch/x86/tools/relocs.c.  Two of the bits are used to specify the
>"visibility" of a symbol.  Also readelf shows a "Vis" column in the
>symbol table.

Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
shouldn't be affected in this case...but despite its sparse usage in the
kernel it does look like using st_other to encode sympos is out of the
question as its meaning is architecture specific..

>> For objname, the simplest solution might be to append ".klp.objname"
>> to the symbol name, and extract out this suffix when resolving the
>> symbol. Another way might be to have st_value contain the index into
>> the strtab (or .kpatch.strings) that contains the objname. Then we'd
>> access the objname just like how we access the symbol's name (strtab +
>> sym->st_name). After we find the objname we can then overwrite
>> st_value with the real address. I think this second method is cleaner.
>> Thoughts?
>
>Yeah, I guess there are a lot of possibilities for ways to encode it.
>
>Personally I think it would be nice if the encoding were something that
>could easily be seen when dumping the symbol table with readelf.  So,
>for example, the objname could be encoded in the symbol name (as you
>suggested), and the sympos could be in st_value.

Sure, that should be doable. So the new process might look like this:

For every livepatch symbol referenced by a rela..

1) Save the sympos encoded in st_value

2) Save the sym_objname that is encoded in the symbol's name with the
'klp' suffix (Just to clarify: the sym_objname specifies the object
in which the symbol lives, and recall that we need this to remove the
need for the "external" flag)

3) Resolve the symbol by using its name (without the klp suffix),
sympos, and sym_objname

4) Set st_value to the found address

>If we do that, it'd probably be good to keep the naming consistent with
>the '__klp_rela_objname.symname' thing.  So something like
>'_klp_sym_objname.symname'.

How about 'symname.klp.objname', and renaming the klp reloc sections
to '.klp.objname.rela.section_name'? Special symbol suffixes and
section names seem to always use '.', so maybe this would look better?
:-) But we can keep the underscores if people like that more. Both
naming methods would work, it is only a matter of preference.

>But... would there be any side effects associated with renaming it?  For
>example, would it cause problems with the s390 PLT?

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>>>+	/* For each __klp_rela section for this object */
> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>>>+		relindex = reloc_sec->index;
> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>>>+
> >>>>+		/* For each rela in this __klp_rela section */
> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>>>+			symname = pmod->core_strtab + sym->st_name;
> >>>>+
> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>>>+				if (sym->st_info == 'K')
> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>>>+				else
> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>>>+				if (ret)
> >>>>+					return ret;
> >>>>+				sym->st_value = addr;
> >>>
> >>>So I think you're also working on removing the 'external' stuff.  Any
> >>>idea how this code will handle that?  Specifically I'm wondering how the
> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
> >>>be encoded somehow in one of the symbol fields (st_value)?
> >>
> >>Thanks for bringing this up. I think we can just encode the symbol's
> >>position in kallsyms in the symbol's st_other field. It isn't used
> >>anywhere and has size char, which is plenty of bits to represent the
> >>small ints.
> >
> >st_other does seem to at least have some trivial usage in the kernel,
> >see print_absolute_symbols() and sym_visibility() in
> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
> >symbol table.
> 
> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
> shouldn't be affected in this case...but despite its sparse usage in the
> kernel it does look like using st_other to encode sympos is out of the
> question as its meaning is architecture specific..
> 
> >>For objname, the simplest solution might be to append ".klp.objname"
> >>to the symbol name, and extract out this suffix when resolving the
> >>symbol. Another way might be to have st_value contain the index into
> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
> >>access the objname just like how we access the symbol's name (strtab +
> >>sym->st_name). After we find the objname we can then overwrite
> >>st_value with the real address. I think this second method is cleaner.
> >>Thoughts?
> >
> >Yeah, I guess there are a lot of possibilities for ways to encode it.
> >
> >Personally I think it would be nice if the encoding were something that
> >could easily be seen when dumping the symbol table with readelf.  So,
> >for example, the objname could be encoded in the symbol name (as you
> >suggested), and the sympos could be in st_value.
> 
> Sure, that should be doable. So the new process might look like this:
> 
> For every livepatch symbol referenced by a rela..
> 
> 1) Save the sympos encoded in st_value
> 
> 2) Save the sym_objname that is encoded in the symbol's name with the
> 'klp' suffix (Just to clarify: the sym_objname specifies the object
> in which the symbol lives, and recall that we need this to remove the
> need for the "external" flag)
> 
> 3) Resolve the symbol by using its name (without the klp suffix),
> sympos, and sym_objname
> 
> 4) Set st_value to the found address

Sounds right to me.

> >If we do that, it'd probably be good to keep the naming consistent with
> >the '__klp_rela_objname.symname' thing.  So something like
> >'_klp_sym_objname.symname'.
> 
> How about 'symname.klp.objname', and renaming the klp reloc sections
> to '.klp.objname.rela.section_name'? Special symbol suffixes and
> section names seem to always use '.', so maybe this would look better?
> :-) But we can keep the underscores if people like that more. Both
> naming methods would work, it is only a matter of preference.

It's your patches, so I'd say you get to pick ;-)  My only request would
be some consistency between the symbol names and the rela section names.

> >But... would there be any side effects associated with renaming it?  For
> >example, would it cause problems with the s390 PLT?

Just to verify, did you see this question? :-)

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> >>>>+	/* For each __klp_rela section for this object */
> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
> >>>>+		relindex = reloc_sec->index;
> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
> >>>>+
> >>>>+		/* For each rela in this __klp_rela section */
> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
> >>>>+			symname = pmod->core_strtab + sym->st_name;
> >>>>+
> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
> >>>>+				if (sym->st_info == 'K')
> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
> >>>>+				else
> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
> >>>>+				if (ret)
> >>>>+					return ret;
> >>>>+				sym->st_value = addr;
> >>>
> >>>So I think you're also working on removing the 'external' stuff.  Any
> >>>idea how this code will handle that?  Specifically I'm wondering how the
> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
> >>>be encoded somehow in one of the symbol fields (st_value)?
> >>
> >>Thanks for bringing this up. I think we can just encode the symbol's
> >>position in kallsyms in the symbol's st_other field. It isn't used
> >>anywhere and has size char, which is plenty of bits to represent the
> >>small ints.
> >
> >st_other does seem to at least have some trivial usage in the kernel,
> >see print_absolute_symbols() and sym_visibility() in
> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
> >symbol table.
> 
> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
> shouldn't be affected in this case...but despite its sparse usage in the
> kernel it does look like using st_other to encode sympos is out of the
> question as its meaning is architecture specific..
> 
> >>For objname, the simplest solution might be to append ".klp.objname"
> >>to the symbol name, and extract out this suffix when resolving the
> >>symbol. Another way might be to have st_value contain the index into
> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
> >>access the objname just like how we access the symbol's name (strtab +
> >>sym->st_name). After we find the objname we can then overwrite
> >>st_value with the real address. I think this second method is cleaner.
> >>Thoughts?
> >
> >Yeah, I guess there are a lot of possibilities for ways to encode it.
> >
> >Personally I think it would be nice if the encoding were something that
> >could easily be seen when dumping the symbol table with readelf.  So,
> >for example, the objname could be encoded in the symbol name (as you
> >suggested), and the sympos could be in st_value.
> 
> Sure, that should be doable. So the new process might look like this:
> 
> For every livepatch symbol referenced by a rela..
> 
> 1) Save the sympos encoded in st_value
> 
> 2) Save the sym_objname that is encoded in the symbol's name with the
> 'klp' suffix (Just to clarify: the sym_objname specifies the object
> in which the symbol lives, and recall that we need this to remove the
> need for the "external" flag)
> 
> 3) Resolve the symbol by using its name (without the klp suffix),
> sympos, and sym_objname
> 
> 4) Set st_value to the found address

Sounds right to me.

> >If we do that, it'd probably be good to keep the naming consistent with
> >the '__klp_rela_objname.symname' thing.  So something like
> >'_klp_sym_objname.symname'.
> 
> How about 'symname.klp.objname', and renaming the klp reloc sections
> to '.klp.objname.rela.section_name'? Special symbol suffixes and
> section names seem to always use '.', so maybe this would look better?
> :-) But we can keep the underscores if people like that more. Both
> naming methods would work, it is only a matter of preference.

It's your patches, so I'd say you get to pick ;-)  My only request would
be some consistency between the symbol names and the rela section names.

> >But... would there be any side effects associated with renaming it?  For
> >example, would it cause problems with the s390 PLT?

Just to verify, did you see this question? :-)

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [10/12/15 15:41 -0600]:
>On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
>> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
>> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> >>>>+	/* For each __klp_rela section for this object */
>> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>> >>>>+		relindex = reloc_sec->index;
>> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> >>>>+
>> >>>>+		/* For each rela in this __klp_rela section */
>> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
>> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
>> >>>>+			symname = pmod->core_strtab + sym->st_name;
>> >>>>+
>> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>> >>>>+				if (sym->st_info == 'K')
>> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>> >>>>+				else
>> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> >>>>+				if (ret)
>> >>>>+					return ret;
>> >>>>+				sym->st_value = addr;
>> >>>
>> >>>So I think you're also working on removing the 'external' stuff.  Any
>> >>>idea how this code will handle that?  Specifically I'm wondering how the
>> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
>> >>>be encoded somehow in one of the symbol fields (st_value)?
>> >>
>> >>Thanks for bringing this up. I think we can just encode the symbol's
>> >>position in kallsyms in the symbol's st_other field. It isn't used
>> >>anywhere and has size char, which is plenty of bits to represent the
>> >>small ints.
>> >
>> >st_other does seem to at least have some trivial usage in the kernel,
>> >see print_absolute_symbols() and sym_visibility() in
>> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
>> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
>> >symbol table.
>>
>> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
>> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
>> shouldn't be affected in this case...but despite its sparse usage in the
>> kernel it does look like using st_other to encode sympos is out of the
>> question as its meaning is architecture specific..
>>
>> >>For objname, the simplest solution might be to append ".klp.objname"
>> >>to the symbol name, and extract out this suffix when resolving the
>> >>symbol. Another way might be to have st_value contain the index into
>> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
>> >>access the objname just like how we access the symbol's name (strtab +
>> >>sym->st_name). After we find the objname we can then overwrite
>> >>st_value with the real address. I think this second method is cleaner.
>> >>Thoughts?
>> >
>> >Yeah, I guess there are a lot of possibilities for ways to encode it.
>> >
>> >Personally I think it would be nice if the encoding were something that
>> >could easily be seen when dumping the symbol table with readelf.  So,
>> >for example, the objname could be encoded in the symbol name (as you
>> >suggested), and the sympos could be in st_value.
>>
>> Sure, that should be doable. So the new process might look like this:
>>
>> For every livepatch symbol referenced by a rela..
>>
>> 1) Save the sympos encoded in st_value
>>
>> 2) Save the sym_objname that is encoded in the symbol's name with the
>> 'klp' suffix (Just to clarify: the sym_objname specifies the object
>> in which the symbol lives, and recall that we need this to remove the
>> need for the "external" flag)
>>
>> 3) Resolve the symbol by using its name (without the klp suffix),
>> sympos, and sym_objname
>>
>> 4) Set st_value to the found address
>
>Sounds right to me.
>
>> >If we do that, it'd probably be good to keep the naming consistent with
>> >the '__klp_rela_objname.symname' thing.  So something like
>> >'_klp_sym_objname.symname'.
>>
>> How about 'symname.klp.objname', and renaming the klp reloc sections
>> to '.klp.objname.rela.section_name'? Special symbol suffixes and
>> section names seem to always use '.', so maybe this would look better?
>> :-) But we can keep the underscores if people like that more. Both
>> naming methods would work, it is only a matter of preference.
>
>It's your patches, so I'd say you get to pick ;-)  My only request would
>be some consistency between the symbol names and the rela section names.
>
>> >But... would there be any side effects associated with renaming it?  For
>> >example, would it cause problems with the s390 PLT?
>
>Just to verify, did you see this question? :-)

Ah, sorry I missed this! So I looked through the module loader code
for x86, s390, arm (and just grepped in a few other archs) and the
symbol's name (st_name) is only ever used in printing error or debug
messages, or not used at all. As long as livepatch correctly resolves
the symbol, and st_value is set, apply_relocate_add() doesn't really
care about the name.

s390 plt/got information (struct mod_arch_syminfo) for each symbol is
stored in an array in mod->arch.syminfo. Every symbol that is in the
module's symbol table will get an entry, but not every symbol will
have their plt/got offsets filled in. Every symbol referenced by a
PLT/GOT rela will have those offsets filled in. Since our livepatch
symbols are real symbols, with indexes and entries in the symbol
table, they will get their own s390 mod_arch_syminfo struct, and
should any PLT rela reference them, a PLT entry as well, which is
good. Tldr, names of symbols don't affect s390 plt/got handling, so we
are safe.

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [10/12/15 15:41 -0600]:
>On Thu, Dec 10, 2015 at 04:33:29PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [10/12/15 08:28 -0600]:
>> >On Wed, Dec 09, 2015 at 02:10:14PM -0500, Jessica Yu wrote:
>> >>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> >>>>+	/* For each __klp_rela section for this object */
>> >>>>+	klp_for_each_reloc_sec(obj, reloc_sec) {
>> >>>>+		relindex = reloc_sec->index;
>> >>>>+		num_relas = pmod->sechdrs[relindex].sh_size / sizeof(Elf_Rela);
>> >>>>+		rela = (Elf_Rela *) pmod->sechdrs[relindex].sh_addr;
>> >>>>+
>> >>>>+		/* For each rela in this __klp_rela section */
>> >>>>+		for (i = 0; i < num_relas; i++, rela++) {
>> >>>>+			sym = symtab + ELF_R_SYM(rela->r_info);
>> >>>>+			symname = pmod->core_strtab + sym->st_name;
>> >>>>+
>> >>>>+			if (sym->st_shndx == SHN_LIVEPATCH) {
>> >>>>+				if (sym->st_info == 'K')
>> >>>>+					ret = klp_find_external_symbol(pmod, symname, &addr);
>> >>>>+				else
>> >>>>+					ret = klp_find_object_symbol(obj->name, symname, &addr);
>> >>>>+				if (ret)
>> >>>>+					return ret;
>> >>>>+				sym->st_value = addr;
>> >>>
>> >>>So I think you're also working on removing the 'external' stuff.  Any
>> >>>idea how this code will handle that?  Specifically I'm wondering how the
>> >>>objname and sympos of the rela's symbol will be specified.  Maybe it can
>> >>>be encoded somehow in one of the symbol fields (st_value)?
>> >>
>> >>Thanks for bringing this up. I think we can just encode the symbol's
>> >>position in kallsyms in the symbol's st_other field. It isn't used
>> >>anywhere and has size char, which is plenty of bits to represent the
>> >>small ints.
>> >
>> >st_other does seem to at least have some trivial usage in the kernel,
>> >see print_absolute_symbols() and sym_visibility() in
>> >arch/x86/tools/relocs.c.  Two of the bits are used to specify the
>> >"visibility" of a symbol.  Also readelf shows a "Vis" column in the
>> >symbol table.
>>
>> Yeah, for x86 it looks like st_other is used only for SHN_ABS symbols
>> in print_absolute_symbols(). Technically SHN_LIVEPATCH symbols
>> shouldn't be affected in this case...but despite its sparse usage in the
>> kernel it does look like using st_other to encode sympos is out of the
>> question as its meaning is architecture specific..
>>
>> >>For objname, the simplest solution might be to append ".klp.objname"
>> >>to the symbol name, and extract out this suffix when resolving the
>> >>symbol. Another way might be to have st_value contain the index into
>> >>the strtab (or .kpatch.strings) that contains the objname. Then we'd
>> >>access the objname just like how we access the symbol's name (strtab +
>> >>sym->st_name). After we find the objname we can then overwrite
>> >>st_value with the real address. I think this second method is cleaner.
>> >>Thoughts?
>> >
>> >Yeah, I guess there are a lot of possibilities for ways to encode it.
>> >
>> >Personally I think it would be nice if the encoding were something that
>> >could easily be seen when dumping the symbol table with readelf.  So,
>> >for example, the objname could be encoded in the symbol name (as you
>> >suggested), and the sympos could be in st_value.
>>
>> Sure, that should be doable. So the new process might look like this:
>>
>> For every livepatch symbol referenced by a rela..
>>
>> 1) Save the sympos encoded in st_value
>>
>> 2) Save the sym_objname that is encoded in the symbol's name with the
>> 'klp' suffix (Just to clarify: the sym_objname specifies the object
>> in which the symbol lives, and recall that we need this to remove the
>> need for the "external" flag)
>>
>> 3) Resolve the symbol by using its name (without the klp suffix),
>> sympos, and sym_objname
>>
>> 4) Set st_value to the found address
>
>Sounds right to me.
>
>> >If we do that, it'd probably be good to keep the naming consistent with
>> >the '__klp_rela_objname.symname' thing.  So something like
>> >'_klp_sym_objname.symname'.
>>
>> How about 'symname.klp.objname', and renaming the klp reloc sections
>> to '.klp.objname.rela.section_name'? Special symbol suffixes and
>> section names seem to always use '.', so maybe this would look better?
>> :-) But we can keep the underscores if people like that more. Both
>> naming methods would work, it is only a matter of preference.
>
>It's your patches, so I'd say you get to pick ;-)  My only request would
>be some consistency between the symbol names and the rela section names.
>
>> >But... would there be any side effects associated with renaming it?  For
>> >example, would it cause problems with the s390 PLT?
>
>Just to verify, did you see this question? :-)

Ah, sorry I missed this! So I looked through the module loader code
for x86, s390, arm (and just grepped in a few other archs) and the
symbol's name (st_name) is only ever used in printing error or debug
messages, or not used at all. As long as livepatch correctly resolves
the symbol, and st_value is set, apply_relocate_add() doesn't really
care about the name.

s390 plt/got information (struct mod_arch_syminfo) for each symbol is
stored in an array in mod->arch.syminfo. Every symbol that is in the
module's symbol table will get an entry, but not every symbol will
have their plt/got offsets filled in. Every symbol referenced by a
PLT/GOT rela will have those offsets filled in. Since our livepatch
symbols are real symbols, with indexes and entries in the symbol
table, they will get their own s390 mod_arch_syminfo struct, and
should any PLT rela reference them, a PLT entry as well, which is
good. Tldr, names of symbols don't affect s390 plt/got handling, so we
are safe.

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Jessica Yu [09/12/15 14:10 -0500]:
>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>>On Mon, Nov 30, 2015 at 11:21:17PM -0500, Jessica Yu wrote:
>>>Reuse module loader code to write relocations, thereby eliminating the need
>>>for architecture specific relocation code in livepatch. Namely, we reuse
>>>apply_relocate_add() in the module loader to write relocations instead of
>>>duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>>>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>>>are resolved and then apply_relocate_add() is called to apply those
>>>relocations.
>>>
>>>In addition, remove x86 livepatch relocation code. It is no longer needed
>>>since symbol resolution and relocation work have been offloaded to module
>>>loader.
>>>
>>>Signed-off-by: Jessica Yu <jeyu@redhat.com>
>>>---
>>> arch/x86/include/asm/livepatch.h |  2 -
>>> arch/x86/kernel/Makefile         |  1 -
>>> arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>>> include/linux/livepatch.h        | 30 ++++++-------
>>> include/linux/module.h           |  6 +++
>>> kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>>> 6 files changed, 70 insertions(+), 154 deletions(-)
>>> delete mode 100644 arch/x86/kernel/livepatch.c
>>>
>>>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>>>index 19c099a..7312e25 100644
>>>--- a/arch/x86/include/asm/livepatch.h
>>>+++ b/arch/x86/include/asm/livepatch.h
>>>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>>> #endif
>>> 	return 0;
>>> }
>>>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>-			   unsigned long loc, unsigned long value);
>>>
>>> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>> {
>>>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>>>index b1b78ff..c5e9a5c 100644
>>>--- a/arch/x86/kernel/Makefile
>>>+++ b/arch/x86/kernel/Makefile
>>>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>>> obj-y				+= apic/
>>> obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>>> obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>>>-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>>> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>>> obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>>> obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>>>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>>>deleted file mode 100644
>>>index d1d35cc..0000000
>>>--- a/arch/x86/kernel/livepatch.c
>>>+++ /dev/null
>>>@@ -1,91 +0,0 @@
>>>-/*
>>>- * livepatch.c - x86-specific Kernel Live Patching Core
>>>- *
>>>- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
>>>- * Copyright (C) 2014 SUSE
>>>- *
>>>- * 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/>.
>>>- */
>>>-
>>>-#include <linux/module.h>
>>>-#include <linux/uaccess.h>
>>>-#include <asm/cacheflush.h>
>>>-#include <asm/page_types.h>
>>>-#include <asm/elf.h>
>>>-#include <asm/livepatch.h>
>>>-
>>>-/**
>>>- * klp_write_module_reloc() - write a relocation in a module
>>>- * @mod:	module in which the section to be modified is found
>>>- * @type:	ELF relocation type (see asm/elf.h)
>>>- * @loc:	address that the relocation should be written to
>>>- * @value:	relocation value (sym address + addend)
>>>- *
>>>- * This function writes a relocation to the specified location for
>>>- * a particular module.
>>>- */
>>>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>-			   unsigned long loc, unsigned long value)
>>>-{
>>>-	int ret, numpages, size = 4;
>>>-	bool readonly;
>>>-	unsigned long val;
>>>-	unsigned long core = (unsigned long)mod->module_core;
>>>-	unsigned long core_size = mod->core_size;
>>>-
>>>-	switch (type) {
>>>-	case R_X86_64_NONE:
>>>-		return 0;
>>>-	case R_X86_64_64:
>>>-		val = value;
>>>-		size = 8;
>>>-		break;
>>>-	case R_X86_64_32:
>>>-		val = (u32)value;
>>>-		break;
>>>-	case R_X86_64_32S:
>>>-		val = (s32)value;
>>>-		break;
>>>-	case R_X86_64_PC32:
>>>-		val = (u32)(value - loc);
>>>-		break;
>>>-	default:
>>>-		/* unsupported relocation type */
>>>-		return -EINVAL;
>>>-	}
>>>-
>>>-	if (loc < core || loc >= core + core_size)
>>>-		/* loc does not point to any symbol inside the module */
>>>-		return -EINVAL;
>>>-
>>>-	readonly = false;
>>>-
>>>-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>>>-	if (loc < core + mod->core_ro_size)
>>>-		readonly = true;
>>>-#endif
>>>-
>>>-	/* determine if the relocation spans a page boundary */
>>>-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>>>-
>>>-	if (readonly)
>>>-		set_memory_rw(loc & PAGE_MASK, numpages);
>>>-
>>>-	ret = probe_kernel_write((void *)loc, &val, size);
>>>-
>>>-	if (readonly)
>>>-		set_memory_ro(loc & PAGE_MASK, numpages);
>>>-
>>>-	return ret;
>>>-}
>>>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>>index 31db7a0..54f62a6 100644
>>>--- a/include/linux/livepatch.h
>>>+++ b/include/linux/livepatch.h
>>>@@ -64,28 +64,21 @@ struct klp_func {
>>> };
>>>
>>> /**
>>>- * struct klp_reloc - relocation structure for live patching
>>>- * @loc:	address where the relocation will be written
>>>- * @val:	address of the referenced symbol (optional,
>>>- *		vmlinux	patches only)
>>>- * @type:	ELF relocation type
>>>- * @name:	name of the referenced symbol (for lookup/verification)
>>>- * @addend:	offset from the referenced symbol
>>>- * @external:	symbol is either exported or within the live patch module itself
>>>+ * struct klp_reloc_sec - relocation section struct for live patching
>>>+ * @index:	Elf section index of the relocation section
>>>+ * @name:	name of the relocation section
>>>+ * @objname:	name of the object associated with the klp reloc section
>>>  */
>>>-struct klp_reloc {
>>>-	unsigned long loc;
>>>-	unsigned long val;
>>>-	unsigned long type;
>>>-	const char *name;
>>>-	int addend;
>>>-	int external;
>>>+struct klp_reloc_sec {
>>>+	unsigned int index;
>>>+	char *name;
>>>+	char *objname;
>>> };
>>>
>>> /**
>>>  * struct klp_object - kernel object structure for live patching
>>>  * @name:	module name (or NULL for vmlinux)
>>>- * @relocs:	relocation entries to be applied at load time
>>>+ * @reloc_secs:	relocation sections to be applied at load time
>>>  * @funcs:	function entries for functions to be patched in the object
>>>  * @kobj:	kobject for sysfs resources
>>>  * @mod:	kernel module associated with the patched object
>>>@@ -95,7 +88,7 @@ struct klp_reloc {
>>> struct klp_object {
>>> 	/* external */
>>> 	const char *name;
>>>-	struct klp_reloc *relocs;
>>>+	struct klp_reloc_sec *reloc_secs;
>>
>>There was a lot of discussion for v1, so I'm not sure, but I thought we
>>ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>>think the symbol table can be walked directly looking for klp rela
>>sections?
>>
>>The benefits of doing so would be to make the interface simpler -- no
>>need to "cache" the section metadata when it's already easily and
>>quickly available in the module struct elf section headers.
>
>Ah, I might have interpreted the conclusions of that last discussion
>incorrectly.
>
>In that case, I think I can just get rid of my klp_for_each_reloc_sec
>macro as well as the lreloc scaffolding code from kpatch. The only
>potentially "ugly" part of this change is that I'll have to move the
>string parsing stuff here to extract the objname of the __klp_rela
>section (which may not actually look that bad, we'll see how that
>turns out).

Turns out the string parsing stuff, even with the help of lib/string.c, doesn't
look very pretty. As I'm working on v3, I'm starting to think having
klp_write_object_relocations() loop simply through all the elf sections might
not be a good idea. Let me explain.

I don't like the amount of string manipulation code that would potentially come
with this change. Even with a string as simple as ".klp.rela.objname", we'll
end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
chop the section name string in place, which I don't think we should do) that
are going to be required at every iteration of the loop, all just to be able to
call strcmp() and see if we're dealing with a klp rela section that belongs to
the object in question. This also leads to more complicated error handling.

In v1, the string parsing was done only *once* for each klp rela section in the
patch module code, and each klp rela section is sorted into their respective
object with the reloc_secs list. Then all klp_write_object_relocations() had to
do is iterate over object->reloc_secs. The tradeoff here is the addition of
another struct (klp_reloc_sec), but that is not nearly as bad as string
parsing, which is imo way more error prone and is unnecessary extra work.

Here's some pseudocode to help visualize the potential issues:

function klp_write_object_relocations(..,struct klp_object *obj,..) {
    for each section in mod->sechdrs { // iterate through all elf sections, no more obj->reloc_secs list
        if not a klp rela section
            continue;
        sec_objname = extract_objname_from_section(section);  // .klp.rela.[objname].sectionname
        if (strcmp(sec_objname, obj->name)) { // this klp rela section doesn't belong to this object
            kfree(sec_objname);
            continue;
        }

      for each rela in this klp rela section {
         sym = symtab + ELF_R_SYM(rela->r_info);
         sym_objname = extract_objname_from_symbol(sym) // symname.klp.[objname]
         if (!sym_objname)
             goto handle_error; // calls kfree(sec_objname);
         symname = extract_symname_from_symbol(sym); // [symname].klp.objname
         if (!symname)
             goto handle_error; // calls kfree(sec_objname);
         ret = klp_find_object_symbol(sym_objname, symname, &addr)
         if (ret)
             goto handle_error2; // calls kfree(symname), then kfree(sec_objname)

...etc., you get the idea how messy that is getting, and we haven't even
reached the call to apply_relocate_add() yet.

So I personally think it is better to keep the old v1 way for applying the klp
reloc secs. The sections are still named ".klp.rela.objname" but the parsing is
done once in the patch module code and used only to build the patch scaffolding
structure.

In order to avoid adding any additional string parsing code in v3, I no longer
thing we should rename livepatch symbols to include the symbol objname (i.e.
'symbol.klp.objname'). Recall that this change is for getting rid of external
dynrelas. Instead of parsing the symbol name, we could just encode 1) the
sympos and 2) the index into the .kpatch.strings strtab (that contains the
sym_objname) in sym->st_value. We define two masks (KLP_MASK_SYMPOS,
KLP_MASK_OBJNAME) that extract these values. st_value has either a 32 bit or 64
bit size, both of which are big enough sizes for the sympos and string table
index. One perk we lose is being able to see which object a symbol belongs to
in readelf, but then again we didn't have this benefit to begin with in
v1 nor v2 (where we didn't know the symbol's object and had to use
klp_find_external_symbol anyway), so I wouldn't say it's a loss.

Apologies for the super long explanations, but I think avoiding string parsing
in livepatch core code will make the relocation code much more succinct and
easier to read. Any objections or thoughts on all this?

Thanks,
Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Jessica Yu [09/12/15 14:10 -0500]:
>+++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>>On Mon, Nov 30, 2015 at 11:21:17PM -0500, Jessica Yu wrote:
>>>Reuse module loader code to write relocations, thereby eliminating the need
>>>for architecture specific relocation code in livepatch. Namely, we reuse
>>>apply_relocate_add() in the module loader to write relocations instead of
>>>duplicating functionality in livepatch's klp_write_module_reloc(). To apply
>>>relocation sections, remaining SHN_LIVEPATCH symbols referenced by relocs
>>>are resolved and then apply_relocate_add() is called to apply those
>>>relocations.
>>>
>>>In addition, remove x86 livepatch relocation code. It is no longer needed
>>>since symbol resolution and relocation work have been offloaded to module
>>>loader.
>>>
>>>Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>---
>>> arch/x86/include/asm/livepatch.h |  2 -
>>> arch/x86/kernel/Makefile         |  1 -
>>> arch/x86/kernel/livepatch.c      | 91 --------------------------------------
>>> include/linux/livepatch.h        | 30 ++++++-------
>>> include/linux/module.h           |  6 +++
>>> kernel/livepatch/core.c          | 94 ++++++++++++++++++++++------------------
>>> 6 files changed, 70 insertions(+), 154 deletions(-)
>>> delete mode 100644 arch/x86/kernel/livepatch.c
>>>
>>>diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>>>index 19c099a..7312e25 100644
>>>--- a/arch/x86/include/asm/livepatch.h
>>>+++ b/arch/x86/include/asm/livepatch.h
>>>@@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>>> #endif
>>> 	return 0;
>>> }
>>>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>-			   unsigned long loc, unsigned long value);
>>>
>>> static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>> {
>>>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>>>index b1b78ff..c5e9a5c 100644
>>>--- a/arch/x86/kernel/Makefile
>>>+++ b/arch/x86/kernel/Makefile
>>>@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>>> obj-y				+= apic/
>>> obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>>> obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>>>-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>>> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>>> obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>>> obj-$(CONFIG_X86_TSC)		+= trace_clock.o
>>>diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
>>>deleted file mode 100644
>>>index d1d35cc..0000000
>>>--- a/arch/x86/kernel/livepatch.c
>>>+++ /dev/null
>>>@@ -1,91 +0,0 @@
>>>-/*
>>>- * livepatch.c - x86-specific Kernel Live Patching Core
>>>- *
>>>- * Copyright (C) 2014 Seth Jennings <sjenning-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>- * Copyright (C) 2014 SUSE
>>>- *
>>>- * 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/>.
>>>- */
>>>-
>>>-#include <linux/module.h>
>>>-#include <linux/uaccess.h>
>>>-#include <asm/cacheflush.h>
>>>-#include <asm/page_types.h>
>>>-#include <asm/elf.h>
>>>-#include <asm/livepatch.h>
>>>-
>>>-/**
>>>- * klp_write_module_reloc() - write a relocation in a module
>>>- * @mod:	module in which the section to be modified is found
>>>- * @type:	ELF relocation type (see asm/elf.h)
>>>- * @loc:	address that the relocation should be written to
>>>- * @value:	relocation value (sym address + addend)
>>>- *
>>>- * This function writes a relocation to the specified location for
>>>- * a particular module.
>>>- */
>>>-int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>-			   unsigned long loc, unsigned long value)
>>>-{
>>>-	int ret, numpages, size = 4;
>>>-	bool readonly;
>>>-	unsigned long val;
>>>-	unsigned long core = (unsigned long)mod->module_core;
>>>-	unsigned long core_size = mod->core_size;
>>>-
>>>-	switch (type) {
>>>-	case R_X86_64_NONE:
>>>-		return 0;
>>>-	case R_X86_64_64:
>>>-		val = value;
>>>-		size = 8;
>>>-		break;
>>>-	case R_X86_64_32:
>>>-		val = (u32)value;
>>>-		break;
>>>-	case R_X86_64_32S:
>>>-		val = (s32)value;
>>>-		break;
>>>-	case R_X86_64_PC32:
>>>-		val = (u32)(value - loc);
>>>-		break;
>>>-	default:
>>>-		/* unsupported relocation type */
>>>-		return -EINVAL;
>>>-	}
>>>-
>>>-	if (loc < core || loc >= core + core_size)
>>>-		/* loc does not point to any symbol inside the module */
>>>-		return -EINVAL;
>>>-
>>>-	readonly = false;
>>>-
>>>-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
>>>-	if (loc < core + mod->core_ro_size)
>>>-		readonly = true;
>>>-#endif
>>>-
>>>-	/* determine if the relocation spans a page boundary */
>>>-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
>>>-
>>>-	if (readonly)
>>>-		set_memory_rw(loc & PAGE_MASK, numpages);
>>>-
>>>-	ret = probe_kernel_write((void *)loc, &val, size);
>>>-
>>>-	if (readonly)
>>>-		set_memory_ro(loc & PAGE_MASK, numpages);
>>>-
>>>-	return ret;
>>>-}
>>>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>>index 31db7a0..54f62a6 100644
>>>--- a/include/linux/livepatch.h
>>>+++ b/include/linux/livepatch.h
>>>@@ -64,28 +64,21 @@ struct klp_func {
>>> };
>>>
>>> /**
>>>- * struct klp_reloc - relocation structure for live patching
>>>- * @loc:	address where the relocation will be written
>>>- * @val:	address of the referenced symbol (optional,
>>>- *		vmlinux	patches only)
>>>- * @type:	ELF relocation type
>>>- * @name:	name of the referenced symbol (for lookup/verification)
>>>- * @addend:	offset from the referenced symbol
>>>- * @external:	symbol is either exported or within the live patch module itself
>>>+ * struct klp_reloc_sec - relocation section struct for live patching
>>>+ * @index:	Elf section index of the relocation section
>>>+ * @name:	name of the relocation section
>>>+ * @objname:	name of the object associated with the klp reloc section
>>>  */
>>>-struct klp_reloc {
>>>-	unsigned long loc;
>>>-	unsigned long val;
>>>-	unsigned long type;
>>>-	const char *name;
>>>-	int addend;
>>>-	int external;
>>>+struct klp_reloc_sec {
>>>+	unsigned int index;
>>>+	char *name;
>>>+	char *objname;
>>> };
>>>
>>> /**
>>>  * struct klp_object - kernel object structure for live patching
>>>  * @name:	module name (or NULL for vmlinux)
>>>- * @relocs:	relocation entries to be applied at load time
>>>+ * @reloc_secs:	relocation sections to be applied at load time
>>>  * @funcs:	function entries for functions to be patched in the object
>>>  * @kobj:	kobject for sysfs resources
>>>  * @mod:	kernel module associated with the patched object
>>>@@ -95,7 +88,7 @@ struct klp_reloc {
>>> struct klp_object {
>>> 	/* external */
>>> 	const char *name;
>>>-	struct klp_reloc *relocs;
>>>+	struct klp_reloc_sec *reloc_secs;
>>
>>There was a lot of discussion for v1, so I'm not sure, but I thought we
>>ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>>think the symbol table can be walked directly looking for klp rela
>>sections?
>>
>>The benefits of doing so would be to make the interface simpler -- no
>>need to "cache" the section metadata when it's already easily and
>>quickly available in the module struct elf section headers.
>
>Ah, I might have interpreted the conclusions of that last discussion
>incorrectly.
>
>In that case, I think I can just get rid of my klp_for_each_reloc_sec
>macro as well as the lreloc scaffolding code from kpatch. The only
>potentially "ugly" part of this change is that I'll have to move the
>string parsing stuff here to extract the objname of the __klp_rela
>section (which may not actually look that bad, we'll see how that
>turns out).

Turns out the string parsing stuff, even with the help of lib/string.c, doesn't
look very pretty. As I'm working on v3, I'm starting to think having
klp_write_object_relocations() loop simply through all the elf sections might
not be a good idea. Let me explain.

I don't like the amount of string manipulation code that would potentially come
with this change. Even with a string as simple as ".klp.rela.objname", we'll
end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
chop the section name string in place, which I don't think we should do) that
are going to be required at every iteration of the loop, all just to be able to
call strcmp() and see if we're dealing with a klp rela section that belongs to
the object in question. This also leads to more complicated error handling.

In v1, the string parsing was done only *once* for each klp rela section in the
patch module code, and each klp rela section is sorted into their respective
object with the reloc_secs list. Then all klp_write_object_relocations() had to
do is iterate over object->reloc_secs. The tradeoff here is the addition of
another struct (klp_reloc_sec), but that is not nearly as bad as string
parsing, which is imo way more error prone and is unnecessary extra work.

Here's some pseudocode to help visualize the potential issues:

function klp_write_object_relocations(..,struct klp_object *obj,..) {
    for each section in mod->sechdrs { // iterate through all elf sections, no more obj->reloc_secs list
        if not a klp rela section
            continue;
        sec_objname = extract_objname_from_section(section);  // .klp.rela.[objname].sectionname
        if (strcmp(sec_objname, obj->name)) { // this klp rela section doesn't belong to this object
            kfree(sec_objname);
            continue;
        }

      for each rela in this klp rela section {
         sym = symtab + ELF_R_SYM(rela->r_info);
         sym_objname = extract_objname_from_symbol(sym) // symname.klp.[objname]
         if (!sym_objname)
             goto handle_error; // calls kfree(sec_objname);
         symname = extract_symname_from_symbol(sym); // [symname].klp.objname
         if (!symname)
             goto handle_error; // calls kfree(sec_objname);
         ret = klp_find_object_symbol(sym_objname, symname, &addr)
         if (ret)
             goto handle_error2; // calls kfree(symname), then kfree(sec_objname)

...etc., you get the idea how messy that is getting, and we haven't even
reached the call to apply_relocate_add() yet.

So I personally think it is better to keep the old v1 way for applying the klp
reloc secs. The sections are still named ".klp.rela.objname" but the parsing is
done once in the patch module code and used only to build the patch scaffolding
structure.

In order to avoid adding any additional string parsing code in v3, I no longer
thing we should rename livepatch symbols to include the symbol objname (i.e.
'symbol.klp.objname'). Recall that this change is for getting rid of external
dynrelas. Instead of parsing the symbol name, we could just encode 1) the
sympos and 2) the index into the .kpatch.strings strtab (that contains the
sym_objname) in sym->st_value. We define two masks (KLP_MASK_SYMPOS,
KLP_MASK_OBJNAME) that extract these values. st_value has either a 32 bit or 64
bit size, both of which are big enough sizes for the sympos and string table
index. One perk we lose is being able to see which object a symbol belongs to
in readelf, but then again we didn't have this benefit to begin with in
v1 nor v2 (where we didn't know the symbol's object and had to use
klp_find_external_symbol anyway), so I wouldn't say it's a loss.

Apologies for the super long explanations, but I think avoiding string parsing
in livepatch core code will make the relocation code much more succinct and
easier to read. Any objections or thoughts on all this?

Thanks,
Jessica

Re: module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Thu, 10 Dec 2015, Josh Poimboeuf wrote:

> On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> > >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > >>For livepatch modules, copy Elf section, symbol, and string information
> > >>from the load_info struct in the module loader.
> > >>
> > >>Livepatch uses special relocation sections in order to be able to patch
> > >>modules that are not yet loaded, as well as apply patches to the kernel
> > >>when the addresses of symbols cannot be determined at compile time (for
> > >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> > >>information such as section indices in order to apply the remaining
> > >>relocation sections at the appropriate time (i.e. when the target module
> > >>loads).
> > >>
> > >>Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > >>---
> > >> include/linux/module.h |  9 +++++
> > >> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > >> 2 files changed, 105 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/include/linux/module.h b/include/linux/module.h
> > >>index 3a19c79..9b46256 100644
> > >>--- a/include/linux/module.h
> > >>+++ b/include/linux/module.h
> > >>@@ -425,6 +425,14 @@ struct module {
> > >>
> > >> 	/* Notes attributes */
> > >> 	struct module_notes_attrs *notes_attrs;
> > >>+
> > >>+	/* Elf information (optionally saved) */
> > >>+	Elf_Ehdr *hdr;
> > >
> > >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> > >
> > >>+	Elf_Shdr *sechdrs;
> > >>+	char *secstrings;
> > >
> > >Probably a good idea to add underscores to the names ("sec_hdrs" and
> > >"sec_strings") to be consistent with most of the other fields in the
> > >struct.
> > >
> > >>+	struct {
> > >>+		unsigned int sym, str, mod, vers, info, pcpu;
> > >>+	} index;
> > >
> > >I might be contradicting myself from what I said before.  But I'm
> > >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> > >Then below, there could be two versions of copy_module_elf(), the real
> > >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> > >story for free_module_elf().
> > 
> > I think in the v1 discussion we were leaning more towards making this
> > generic. We could potentially just have the Elf module fields
> > available in the generic case, independent of whether CONFIG_LIVEPATCH
> > is set, whereas the mod->klp field should probably be only available
> > when LIVEPATCH is set. I think this makes sense since the Elf fields
> > aren't dependent on livepatch (although livepatch would be the only
> > user of these fields at the moment). I don't know if there would be
> > any users in the future that would be interested in using this Elf
> > information. Thoughts on this?
> 
> IIRC, I think I made the suggestion to always save the elf fields
> because otherwise it was looking like we were going to need a lot of
> spaghetti code.
> 
> But if we can find a way to wrap the elf fields in LIVEPATCH while
> keeping the code simple, then there's no real downside and I think
> that's the way to go.  If somebody else wants to use the fields later,
> then they can remove or change the ifdefs as needed.

I agree with Josh. There is a generic function prepared 
(copy_module_elf()) if someone needs it in the future. But for now we can 
hide everything under CONFIG_LIVEPATCH in the same way layout_symtab() and 
add_kallsyms() are under CONFIG_KALLSYMS.

Miroslav

Re: module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Thu, 10 Dec 2015, Josh Poimboeuf wrote:

> On Wed, Dec 09, 2015 at 03:05:23PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [08/12/15 12:32 -0600]:
> > >On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > >>For livepatch modules, copy Elf section, symbol, and string information
> > >>from the load_info struct in the module loader.
> > >>
> > >>Livepatch uses special relocation sections in order to be able to patch
> > >>modules that are not yet loaded, as well as apply patches to the kernel
> > >>when the addresses of symbols cannot be determined at compile time (for
> > >>example, when kaslr is enabled). Livepatch modules must preserve Elf
> > >>information such as section indices in order to apply the remaining
> > >>relocation sections at the appropriate time (i.e. when the target module
> > >>loads).
> > >>
> > >>Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >>---
> > >> include/linux/module.h |  9 +++++
> > >> kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > >> 2 files changed, 105 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/include/linux/module.h b/include/linux/module.h
> > >>index 3a19c79..9b46256 100644
> > >>--- a/include/linux/module.h
> > >>+++ b/include/linux/module.h
> > >>@@ -425,6 +425,14 @@ struct module {
> > >>
> > >> 	/* Notes attributes */
> > >> 	struct module_notes_attrs *notes_attrs;
> > >>+
> > >>+	/* Elf information (optionally saved) */
> > >>+	Elf_Ehdr *hdr;
> > >
> > >I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> > >
> > >>+	Elf_Shdr *sechdrs;
> > >>+	char *secstrings;
> > >
> > >Probably a good idea to add underscores to the names ("sec_hdrs" and
> > >"sec_strings") to be consistent with most of the other fields in the
> > >struct.
> > >
> > >>+	struct {
> > >>+		unsigned int sym, str, mod, vers, info, pcpu;
> > >>+	} index;
> > >
> > >I might be contradicting myself from what I said before.  But I'm
> > >thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> > >Then below, there could be two versions of copy_module_elf(), the real
> > >one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> > >story for free_module_elf().
> > 
> > I think in the v1 discussion we were leaning more towards making this
> > generic. We could potentially just have the Elf module fields
> > available in the generic case, independent of whether CONFIG_LIVEPATCH
> > is set, whereas the mod->klp field should probably be only available
> > when LIVEPATCH is set. I think this makes sense since the Elf fields
> > aren't dependent on livepatch (although livepatch would be the only
> > user of these fields at the moment). I don't know if there would be
> > any users in the future that would be interested in using this Elf
> > information. Thoughts on this?
> 
> IIRC, I think I made the suggestion to always save the elf fields
> because otherwise it was looking like we were going to need a lot of
> spaghetti code.
> 
> But if we can find a way to wrap the elf fields in LIVEPATCH while
> keeping the code simple, then there's no real downside and I think
> that's the way to go.  If somebody else wants to use the fields later,
> then they can remove or change the ifdefs as needed.

I agree with Josh. There is a generic function prepared 
(copy_module_elf()) if someone needs it in the future. But for now we can 
hide everything under CONFIG_LIVEPATCH in the same way layout_symtab() and 
add_kallsyms() are under CONFIG_KALLSYMS.

Miroslav

Re: [RFC PATCH v2 2/6] module: preserve Elf information for livepatch modules

[Miroslav Benes]

On Mon, 30 Nov 2015, Jessica Yu wrote:

> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto bug_cleanup;
>  
> +	/*
> +	 * Save sechdrs, indices, and other data from info
> +	 * in order to patch to-be-loaded modules.
> +	 * Do not call free_copy() for livepatch modules.
> +	 */
> +	if (mod->klp)
> +		err = copy_module_elf(mod, info);
> +	if (err < 0)
> +		goto bug_cleanup;
> +

I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is 
created. So in case of error here we should call mod_sysfs_teardown() or 
something before going to bug_cleanup. That is new error label is needed 
before bug_cleanup. Correct?

Miroslav

Re: [RFC PATCH v2 3/6] module: s390: keep mod_arch_specific for livepatch modules

[Miroslav Benes]

On Mon, 30 Nov 2015, Jessica Yu wrote:

> Livepatch needs to utilize the symbol information contained in the
> mod_arch_specific struct in order to be able to call the s390
> apply_relocate_add() function to apply relocations. Remove the redundant
> vfree() in module_finalize() since module_arch_freeing_init() (which also frees
> said structures) is called in do_init_module(). Keep a reference to syminfo if
> the module is a livepatch module and free the structures in
> module_arch_cleanup(). If the module isn't a livepatch module, we free the
> structures in module_arch_freeing_init() as usual.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  arch/s390/kernel/module.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index 0c1a679..17a1979 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -51,6 +51,9 @@ void *module_alloc(unsigned long size)
>  
>  void module_arch_freeing_init(struct module *mod)
>  {
> +	if (mod->klp)
> +		return;
> +
>  	vfree(mod->arch.syminfo);
>  	mod->arch.syminfo = NULL;
>  }

Hm, this is problematic. module_arch_freeing_init() is called from 
module_deallocate() and this is called in the error path in load_module(). 
So if there was an error during load_module() of livepatch module which 
led to free_modinfo label or behind mod->arch.syminfo would not be freed 
at all. module_arch_cleanup() is called earlier under 
free_arch_cleanup.

Miroslav

> @@ -420,12 +423,18 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  	return 0;
>  }
>  
> +void module_arch_cleanup(struct module *mod)
> +{
> +	if (mod->klp) {
> +		vfree(mod->arch.syminfo);
> +		mod->arch.syminfo = NULL;
> +	}
> +}
> +
>  int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
>  	jump_label_apply_nops(me);
> -	vfree(me->arch.syminfo);
> -	me->arch.syminfo = NULL;
>  	return 0;
>  }
> -- 
> 2.4.3
> 

Re: livepatch: reuse module loader code to write relocations

[Miroslav Benes]

On Wed, 16 Dec 2015, Jessica Yu wrote:

> +++ Jessica Yu [09/12/15 14:10 -0500]:
> > +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
> > > 
> > > There was a lot of discussion for v1, so I'm not sure, but I thought we
> > > ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
> > > think the symbol table can be walked directly looking for klp rela
> > > sections?
> > > 
> > > The benefits of doing so would be to make the interface simpler -- no
> > > need to "cache" the section metadata when it's already easily and
> > > quickly available in the module struct elf section headers.
> > 
> > Ah, I might have interpreted the conclusions of that last discussion
> > incorrectly.
> > 
> > In that case, I think I can just get rid of my klp_for_each_reloc_sec
> > macro as well as the lreloc scaffolding code from kpatch. The only
> > potentially "ugly" part of this change is that I'll have to move the
> > string parsing stuff here to extract the objname of the __klp_rela
> > section (which may not actually look that bad, we'll see how that
> > turns out).

Yes, that was a conclusion we made. At least I thought so :)

> Turns out the string parsing stuff, even with the help of lib/string.c,
> doesn't
> look very pretty. As I'm working on v3, I'm starting to think having
> klp_write_object_relocations() loop simply through all the elf sections might
> not be a good idea. Let me explain.

Hm, I still think it is worth it...

> I don't like the amount of string manipulation code that would potentially
> come
> with this change. Even with a string as simple as ".klp.rela.objname", we'll
> end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
> chop the section name string in place, which I don't think we should do) that
> are going to be required at every iteration of the loop, all just to be able
> to
> call strcmp() and see if we're dealing with a klp rela section that belongs to
> the object in question. This also leads to more complicated error handling.

But this shouldn't be much different from what init function of the patch 
module does now, right? You have a klp_extract_objname() which I thought 
could be moved right to livepatch code.

> In v1, the string parsing was done only *once* for each klp rela section in
> the
> patch module code, and each klp rela section is sorted into their respective
> object with the reloc_secs list. Then all klp_write_object_relocations() had
> to
> do is iterate over object->reloc_secs. The tradeoff here is the addition of
> another struct (klp_reloc_sec), but that is not nearly as bad as string
> parsing, which is imo way more error prone and is unnecessary extra work.

If it is a problem (and Josh thought it was not if I recall correctly) we 
can cache indices to relevant rela sections in klp_object as we discussed 
in v1.
 
> Here's some pseudocode to help visualize the potential issues:
> 
> function klp_write_object_relocations(..,struct klp_object *obj,..) {
>    for each section in mod->sechdrs { // iterate through all elf sections, no
> more obj->reloc_secs list
>        if not a klp rela section
>            continue;
>        sec_objname = extract_objname_from_section(section);  //
> .klp.rela.[objname].sectionname
>        if (strcmp(sec_objname, obj->name)) { // this klp rela section doesn't
> belong to this object
>            kfree(sec_objname);
>            continue;
>        }
> 
>      for each rela in this klp rela section {
>         sym = symtab + ELF_R_SYM(rela->r_info);
>         sym_objname = extract_objname_from_symbol(sym) //
> symname.klp.[objname]

This is because of 'external' stuff, right?

>         if (!sym_objname)
>             goto handle_error; // calls kfree(sec_objname);
>         symname = extract_symname_from_symbol(sym); // [symname].klp.objname

Can't we use a method from the patch? That is 

symname = pmod->core_strtab + sym->st_name;

>         if (!symname)
>             goto handle_error; // calls kfree(sec_objname);
>         ret = klp_find_object_symbol(sym_objname, symname, &addr)
>         if (ret)
>             goto handle_error2; // calls kfree(symname), then
> kfree(sec_objname)
> 
> ...etc., you get the idea how messy that is getting, and we haven't even
> reached the call to apply_relocate_add() yet.
> 
> So I personally think it is better to keep the old v1 way for applying the klp
> reloc secs. The sections are still named ".klp.rela.objname" but the parsing
> is
> done once in the patch module code and used only to build the patch
> scaffolding
> structure.
> 
> In order to avoid adding any additional string parsing code in v3, I no longer
> thing we should rename livepatch symbols to include the symbol objname (i.e.
> 'symbol.klp.objname'). Recall that this change is for getting rid of external
> dynrelas. Instead of parsing the symbol name, we could just encode 1) the
> sympos and 2) the index into the .kpatch.strings strtab (that contains the
> sym_objname) in sym->st_value. We define two masks (KLP_MASK_SYMPOS,
> KLP_MASK_OBJNAME) that extract these values. st_value has either a 32 bit or
> 64
> bit size, both of which are big enough sizes for the sympos and string table
> index. One perk we lose is being able to see which object a symbol belongs to
> in readelf, but then again we didn't have this benefit to begin with in
> v1 nor v2 (where we didn't know the symbol's object and had to use
> klp_find_external_symbol anyway), so I wouldn't say it's a loss.

Just a note. This would lead to another assumption about a patch 
module... .kpatch.strings. I suspect 'external' symbols are kpatch 
specific (I think I did not deal with it while working on relocations for 
kgraft. But I am not sure now. I haven't finished it too.), but let's try 
to make it "generic".

> Apologies for the super long explanations, but I think avoiding string parsing
> in livepatch core code will make the relocation code much more succinct and
> easier to read. Any objections or thoughts on all this?

My main argument is that the work needs to be done somewhere and there is 
no reason why not do it right in the kernel and not in patch modules. The 
code should be more or less the same and we'd get rid of unnecessary 
layers (klp_reloc_sec) as Josh pointed out.

Miroslav

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Miroslav Benes [16/12/15 13:59 +0100]:
>On Wed, 16 Dec 2015, Jessica Yu wrote:
>
>> +++ Jessica Yu [09/12/15 14:10 -0500]:
>> > +++ Josh Poimboeuf [08/12/15 12:38 -0600]:
>> > >
>> > > There was a lot of discussion for v1, so I'm not sure, but I thought we
>> > > ended up deciding to get rid of the klp_reloc_sec struct?  Instead I
>> > > think the symbol table can be walked directly looking for klp rela
>> > > sections?
>> > >
>> > > The benefits of doing so would be to make the interface simpler -- no
>> > > need to "cache" the section metadata when it's already easily and
>> > > quickly available in the module struct elf section headers.
>> >
>> > Ah, I might have interpreted the conclusions of that last discussion
>> > incorrectly.
>> >
>> > In that case, I think I can just get rid of my klp_for_each_reloc_sec
>> > macro as well as the lreloc scaffolding code from kpatch. The only
>> > potentially "ugly" part of this change is that I'll have to move the
>> > string parsing stuff here to extract the objname of the __klp_rela
>> > section (which may not actually look that bad, we'll see how that
>> > turns out).
>
>Yes, that was a conclusion we made. At least I thought so :)
>
>> Turns out the string parsing stuff, even with the help of lib/string.c,
>> doesn't
>> look very pretty. As I'm working on v3, I'm starting to think having
>> klp_write_object_relocations() loop simply through all the elf sections might
>> not be a good idea. Let me explain.
>
>Hm, I still think it is worth it...
>
>> I don't like the amount of string manipulation code that would potentially
>> come
>> with this change. Even with a string as simple as ".klp.rela.objname", we'll
>> end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
>> chop the section name string in place, which I don't think we should do) that
>> are going to be required at every iteration of the loop, all just to be able
>> to
>> call strcmp() and see if we're dealing with a klp rela section that belongs to
>> the object in question. This also leads to more complicated error handling.
>
>But this shouldn't be much different from what init function of the patch
>module does now, right? You have a klp_extract_objname() which I thought
>could be moved right to livepatch code.

First, thanks for the comments. I think my only real gripe is that, in the
pseudo code I provided, we are not only doing the for loop for each object, but
also a bunch of kmalloc's/kstrdup's and kfree's just to be able to check if
this klp section belongs to the current object, *and* also kmallocing/kfreeing
just to provide buffers to extract parts of a symbol's name. In the patch
module code, each klp section was parsed just once and sorted by object:
https://github.com/flaming-toast/kpatch/blob/no_dynrela_redux/kmod/patch/livepatch-patch-hook.c#L213
^^^ This was before we decided to get rid of 'external', so only klp sections
had to be parsed. In v3, we are also looking to parse the symbol name (to
eliminate 'external'), so now I'm just trying to reduce all the string stuff to
the bare minimum needed to have this work.

function klp_write_object_relocations(..,struct klp_object *obj,..) {
   for each section in mod->sechdrs { // iterate through all elf sections, no more obj->reloc_secs list
       if not a klp rela section
           continue;
->     sec_objname = extract_objname_from_section(section);  // .klp.rela.[objname].sectionname
->     if (strcmp(sec_objname, obj->name)) { // this klp rela section doesn't belong to this object
->         kfree(sec_objname);
->         continue;
       }

     for each rela in this klp rela section {
        sym = symtab + ELF_R_SYM(rela->r_info);
->      sym_objname = extract_objname_from_symbol(sym) // symname.klp.[objname]
->      if (!sym_objname)
->           goto handle_error; // calls kfree(sec_objname);
->      symname = extract_symname_from_symbol(sym); // [symname].klp.objname
->      if (!symname)
->          goto handle_error; // calls kfree(sec_objname);
        ret = klp_find_object_symbol(sym_objname, symname, &addr)
        if (ret)
->          goto handle_error2; // calls kfree(symname), then kfree(sec_objname)
        sym->st_value = addr;
->      if (sym_objname) // free buffers before proceeding to next iteration of loop
->         kfree(sym_objname);
->      kfree(symname);
     }
[ snipped rest of function ]

I'm trying to figure out a way to simplify all the lines marked with '->', and
make this code as readable and palatable as possible. I think primarily the
kfree's and multiple error-handling goto's bothered me, which is why I
suggested putting the symname.klp.[objname] part in .kpatch.strings and using
those masks I suggested. Then we wouldn't even need to set-up temporary buffers to
hold the sym_objname + symname strings. You're right that this would require
another assumption about the patch module, but I think having the names of
livepatch symbols formatted as 'symname.klp.objname' was already another
assumption, and using .kpatch.strings would just be changing this assumption.

>> In v1, the string parsing was done only *once* for each klp rela section in
>> the
>> patch module code, and each klp rela section is sorted into their respective
>> object with the reloc_secs list. Then all klp_write_object_relocations() had
>> to
>> do is iterate over object->reloc_secs. The tradeoff here is the addition of
>> another struct (klp_reloc_sec), but that is not nearly as bad as string
>> parsing, which is imo way more error prone and is unnecessary extra work.
>
>If it is a problem (and Josh thought it was not if I recall correctly) we
>can cache indices to relevant rela sections in klp_object as we discussed
>in v1.
>
>> Here's some pseudocode to help visualize the potential issues:
>>
>> function klp_write_object_relocations(..,struct klp_object *obj,..) {
>>    for each section in mod->sechdrs { // iterate through all elf sections, no
>> more obj->reloc_secs list
>>        if not a klp rela section
>>            continue;
>>        sec_objname = extract_objname_from_section(section);  //
>> .klp.rela.[objname].sectionname
>>        if (strcmp(sec_objname, obj->name)) { // this klp rela section doesn't
>> belong to this object
>>            kfree(sec_objname);
>>            continue;
>>        }
>>
>>      for each rela in this klp rela section {
>>         sym = symtab + ELF_R_SYM(rela->r_info);
>>         sym_objname = extract_objname_from_symbol(sym) //
>> symname.klp.[objname]
>
>This is because of 'external' stuff, right?

Correct, in the v2 discussion we decided symbol names can have the
names of their objects attached, which will eliminate the need for the
external flag and klp_find_external_symbol().

>>         if (!sym_objname)
>>             goto handle_error; // calls kfree(sec_objname);
>>         symname = extract_symname_from_symbol(sym); // [symname].klp.objname
>
>Can't we use a method from the patch? That is
>
>symname = pmod->core_strtab + sym->st_name;

Since we are removing 'external', we decided to append the objname to the
symbol name. But then extra string parsing is needed to get the real symbol
name. So, pmod->core_strtab + sym->st_name would correspond to the full name,
formatted as 'symname.klp.objname'. We parse to get the real symname and the
objname. Then we no longer need the klp_find_external_symbol() call, it'd just
be one call to klp_find_object_symbol(sym_objname, symname, addr)

>>         if (!symname)
>>             goto handle_error; // calls kfree(sec_objname);
>>         ret = klp_find_object_symbol(sym_objname, symname, &addr)
>>         if (ret)
>>             goto handle_error2; // calls kfree(symname), then
>> kfree(sec_objname)
>>
>> ...etc., you get the idea how messy that is getting, and we haven't even
>> reached the call to apply_relocate_add() yet.
>>
>> So I personally think it is better to keep the old v1 way for applying the klp
>> reloc secs. The sections are still named ".klp.rela.objname" but the parsing
>> is
>> done once in the patch module code and used only to build the patch
>> scaffolding
>> structure.
>>
>> In order to avoid adding any additional string parsing code in v3, I no longer
>> thing we should rename livepatch symbols to include the symbol objname (i.e.
>> 'symbol.klp.objname'). Recall that this change is for getting rid of external
>> dynrelas. Instead of parsing the symbol name, we could just encode 1) the
>> sympos and 2) the index into the .kpatch.strings strtab (that contains the
>> sym_objname) in sym->st_value. We define two masks (KLP_MASK_SYMPOS,
>> KLP_MASK_OBJNAME) that extract these values. st_value has either a 32 bit or
>> 64
>> bit size, both of which are big enough sizes for the sympos and string table
>> index. One perk we lose is being able to see which object a symbol belongs to
>> in readelf, but then again we didn't have this benefit to begin with in
>> v1 nor v2 (where we didn't know the symbol's object and had to use
>> klp_find_external_symbol anyway), so I wouldn't say it's a loss.
>
>Just a note. This would lead to another assumption about a patch
>module... .kpatch.strings. I suspect 'external' symbols are kpatch
>specific (I think I did not deal with it while working on relocations for
>kgraft. But I am not sure now. I haven't finished it too.), but let's try
>to make it "generic".

I think 'external' is kpatch-specific (correct me if  I'm wrong), as I
explained above we're trying to get rid of it.

>> Apologies for the super long explanations, but I think avoiding string parsing
>> in livepatch core code will make the relocation code much more succinct and
>> easier to read. Any objections or thoughts on all this?
>
>My main argument is that the work needs to be done somewhere and there is
>no reason why not do it right in the kernel and not in patch modules. The
>code should be more or less the same and we'd get rid of unnecessary
>layers (klp_reloc_sec) as Josh pointed out.

Right, OK. I agree that this work has to be done somewhere, I am just hesitant
about the new string handling code, as I pointed out above. If people are OK and
agree that this is the better approach, then I will keep it.

Thanks again,
Jessica

Re: module: s390: keep mod_arch_specific for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [16/12/15 13:02 +0100]:
>On Mon, 30 Nov 2015, Jessica Yu wrote:
>
>> Livepatch needs to utilize the symbol information contained in the
>> mod_arch_specific struct in order to be able to call the s390
>> apply_relocate_add() function to apply relocations. Remove the redundant
>> vfree() in module_finalize() since module_arch_freeing_init() (which also frees
>> said structures) is called in do_init_module(). Keep a reference to syminfo if
>> the module is a livepatch module and free the structures in
>> module_arch_cleanup(). If the module isn't a livepatch module, we free the
>> structures in module_arch_freeing_init() as usual.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  arch/s390/kernel/module.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
>> index 0c1a679..17a1979 100644
>> --- a/arch/s390/kernel/module.c
>> +++ b/arch/s390/kernel/module.c
>> @@ -51,6 +51,9 @@ void *module_alloc(unsigned long size)
>>
>>  void module_arch_freeing_init(struct module *mod)
>>  {
>> +	if (mod->klp)
>> +		return;
>> +
>>  	vfree(mod->arch.syminfo);
>>  	mod->arch.syminfo = NULL;
>>  }
>
>Hm, this is problematic. module_arch_freeing_init() is called from
>module_deallocate() and this is called in the error path in load_module().
>So if there was an error during load_module() of livepatch module which
>led to free_modinfo label or behind mod->arch.syminfo would not be freed
>at all. module_arch_cleanup() is called earlier under
>free_arch_cleanup.

Gah. Good catch. How about we add an additional check to see if
mod->state is MODULE_STATE_LIVE? If so, we can return. This means
we're coming from do_init_module(). If module_arch_freeing_init() was
called and the module was in a state other than MODULE_STATE_LIVE
(UNFORMED, GOING, COMING), we know we need to free. Think that would
work?

Jessica

Re: module: s390: keep mod_arch_specific for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [16/12/15 13:02 +0100]:
>On Mon, 30 Nov 2015, Jessica Yu wrote:
>
>> Livepatch needs to utilize the symbol information contained in the
>> mod_arch_specific struct in order to be able to call the s390
>> apply_relocate_add() function to apply relocations. Remove the redundant
>> vfree() in module_finalize() since module_arch_freeing_init() (which also frees
>> said structures) is called in do_init_module(). Keep a reference to syminfo if
>> the module is a livepatch module and free the structures in
>> module_arch_cleanup(). If the module isn't a livepatch module, we free the
>> structures in module_arch_freeing_init() as usual.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/s390/kernel/module.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
>> index 0c1a679..17a1979 100644
>> --- a/arch/s390/kernel/module.c
>> +++ b/arch/s390/kernel/module.c
>> @@ -51,6 +51,9 @@ void *module_alloc(unsigned long size)
>>
>>  void module_arch_freeing_init(struct module *mod)
>>  {
>> +	if (mod->klp)
>> +		return;
>> +
>>  	vfree(mod->arch.syminfo);
>>  	mod->arch.syminfo = NULL;
>>  }
>
>Hm, this is problematic. module_arch_freeing_init() is called from
>module_deallocate() and this is called in the error path in load_module().
>So if there was an error during load_module() of livepatch module which
>led to free_modinfo label or behind mod->arch.syminfo would not be freed
>at all. module_arch_cleanup() is called earlier under
>free_arch_cleanup.

Gah. Good catch. How about we add an additional check to see if
mod->state is MODULE_STATE_LIVE? If so, we can return. This means
we're coming from do_init_module(). If module_arch_freeing_init() was
called and the module was in a state other than MODULE_STATE_LIVE
(UNFORMED, GOING, COMING), we know we need to free. Think that would
work?

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [16/12/15 11:58 +0100]:
>On Mon, 30 Nov 2015, Jessica Yu wrote:
>
>> @@ -3530,6 +3614,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>  	if (err < 0)
>>  		goto bug_cleanup;
>>
>> +	/*
>> +	 * Save sechdrs, indices, and other data from info
>> +	 * in order to patch to-be-loaded modules.
>> +	 * Do not call free_copy() for livepatch modules.
>> +	 */
>> +	if (mod->klp)
>> +		err = copy_module_elf(mod, info);
>> +	if (err < 0)
>> +		goto bug_cleanup;
>> +
>
>I think goto bug_cleanup is not sufficient. Just before this hunk sysfs is
>created. So in case of error here we should call mod_sysfs_teardown() or
>something before going to bug_cleanup. That is new error label is needed
>before bug_cleanup. Correct?

Yes I think you're right. So before bug_cleanup, we'll just need a new
label (maybe called sysfs_cleanup) that calls mod_sysfs_teardown(). In
addition, the if (err < 0) check should have been enclosed in the if
(mod->klp) block.

Jessica

Re: module: s390: keep mod_arch_specific for livepatch modules

[Miroslav Benes]

On Wed, 16 Dec 2015, Jessica Yu wrote:

> +++ Miroslav Benes [16/12/15 13:02 +0100]:
> > On Mon, 30 Nov 2015, Jessica Yu wrote:
> > 
> > > Livepatch needs to utilize the symbol information contained in the
> > > mod_arch_specific struct in order to be able to call the s390
> > > apply_relocate_add() function to apply relocations. Remove the redundant
> > > vfree() in module_finalize() since module_arch_freeing_init() (which also
> > > frees
> > > said structures) is called in do_init_module(). Keep a reference to
> > > syminfo if
> > > the module is a livepatch module and free the structures in
> > > module_arch_cleanup(). If the module isn't a livepatch module, we free the
> > > structures in module_arch_freeing_init() as usual.
> > > 
> > > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > > ---
> > >  arch/s390/kernel/module.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> > > index 0c1a679..17a1979 100644
> > > --- a/arch/s390/kernel/module.c
> > > +++ b/arch/s390/kernel/module.c
> > > @@ -51,6 +51,9 @@ void *module_alloc(unsigned long size)
> > > 
> > >  void module_arch_freeing_init(struct module *mod)
> > >  {
> > > +	if (mod->klp)
> > > +		return;
> > > +
> > >  	vfree(mod->arch.syminfo);
> > >  	mod->arch.syminfo = NULL;
> > >  }
> > 
> > Hm, this is problematic. module_arch_freeing_init() is called from
> > module_deallocate() and this is called in the error path in load_module().
> > So if there was an error during load_module() of livepatch module which
> > led to free_modinfo label or behind mod->arch.syminfo would not be freed
> > at all. module_arch_cleanup() is called earlier under
> > free_arch_cleanup.
> 
> Gah. Good catch. How about we add an additional check to see if
> mod->state is MODULE_STATE_LIVE? If so, we can return. This means
> we're coming from do_init_module(). If module_arch_freeing_init() was
> called and the module was in a state other than MODULE_STATE_LIVE
> (UNFORMED, GOING, COMING), we know we need to free. Think that would
> work?

It would I think. module_arch_freeing_init is called thrice.

1. in free_module(). The module is in UNFORMED state so everything is 
fine.

2. in module_deallocate(). The module is not LIVE yet here so ok as well.

3. in do_init_module() and the module is LIVE there.

So yes, you are right, that should work.

Miroslav

Re: livepatch: reuse module loader code to write relocations

[Petr Mladek]

On Wed 2015-12-16 00:40:48, Jessica Yu wrote:
> Turns out the string parsing stuff, even with the help of lib/string.c, doesn't
> look very pretty. As I'm working on v3, I'm starting to think having
> klp_write_object_relocations() loop simply through all the elf sections might
> not be a good idea. Let me explain.
> 
> I don't like the amount of string manipulation code that would potentially come
> with this change. Even with a string as simple as ".klp.rela.objname", we'll
> end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
> chop the section name string in place, which I don't think we should do) that
> are going to be required at every iteration of the loop, all just to be able to
> call strcmp() and see if we're dealing with a klp rela section that belongs to
> the object in question. This also leads to more complicated error handling.

I do not think that we need to allocate and free buffers every time
we compare a substring.

One possibility is to find the position of the substring using
strchr(). Then you could compare it using strncmp() and pass there
the pointer where the substring begins.

Alternatively we could use a static buffer or allocate a big enough
one at the beginning. IMHO, we could assume that function name must
fit into 80 characters, otherwise, that code would be unmanageable
(won't deserve patching). If we use a buffer size 256 bytes,
we should be definitely on the safe side.

Just 2 my cents,
Petr

Re: livepatch: reuse module loader code to write relocations

[Petr Mladek]

On Wed 2015-12-16 00:40:48, Jessica Yu wrote:
> Turns out the string parsing stuff, even with the help of lib/string.c, doesn't
> look very pretty. As I'm working on v3, I'm starting to think having
> klp_write_object_relocations() loop simply through all the elf sections might
> not be a good idea. Let me explain.
> 
> I don't like the amount of string manipulation code that would potentially come
> with this change. Even with a string as simple as ".klp.rela.objname", we'll
> end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
> chop the section name string in place, which I don't think we should do) that
> are going to be required at every iteration of the loop, all just to be able to
> call strcmp() and see if we're dealing with a klp rela section that belongs to
> the object in question. This also leads to more complicated error handling.

I do not think that we need to allocate and free buffers every time
we compare a substring.

One possibility is to find the position of the substring using
strchr(). Then you could compare it using strncmp() and pass there
the pointer where the substring begins.

Alternatively we could use a static buffer or allocate a big enough
one at the beginning. IMHO, we could assume that function name must
fit into 80 characters, otherwise, that code would be unmanageable
(won't deserve patching). If we use a buffer size 256 bytes,
we should be definitely on the safe side.

Just 2 my cents,
Petr

Re: [RFC PATCH v2 2/6] module: preserve Elf information for livepatch modules

[Petr Mladek]

On Mon 2015-11-30 23:21:15, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader.
> 
> Livepatch uses special relocation sections in order to be able to patch
> modules that are not yet loaded, as well as apply patches to the kernel
> when the addresses of symbols cannot be determined at compile time (for
> example, when kaslr is enabled). Livepatch modules must preserve Elf
> information such as section indices in order to apply the remaining
> relocation sections at the appropriate time (i.e. when the target module
> loads).
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/module.h |  9 +++++
>  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79..9b46256 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -425,6 +425,14 @@ struct module {
>  
>  	/* Notes attributes */
>  	struct module_notes_attrs *notes_attrs;
> +
> +	/* Elf information (optionally saved) */
> +	Elf_Ehdr *hdr;
> +	Elf_Shdr *sechdrs;
> +	char *secstrings;
> +	struct {
> +		unsigned int sym, str, mod, vers, info, pcpu;
> +	} index;
>  #endif

I would hide this into a structure. It is 3 pointers and 6 integers
that are mostly unused. I think about using a pointer to struct load_info
here. We could set the unused stuff to zero and NULL.

Any better idea how to share the definition with struct load_info
is welcome.

Best Regards,
Petr

Re: [RFC PATCH v2 2/6] module: preserve Elf information for livepatch modules

[Petr Mladek]

On Tue 2015-12-08 12:32:12, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:21:15PM -0500, Jessica Yu wrote:
> > For livepatch modules, copy Elf section, symbol, and string information
> > from the load_info struct in the module loader.
> > 
> > Livepatch uses special relocation sections in order to be able to patch
> > modules that are not yet loaded, as well as apply patches to the kernel
> > when the addresses of symbols cannot be determined at compile time (for
> > example, when kaslr is enabled). Livepatch modules must preserve Elf
> > information such as section indices in order to apply the remaining
> > relocation sections at the appropriate time (i.e. when the target module
> > loads).
> > 
> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > ---
> >  include/linux/module.h |  9 +++++
> >  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 105 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 3a19c79..9b46256 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -425,6 +425,14 @@ struct module {
> >  
> >  	/* Notes attributes */
> >  	struct module_notes_attrs *notes_attrs;
> > +
> > +	/* Elf information (optionally saved) */
> > +	Elf_Ehdr *hdr;
> 
> I would rename "hdr" to "elf_hdr" to make its purpose clearer.
> 
> > +	Elf_Shdr *sechdrs;
> > +	char *secstrings;
> 
> Probably a good idea to add underscores to the names ("sec_hdrs" and
> "sec_strings") to be consistent with most of the other fields in the
> struct.

We should keep the names in sync with struct load_info.

> > +	struct {
> > +		unsigned int sym, str, mod, vers, info, pcpu;
> > +	} index;
> 
> I might be contradicting myself from what I said before.  But I'm
> thinking we should put all these fields inside a CONFIG_LIVEPATCH ifdef.
> Then below, there could be two versions of copy_module_elf(), the real
> one for LIVEPATCH and and an empty one for !LIVEPATCH.  And the same
> story for free_module_elf().

Yes, I would hide these fields for !LIVEPATCH.


Best Regards,
Petr

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Petr Mladek [17/12/15 17:26 +0100]:
>On Mon 2015-11-30 23:21:15, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader.
>>
>> Livepatch uses special relocation sections in order to be able to patch
>> modules that are not yet loaded, as well as apply patches to the kernel
>> when the addresses of symbols cannot be determined at compile time (for
>> example, when kaslr is enabled). Livepatch modules must preserve Elf
>> information such as section indices in order to apply the remaining
>> relocation sections at the appropriate time (i.e. when the target module
>> loads).
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  include/linux/module.h |  9 +++++
>>  kernel/module.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 105 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 3a19c79..9b46256 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -425,6 +425,14 @@ struct module {
>>
>>  	/* Notes attributes */
>>  	struct module_notes_attrs *notes_attrs;
>> +
>> +	/* Elf information (optionally saved) */
>> +	Elf_Ehdr *hdr;
>> +	Elf_Shdr *sechdrs;
>> +	char *secstrings;
>> +	struct {
>> +		unsigned int sym, str, mod, vers, info, pcpu;
>> +	} index;
>>  #endif
>
>I would hide this into a structure. It is 3 pointers and 6 integers
>that are mostly unused. I think about using a pointer to struct load_info
>here. We could set the unused stuff to zero and NULL.
>
>Any better idea how to share the definition with struct load_info
>is welcome.

Sure. If we want to encapsulate all this information, we can perhaps
replace this with a single pointer to a copy of load_info, and maybe
move the struct definition of load_info to module.h.

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Petr Mladek [17/12/15 16:45 +0100]:
>On Wed 2015-12-16 00:40:48, Jessica Yu wrote:
>> Turns out the string parsing stuff, even with the help of lib/string.c, doesn't
>> look very pretty. As I'm working on v3, I'm starting to think having
>> klp_write_object_relocations() loop simply through all the elf sections might
>> not be a good idea. Let me explain.
>>
>> I don't like the amount of string manipulation code that would potentially come
>> with this change. Even with a string as simple as ".klp.rela.objname", we'll
>> end up with a bunch of kstrdup's/kmalloc's and kfree's (unless we modify and
>> chop the section name string in place, which I don't think we should do) that
>> are going to be required at every iteration of the loop, all just to be able to
>> call strcmp() and see if we're dealing with a klp rela section that belongs to
>> the object in question. This also leads to more complicated error handling.
>
>I do not think that we need to allocate and free buffers every time
>we compare a substring.
>
>One possibility is to find the position of the substring using
>strchr(). Then you could compare it using strncmp() and pass there
>the pointer where the substring begins.

Hm, yes you're right. Specifically, it looks like strcspn() would also
be useful for this situation (i.e. calculate the length of a substring
that does not contain certain characters); combined with strncmp(),
this should make the string code much simpler, and no more buffer
allocating/freeing. :-)

Jessica