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

[RFC PATCH v3 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.rel. 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. These ELF constants
were selected from OS-specific ranges according to the definitions from
glibc.

Based on the following branch:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git
  for-4.5/core

v3:
 - Remove usage of the klp_reloc_sec struct, since we can simply loop
   through the patch module's section headers.
 - Remove necessity of the "external" flag by prefixing symbol names
   with the object name and extracting this name during symbol
   resolution.
 - Create CONFIG_LIVEPATCH and !CONFIG_LIVEPATCH versions of
   {copy,free}_module_elf(), is_livepatch_module(), and
   check_livepatch_modinfo().
 - Encoded symbol position of a livepatch sym in its st_other field.
 - Various bug fixes from v2

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: mark as livepatch module
  Documentation: livepatch: outline the Elf format of a livepatch module

 Documentation/livepatch/patch-module-format.txt |  98 ++++++++++++++++
 arch/s390/kernel/module.c                       |  16 ++-
 arch/x86/include/asm/livepatch.h                |   2 -
 arch/x86/kernel/Makefile                        |   1 -
 arch/x86/kernel/livepatch.c                     |  70 -----------
 include/linux/livepatch.h                       |  33 +++---
 include/linux/module.h                          |  31 +++++
 include/uapi/linux/elf.h                        |  10 +-
 kernel/livepatch/core.c                         | 104 ++++++++---------
 kernel/module.c                                 | 149 +++++++++++++++++++++---
 samples/livepatch/livepatch-sample.c            |   1 +
 11 files changed, 347 insertions(+), 168 deletions(-)
 create mode 100644 Documentation/livepatch/patch-module-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

[RFC PATCH v3 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.rel. 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. These ELF constants
were selected from OS-specific ranges according to the definitions from
glibc.

Based on the following branch:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git
  for-4.5/core

v3:
 - Remove usage of the klp_reloc_sec struct, since we can simply loop
   through the patch module's section headers.
 - Remove necessity of the "external" flag by prefixing symbol names
   with the object name and extracting this name during symbol
   resolution.
 - Create CONFIG_LIVEPATCH and !CONFIG_LIVEPATCH versions of
   {copy,free}_module_elf(), is_livepatch_module(), and
   check_livepatch_modinfo().
 - Encoded symbol position of a livepatch sym in its st_other field.
 - Various bug fixes from v2

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: mark as livepatch module
  Documentation: livepatch: outline the Elf format of a livepatch module

 Documentation/livepatch/patch-module-format.txt |  98 ++++++++++++++++
 arch/s390/kernel/module.c                       |  16 ++-
 arch/x86/include/asm/livepatch.h                |   2 -
 arch/x86/kernel/Makefile                        |   1 -
 arch/x86/kernel/livepatch.c                     |  70 -----------
 include/linux/livepatch.h                       |  33 +++---
 include/linux/module.h                          |  31 +++++
 include/uapi/linux/elf.h                        |  10 +-
 kernel/livepatch/core.c                         | 104 ++++++++---------
 kernel/module.c                                 | 149 +++++++++++++++++++++---
 samples/livepatch/livepatch-sample.c            |   1 +
 11 files changed, 347 insertions(+), 168 deletions(-)
 create mode 100644 Documentation/livepatch/patch-module-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

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

[Jessica Yu]

Add livepatch Elf relocation section flag (SHF_RELA_LIVEPATCH), and symbol
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.

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

diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 71e1d0e..cb4a72f 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -282,16 +282,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 v3 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 |  31 ++++++++++
 kernel/module.c        | 149 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 164 insertions(+), 16 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 4560d8f..ba6052d 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -281,6 +281,21 @@ void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
 #define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x))))
 
+struct load_info {
+	Elf_Ehdr *hdr;
+	unsigned long len;
+	Elf_Shdr *sechdrs;
+	char *secstrings, *strtab;
+	unsigned long symoffs, stroffs;
+	struct _ddebug *debug;
+	unsigned int num_debug;
+	bool sig_ok;
+	struct {
+		unsigned int sym, str, mod, vers, info, pcpu;
+	} index;
+};
+
+
 /* modules using other modules: kdb wants to see this. */
 struct module_use {
 	struct list_head source_list;
@@ -455,7 +470,11 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+	bool klp; /* Is this a livepatch module? */
 	bool klp_alive;
+
+	/* Elf information */
+	struct load_info *info;
 #endif
 
 #ifdef CONFIG_MODULE_UNLOAD
@@ -798,4 +817,16 @@ static inline bool module_sig_ok(struct module *module)
 }
 #endif	/* CONFIG_MODULE_SIG */
 
+#ifdef CONFIG_LIVEPATCH
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return mod->klp;
+}
+#else
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return false;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 #endif /* _LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 912e891..3d14e36 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -294,20 +294,6 @@ int unregister_module_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
-struct load_info {
-	Elf_Ehdr *hdr;
-	unsigned long len;
-	Elf_Shdr *sechdrs;
-	char *secstrings, *strtab;
-	unsigned long symoffs, stroffs;
-	struct _ddebug *debug;
-	unsigned int num_debug;
-	bool sig_ok;
-	struct {
-		unsigned int sym, str, mod, vers, info, pcpu;
-	} index;
-};
-
 /* We require a truly strong try_module_get(): 0 means failure due to
    ongoing or failed initialization etc. */
 static inline int strong_try_module_get(struct module *mod)
@@ -1966,6 +1952,89 @@ static void module_enable_nx(const struct module *mod) { }
 static void module_disable_nx(const struct module *mod) { }
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+/*
+ * copy_module_elf - preserve Elf information about a module
+ *
+ * Copy relevant Elf information from the load_info struct.
+ * Note: not all fields from the original load_info are
+ * copied into mod->info.
+ */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	unsigned int size;
+	int ret = 0;
+
+	mod->info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (mod->info == NULL)
+		return -ENOMEM;
+
+	/* Elf header */
+	size = sizeof(Elf_Ehdr);
+	mod->info->hdr = kmalloc(size, GFP_KERNEL);
+	if (mod->info->hdr == NULL) {
+		ret = -ENOMEM;
+		goto free_info;
+	}
+	memcpy(mod->info->hdr, info->hdr, size);
+
+	/* Elf section header table */
+	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
+	mod->info->sechdrs = kmalloc(size, GFP_KERNEL);
+	if (mod->info->sechdrs == NULL) {
+		ret = -ENOMEM;
+		goto free_hdr;
+	}
+	memcpy(mod->info->sechdrs, info->sechdrs, size);
+
+	/* Elf section name string table */
+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+	mod->info->secstrings = kmalloc(size, GFP_KERNEL);
+	if (mod->info->secstrings == NULL) {
+		ret = -ENOMEM;
+		goto free_sechdrs;
+	}
+	memcpy(mod->info->secstrings, info->secstrings, size);
+
+	/* Elf section indices */
+	memcpy(&mod->info->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
+	 */
+	mod->info->sechdrs[mod->info->index.sym].sh_addr = (unsigned long)mod->core_symtab;
+
+	return ret;
+
+free_sechdrs:
+	kfree(mod->info->sechdrs);
+free_hdr:
+	kfree(mod->info->hdr);
+free_info:
+	kfree(mod->info);
+	return ret;
+}
+
+static void free_module_elf(struct module *mod)
+{
+	kfree(mod->info->sechdrs);
+	kfree(mod->info->hdr);
+	kfree(mod->info->secstrings);
+	kfree(mod->info);
+}
+#else /* !CONFIG_LIVEPATCH */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	return 0;
+}
+
+static void free_module_elf(struct module *mod)
+{
+}
+#endif /* CONFIG_LIVEPATCH */
+
 void __weak module_memfree(void *module_region)
 {
 	vfree(module_region);
@@ -2004,6 +2073,10 @@ 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 */
+	if (is_livepatch_module(mod))
+		free_module_elf(mod);
+
 	/* Now we can delete it from the lists */
 	mutex_lock(&module_mutex);
 	/* Unlink carefully: kallsyms could be walking list. */
@@ -2119,6 +2192,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.  */
@@ -2167,6 +2244,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);
@@ -2385,6 +2466,8 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
 		return 'U';
 	if (sym->st_shndx == SHN_ABS || sym->st_shndx == info->index.pcpu)
 		return 'a';
+	if (sym->st_shndx == SHN_LIVEPATCH)
+		return 'k';
 	if (sym->st_shndx >= SHN_LORESERVE)
 		return '?';
 	if (sechdrs[sym->st_shndx].sh_flags & SHF_EXECINSTR)
@@ -2462,7 +2545,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 || is_livepatch_module(mod) ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			strtab_size += strlen(&info->strtab[src[i].st_name])+1;
@@ -2505,7 +2588,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
 	src = mod->symtab;
 	for (ndst = i = 0; i < mod->num_symtab; i++) {
-		if (i == 0 ||
+		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			dst[ndst] = src[i];
@@ -2672,6 +2755,24 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 	return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	mod->klp = get_modinfo(info, "livepatch") ? true : false;
+
+	return 0;
+}
+#else
+static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	if (get_modinfo(info, "livepatch"))
+		return -EINVAL;
+
+	return 0;
+}
+#endif
+
+
 /* Sets info->hdr and info->len. */
 static int copy_module_from_fd(int fd, struct load_info *info)
 {
@@ -2855,6 +2956,10 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+	err = check_livepatch_modinfo(mod, info);
+	if (err)
+		return -ENOEXEC;
+
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
 
@@ -3511,6 +3616,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
+	 * load_info for livepatch modules
+	 */
+	if (is_livepatch_module(mod)) {
+		err = copy_module_elf(mod, info);
+		if (err < 0)
+			goto sysfs_cleanup;
+	}
+
 	/* Get rid of temporary copy. */
 	free_copy(info);
 
@@ -3519,6 +3634,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ sysfs_cleanup:
+	mod_sysfs_teardown(mod);
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */
 	mutex_lock(&module_mutex);
-- 
2.4.3

[RFC PATCH v3 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. Keep a reference to
syminfo if the module is a livepatch module. Remove the redundant vfree()
in module_finalize() since module_arch_freeing_init() (which also frees
those structures) is called in do_init_module(). 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index 7873e17..fbc0789 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -51,6 +51,10 @@ void *module_alloc(unsigned long size)
 
 void module_arch_freeing_init(struct module *mod)
 {
+	if (is_livepatch_module(mod) &&
+	    mod->state == MODULE_STATE_LIVE)
+		return;
+
 	vfree(mod->arch.syminfo);
 	mod->arch.syminfo = NULL;
 }
@@ -425,7 +429,5 @@ int module_finalize(const Elf_Ehdr *hdr,
 		    struct module *me)
 {
 	jump_label_apply_nops(me);
-	vfree(me->arch.syminfo);
-	me->arch.syminfo = NULL;
 	return 0;
 }
-- 
2.4.3

[RFC PATCH v3 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      |  70 ---------------------------
 include/linux/livepatch.h        |  33 +++++--------
 kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,70 +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/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)
-{
-	size_t size = 4;
-	unsigned long val;
-	unsigned long core = (unsigned long)mod->core_layout.base;
-	unsigned long core_size = mod->core_layout.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;
-
-	return probe_kernel_write((void *)loc, &val, size);
-}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a882865..2f12ce7 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -65,27 +65,8 @@ struct klp_func {
 };
 
 /**
- * struct klp_reloc - relocation structure for live patching
- * @loc:	address where the relocation will be written
- * @sympos:	position in kallsyms to disambiguate symbols (optional)
- * @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 {
-	unsigned long loc;
-	unsigned long sympos;
-	unsigned long type;
-	const char *name;
-	int addend;
-	int external;
-};
-
-/**
  * 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
  * @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 +76,6 @@ struct klp_reloc {
 struct klp_object {
 	/* external */
 	const char *name;
-	struct klp_reloc *relocs;
 	struct klp_func *funcs;
 
 	/* internal */
@@ -123,6 +103,19 @@ struct klp_patch {
 	enum klp_state state;
 };
 
+/*
+ * Livepatch-specific symbols and relocation
+ * sections are prefixed with a tag:
+ * .klp.rel. for relocation sections
+ * .klp.sym. for livepatch symbols
+ */
+#define KLP_TAG_LEN 9
+/*
+ * Livepatch-specific bits for specifying symbol
+ * positions in the Elf_Sym st_other field
+ */
+#define KLP_SYMPOS(o) (o >> 2) & 0xff
+
 #define klp_for_each_object(patch, obj) \
 	for (obj = patch->objs; obj->funcs; obj++)
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc2c85c..64536a4 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 <linux/string.h>
+#include <linux/moduleloader.h>
 #include <asm/cacheflush.h>
 
 /**
@@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-/*
- * external symbols are located outside the parent object (where the parent
- * object is either vmlinux or the kmod being patched).
- */
-static int klp_find_external_symbol(struct module *pmod, const char *name,
-				    unsigned long *addr)
+static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
 {
-	const struct kernel_symbol *sym;
+	int i, len, ret = 0;
+	Elf_Rela *relas;
+	Elf_Sym *sym;
+	char *symname, *sym_objname;
 
-	/* first, check if it's an exported symbol */
-	preempt_disable();
-	sym = find_symbol(name, NULL, NULL, true, true);
-	if (sym) {
-		*addr = sym->value;
-		preempt_enable();
-		return 0;
+	relas = (Elf_Rela *) relsec->sh_addr;
+	/* For each rela in this .klp.rel. section */
+	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
+		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
+		symname = pmod->core_strtab + sym->st_name;
+
+		len = strcspn(symname + KLP_TAG_LEN, ".");
+		sym_objname = strncmp(symname + KLP_TAG_LEN, "vmlinux", len) ?
+			kstrndup(symname + KLP_TAG_LEN, len, GFP_KERNEL) : NULL;
+		/* .klp.sym.objname.[symbol_name] */
+		symname += KLP_TAG_LEN + len + 1;
+
+		ret = klp_find_object_symbol(sym_objname, symname,
+					     KLP_SYMPOS(sym->st_other),
+					     (unsigned long *) &sym->st_value);
+		kfree(sym_objname);
+		if (ret)
+			return ret;
 	}
-	preempt_enable();
 
-	/*
-	 * Check if it's in another .o within the patch module. This also
-	 * checks that the external symbol is unique.
-	 */
-	return klp_find_object_symbol(pmod->name, name, 0, addr);
+	return ret;
 }
 
 static int klp_write_object_relocations(struct module *pmod,
 					struct klp_object *obj)
 {
-	int ret = 0;
-	unsigned long val;
-	struct klp_reloc *reloc;
+	int i, len, ret = 0;
+	char *secname;
+	const char *objname;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	if (WARN_ON(!obj->relocs))
-		return -EINVAL;
+	objname = klp_is_module(obj) ? obj->name : "vmlinux";
 
 	module_disable_ro(pmod);
+	/* For each klp rela section for this object */
+	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
+		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
+			continue;
+
+		secname = pmod->info->secstrings + pmod->info->sechdrs[i].sh_name;
+		/* .klp.rel.[objname].section_name */
+		len = strcspn(secname + KLP_TAG_LEN, ".");
+
+		if (strncmp(objname, secname + KLP_TAG_LEN, len))
+			continue;
 
-	for (reloc = obj->relocs; reloc->name; reloc++) {
-		/* discover the address of the referenced symbol */
-		if (reloc->external) {
-			if (reloc->sympos > 0) {
-				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
-				       reloc->name);
-				ret = -EINVAL;
-				goto out;
-			}
-			ret = klp_find_external_symbol(pmod, reloc->name, &val);
-		} else
-			ret = klp_find_object_symbol(obj->name,
-						     reloc->name,
-						     reloc->sympos,
-						     &val);
+		ret = klp_resolve_symbols(pmod->info->sechdrs + i, pmod);
 		if (ret)
 			goto out;
 
-		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
-					     val + reloc->addend);
-		if (ret) {
-			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
-			       reloc->name, val, ret);
+		ret = apply_relocate_add(pmod->info->sechdrs, pmod->core_strtab,
+					 pmod->info->index.sym, i, pmod);
+		if (ret)
 			goto out;
-		}
 	}
-
 out:
 	module_enable_ro(pmod);
 	return ret;
@@ -703,11 +702,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	struct klp_func *func;
 	int ret;
 
-	if (obj->relocs) {
-		ret = klp_write_object_relocations(patch->mod, obj);
-		if (ret)
-			return ret;
-	}
+	ret = klp_write_object_relocations(patch->mod, obj);
+	if (ret)
+		return ret;
 
 	klp_for_each_func(obj, func) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
-- 
2.4.3

[RFC PATCH v3 5/6] samples: livepatch: mark as livepatch module

[Jessica Yu]

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index fb8c861..e34f871 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -89,3 +89,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 v3 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 | 106 ++++++++++++++++++++++++
 1 file changed, 106 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..d825629
--- /dev/null
+++ b/Documentation/livepatch/patch-module-format.txt
@@ -0,0 +1,106 @@
+---------------------------
+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 their names are prefixed
+with the string ".klp.sym.${objname}.", where ${objname} is the name of the
+"object" where symbol stems from (the name of a module, for example).
+A symbol's position (used to differentiate duplicate symbols within the
+same object) in its object is encoded in the Elf_Sym st_other field
+and accessed with the KLP_SYMPOS macro (see include/linux/livepatch.h)
+
+Livepatch symbols are manually resolved by livepatch, and are used 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 symbol tables, and the symbol table is made accessible
+through module->core_symtab. For livepatch modules, core_symtab will
+contain an exact copy of the original symbol table as opposed to a stripped
+down version containing just the "core" symbols.
+
+-----------------------------------
+3. ".klp.rel." 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.rel.${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.rel.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
+  [30] .klp.rel.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
+  ...
+  [34] .klp.rel.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
+  [35] .klp.rel.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
+  [36] .klp.rel.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
+  [37] .klp.rel.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.
+
+Since Elf information is preserved for livepatch modules (see Section 4), a
+klp relocation section can be applied simply by passing in the appropriate
+section index to apply_relocate_add() (in the module loader code), which
+then uses it to access the relocation section and apply the relocations.
+
+--------------------------------------------------------
+4. 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.
+
+As for the klp relocation sections, since we save Elf information for
+livepatch modules, we can simply iterate through module->info->sechdrs
+and check if the SHF_RELA_LIVEPATCH flag is set for a particular section.
-- 
2.4.3

[RFC PATCH v3 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/livepatch/patch-module-format.txt | 106 ++++++++++++++++++++++++
 1 file changed, 106 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..d825629
--- /dev/null
+++ b/Documentation/livepatch/patch-module-format.txt
@@ -0,0 +1,106 @@
+---------------------------
+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 their names are prefixed
+with the string ".klp.sym.${objname}.", where ${objname} is the name of the
+"object" where symbol stems from (the name of a module, for example).
+A symbol's position (used to differentiate duplicate symbols within the
+same object) in its object is encoded in the Elf_Sym st_other field
+and accessed with the KLP_SYMPOS macro (see include/linux/livepatch.h)
+
+Livepatch symbols are manually resolved by livepatch, and are used 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 symbol tables, and the symbol table is made accessible
+through module->core_symtab. For livepatch modules, core_symtab will
+contain an exact copy of the original symbol table as opposed to a stripped
+down version containing just the "core" symbols.
+
+-----------------------------------
+3. ".klp.rel." 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.rel.${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.rel.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
+  [30] .klp.rel.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
+  ...
+  [34] .klp.rel.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
+  [35] .klp.rel.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
+  [36] .klp.rel.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
+  [37] .klp.rel.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.
+
+Since Elf information is preserved for livepatch modules (see Section 4), a
+klp relocation section can be applied simply by passing in the appropriate
+section index to apply_relocate_add() (in the module loader code), which
+then uses it to access the relocation section and apply the relocations.
+
+--------------------------------------------------------
+4. 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.
+
+As for the klp relocation sections, since we save Elf information for
+livepatch modules, we can simply iterate through module->info->sechdrs
+and check if the SHF_RELA_LIVEPATCH flag is set for a particular section.
-- 
2.4.3

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

[Rusty Russell]

Hi Jessica,

        Nice patch series.  Minor issues below, but they're really
just nits which can be patched afterwards if you're getting sick of
rebasing :)

> +#ifdef CONFIG_LIVEPATCH
> +/*
> + * copy_module_elf - preserve Elf information about a module
> + *
> + * Copy relevant Elf information from the load_info struct.
> + * Note: not all fields from the original load_info are
> + * copied into mod->info.

This makes me nervous, to have a struct which is half-initialized.

Better would be to have a separate type for this, eg:

struct livepatch_modinfo {
        Elf_Ehdr hdr;
        Elf_Shdr *sechdrs;
        char *secstrings;
        /* FIXME: Which of these do you need? */
        struct {
        	unsigned int sym, str, mod, vers, info, pcpu;
        } index;
};

This also avoids an extra allocation as hdr is no longer a ptr.

> +	/*
> +	 * Update symtab's sh_addr to point to a valid
> +	 * symbol table, as the temporary symtab in module
> +	 * init memory will be freed
> +	 */
> +	mod->info->sechdrs[mod->info->index.sym].sh_addr = (unsigned long)mod->core_symtab;

This comment is a bit misleading: it's actually pointing into the
temporary module copy, which will be discarded.  The init section is
slightly different...

> +#ifdef CONFIG_LIVEPATCH
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif

The term "check" implies no side-effects; I'd usually call this
int find_livepatch_modinfo(), and make sure you use the error code in
the caller:

> +	err = check_livepatch_modinfo(mod, info);
> +	if (err)
> +		return -ENOEXEC;
> +

Thanks,
Rusty.

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

[Rusty Russell]

Hi Jessica,

        Nice patch series.  Minor issues below, but they're really
just nits which can be patched afterwards if you're getting sick of
rebasing :)

> +#ifdef CONFIG_LIVEPATCH
> +/*
> + * copy_module_elf - preserve Elf information about a module
> + *
> + * Copy relevant Elf information from the load_info struct.
> + * Note: not all fields from the original load_info are
> + * copied into mod->info.

This makes me nervous, to have a struct which is half-initialized.

Better would be to have a separate type for this, eg:

struct livepatch_modinfo {
        Elf_Ehdr hdr;
        Elf_Shdr *sechdrs;
        char *secstrings;
        /* FIXME: Which of these do you need? */
        struct {
        	unsigned int sym, str, mod, vers, info, pcpu;
        } index;
};

This also avoids an extra allocation as hdr is no longer a ptr.

> +	/*
> +	 * Update symtab's sh_addr to point to a valid
> +	 * symbol table, as the temporary symtab in module
> +	 * init memory will be freed
> +	 */
> +	mod->info->sechdrs[mod->info->index.sym].sh_addr = (unsigned long)mod->core_symtab;

This comment is a bit misleading: it's actually pointing into the
temporary module copy, which will be discarded.  The init section is
slightly different...

> +#ifdef CONFIG_LIVEPATCH
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else
> +static int check_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif

The term "check" implies no side-effects; I'd usually call this
int find_livepatch_modinfo(), and make sure you use the error code in
the caller:

> +	err = check_livepatch_modinfo(mod, info);
> +	if (err)
> +		return -ENOEXEC;
> +

Thanks,
Rusty.

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

[Petr Mladek]

On Fri 2016-01-08 14:28:22, 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.
> 
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;
> +
> +		len = strcspn(symname + KLP_TAG_LEN, ".");

We should check that len is non-zero. Otherwise, sym_objname might
be empty string and symname might overflow below.

Also we should check that symname really starts with .klp.sym. to
avoid invalid memory access.

> +		sym_objname = strncmp(symname + KLP_TAG_LEN, "vmlinux", len) ?
> +			kstrndup(symname + KLP_TAG_LEN, len, GFP_KERNEL) : NULL;
> +		/* .klp.sym.objname.[symbol_name] */
> +		symname += KLP_TAG_LEN + len + 1;
> +
> +		ret = klp_find_object_symbol(sym_objname, symname,
> +					     KLP_SYMPOS(sym->st_other),
> +					     (unsigned long *) &sym->st_value);
> +		kfree(sym_objname);
> +		if (ret)
> +			return ret;
>  	}
> -	preempt_enable();
>  
> -	/*
> -	 * Check if it's in another .o within the patch module. This also
> -	 * checks that the external symbol is unique.
> -	 */
> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
> +	return ret;
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, len, ret = 0;
> +	char *secname;
> +	const char *objname;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp rela section for this object */
> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;
> +
> +		secname = pmod->info->secstrings + pmod->info->sechdrs[i].sh_name;
> +		/* .klp.rel.[objname].section_name */
> +		len = strcspn(secname + KLP_TAG_LEN, ".");

We should check that secname realy starts with .klp.rel. string
to avoid access out of memmory.

Otherwise, nice work.

Thanks,
Petr.

> +
> +		if (strncmp(objname, secname + KLP_TAG_LEN, len))
> +			continue;
>  
> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		/* discover the address of the referenced symbol */
> -		if (reloc->external) {
> -			if (reloc->sympos > 0) {
> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> -				       reloc->name);
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> -		} else
> -			ret = klp_find_object_symbol(obj->name,
> -						     reloc->name,
> -						     reloc->sympos,
> -						     &val);
> +		ret = klp_resolve_symbols(pmod->info->sechdrs + i, pmod);
>  		if (ret)
>  			goto out;
>  
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, val, ret);
> +		ret = apply_relocate_add(pmod->info->sechdrs, pmod->core_strtab,
> +					 pmod->info->index.sym, i, pmod);
> +		if (ret)
>  			goto out;
> -		}
>  	}
> -
>  out:
>  	module_enable_ro(pmod);
>  	return ret;

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

[Petr Mladek]

On Fri 2016-01-08 14:28:22, 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.
> 
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;
> +
> +		len = strcspn(symname + KLP_TAG_LEN, ".");

We should check that len is non-zero. Otherwise, sym_objname might
be empty string and symname might overflow below.

Also we should check that symname really starts with .klp.sym. to
avoid invalid memory access.

> +		sym_objname = strncmp(symname + KLP_TAG_LEN, "vmlinux", len) ?
> +			kstrndup(symname + KLP_TAG_LEN, len, GFP_KERNEL) : NULL;
> +		/* .klp.sym.objname.[symbol_name] */
> +		symname += KLP_TAG_LEN + len + 1;
> +
> +		ret = klp_find_object_symbol(sym_objname, symname,
> +					     KLP_SYMPOS(sym->st_other),
> +					     (unsigned long *) &sym->st_value);
> +		kfree(sym_objname);
> +		if (ret)
> +			return ret;
>  	}
> -	preempt_enable();
>  
> -	/*
> -	 * Check if it's in another .o within the patch module. This also
> -	 * checks that the external symbol is unique.
> -	 */
> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
> +	return ret;
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, len, ret = 0;
> +	char *secname;
> +	const char *objname;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp rela section for this object */
> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;
> +
> +		secname = pmod->info->secstrings + pmod->info->sechdrs[i].sh_name;
> +		/* .klp.rel.[objname].section_name */
> +		len = strcspn(secname + KLP_TAG_LEN, ".");

We should check that secname realy starts with .klp.rel. string
to avoid access out of memmory.

Otherwise, nice work.

Thanks,
Petr.

> +
> +		if (strncmp(objname, secname + KLP_TAG_LEN, len))
> +			continue;
>  
> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		/* discover the address of the referenced symbol */
> -		if (reloc->external) {
> -			if (reloc->sympos > 0) {
> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> -				       reloc->name);
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> -		} else
> -			ret = klp_find_object_symbol(obj->name,
> -						     reloc->name,
> -						     reloc->sympos,
> -						     &val);
> +		ret = klp_resolve_symbols(pmod->info->sechdrs + i, pmod);
>  		if (ret)
>  			goto out;
>  
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, val, ret);
> +		ret = apply_relocate_add(pmod->info->sechdrs, pmod->core_strtab,
> +					 pmod->info->index.sym, i, pmod);
> +		if (ret)
>  			goto out;
> -		}
>  	}
> -
>  out:
>  	module_enable_ro(pmod);
>  	return ret;

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

[Josh Poimboeuf]

On Mon, Jan 11, 2016 at 05:56:13PM +0100, Petr Mladek wrote:
> On Fri 2016-01-08 14:28:22, 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.
> > 
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> >  	return -EINVAL;
> >  }
> >  
> > -/*
> > - * external symbols are located outside the parent object (where the parent
> > - * object is either vmlinux or the kmod being patched).
> > - */
> > -static int klp_find_external_symbol(struct module *pmod, const char *name,
> > -				    unsigned long *addr)
> > +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
> >  {
> > -	const struct kernel_symbol *sym;
> > +	int i, len, ret = 0;
> > +	Elf_Rela *relas;
> > +	Elf_Sym *sym;
> > +	char *symname, *sym_objname;
> >  
> > -	/* first, check if it's an exported symbol */
> > -	preempt_disable();
> > -	sym = find_symbol(name, NULL, NULL, true, true);
> > -	if (sym) {
> > -		*addr = sym->value;
> > -		preempt_enable();
> > -		return 0;
> > +	relas = (Elf_Rela *) relsec->sh_addr;
> > +	/* For each rela in this .klp.rel. section */
> > +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> > +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> > +		symname = pmod->core_strtab + sym->st_name;
> > +
> > +		len = strcspn(symname + KLP_TAG_LEN, ".");
> 
> We should check that len is non-zero. Otherwise, sym_objname might
> be empty string and symname might overflow below.
> 
> Also we should check that symname really starts with .klp.sym. to
> avoid invalid memory access.

It would also be good to check for SHN_LIVEPATCH.

-- 
Josh

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

[Josh Poimboeuf]

On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>  include/linux/livepatch.h        |  33 +++++--------
>  kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>  5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> --- a/arch/x86/kernel/livepatch.c
> +++ /dev/null
> @@ -1,70 +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/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)
> -{
> -	size_t size = 4;
> -	unsigned long val;
> -	unsigned long core = (unsigned long)mod->core_layout.base;
> -	unsigned long core_size = mod->core_layout.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;
> -
> -	return probe_kernel_write((void *)loc, &val, size);
> -}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..2f12ce7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -65,27 +65,8 @@ struct klp_func {
>  };
>  
>  /**
> - * struct klp_reloc - relocation structure for live patching
> - * @loc:	address where the relocation will be written
> - * @sympos:	position in kallsyms to disambiguate symbols (optional)
> - * @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 {
> -	unsigned long loc;
> -	unsigned long sympos;
> -	unsigned long type;
> -	const char *name;
> -	int addend;
> -	int external;
> -};
> -
> -/**
>   * 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
>   * @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 +76,6 @@ struct klp_reloc {
>  struct klp_object {
>  	/* external */
>  	const char *name;
> -	struct klp_reloc *relocs;
>  	struct klp_func *funcs;
>  
>  	/* internal */
> @@ -123,6 +103,19 @@ struct klp_patch {
>  	enum klp_state state;
>  };
>  
> +/*
> + * Livepatch-specific symbols and relocation
> + * sections are prefixed with a tag:
> + * .klp.rel. for relocation sections
> + * .klp.sym. for livepatch symbols
> + */
> +#define KLP_TAG_LEN 9
> +/*
> + * Livepatch-specific bits for specifying symbol
> + * positions in the Elf_Sym st_other field
> + */
> +#define KLP_SYMPOS(o) (o >> 2) & 0xff
> +

Can st_value be used instead?  I think we ended up deciding that would
be better:

  https://lkml.kernel.org/g/20151210213328.GA6553@packer-debian-8-amd64.digitalocean.com

Because:

- st_value is easily viewable in readelf
- st_other has some arch-specific uses

And another reason not previously discussed:

- st_other is an unsigned char, which limits sympos to values < 64

>  #define klp_for_each_object(patch, obj) \
>  	for (obj = patch->objs; obj->funcs; obj++)
>  
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..64536a4 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 <linux/string.h>
> +#include <linux/moduleloader.h>
>  #include <asm/cacheflush.h>
>  
>  /**
> @@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;
> +
> +		len = strcspn(symname + KLP_TAG_LEN, ".");

It would be nice to have a comment here similar to the below one:

		/* .klp.sym.[objname].symbol_name */

> +		sym_objname = strncmp(symname + KLP_TAG_LEN, "vmlinux", len) ?

I think there's a bug here in the (unlikely) case where the module's
name is a subset of "vmlinux", e.g.  "vm".

> +			kstrndup(symname + KLP_TAG_LEN, len, GFP_KERNEL) : NULL;
> +		/* .klp.sym.objname.[symbol_name] */
> +		symname += KLP_TAG_LEN + len + 1;
> +
> +		ret = klp_find_object_symbol(sym_objname, symname,
> +					     KLP_SYMPOS(sym->st_other),
> +					     (unsigned long *) &sym->st_value);
> +		kfree(sym_objname);
> +		if (ret)
> +			return ret;
>  	}
> -	preempt_enable();
>  
> -	/*
> -	 * Check if it's in another .o within the patch module. This also
> -	 * checks that the external symbol is unique.
> -	 */
> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
> +	return ret;
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, len, ret = 0;
> +	char *secname;
> +	const char *objname;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp rela section for this object */
> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;
> +
> +		secname = pmod->info->secstrings + pmod->info->sechdrs[i].sh_name;
> +		/* .klp.rel.[objname].section_name */
> +		len = strcspn(secname + KLP_TAG_LEN, ".");
> +
> +		if (strncmp(objname, secname + KLP_TAG_LEN, len))
> +			continue;

Same problem here.  For ".klp.rel.foo.sym", an objname of "foobar" will
give a false match.

> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		/* discover the address of the referenced symbol */
> -		if (reloc->external) {
> -			if (reloc->sympos > 0) {
> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> -				       reloc->name);
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> -		} else
> -			ret = klp_find_object_symbol(obj->name,
> -						     reloc->name,
> -						     reloc->sympos,
> -						     &val);
> +		ret = klp_resolve_symbols(pmod->info->sechdrs + i, pmod);
>  		if (ret)
>  			goto out;
>  
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, val, ret);
> +		ret = apply_relocate_add(pmod->info->sechdrs, pmod->core_strtab,
> +					 pmod->info->index.sym, i, pmod);
> +		if (ret)
>  			goto out;
> -		}
>  	}
> -
>  out:
>  	module_enable_ro(pmod);
>  	return ret;
> @@ -703,11 +702,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  	struct klp_func *func;
>  	int ret;
>  
> -	if (obj->relocs) {
> -		ret = klp_write_object_relocations(patch->mod, obj);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = klp_write_object_relocations(patch->mod, obj);
> +	if (ret)
> +		return ret;
>  
>  	klp_for_each_func(obj, func) {
>  		ret = klp_find_object_symbol(obj->name, func->old_name,
> -- 
> 2.4.3
> 

-- 
Josh

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

[Josh Poimboeuf]

On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>  include/linux/livepatch.h        |  33 +++++--------
>  kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>  5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> --- a/arch/x86/kernel/livepatch.c
> +++ /dev/null
> @@ -1,70 +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/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)
> -{
> -	size_t size = 4;
> -	unsigned long val;
> -	unsigned long core = (unsigned long)mod->core_layout.base;
> -	unsigned long core_size = mod->core_layout.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;
> -
> -	return probe_kernel_write((void *)loc, &val, size);
> -}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a882865..2f12ce7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -65,27 +65,8 @@ struct klp_func {
>  };
>  
>  /**
> - * struct klp_reloc - relocation structure for live patching
> - * @loc:	address where the relocation will be written
> - * @sympos:	position in kallsyms to disambiguate symbols (optional)
> - * @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 {
> -	unsigned long loc;
> -	unsigned long sympos;
> -	unsigned long type;
> -	const char *name;
> -	int addend;
> -	int external;
> -};
> -
> -/**
>   * 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
>   * @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 +76,6 @@ struct klp_reloc {
>  struct klp_object {
>  	/* external */
>  	const char *name;
> -	struct klp_reloc *relocs;
>  	struct klp_func *funcs;
>  
>  	/* internal */
> @@ -123,6 +103,19 @@ struct klp_patch {
>  	enum klp_state state;
>  };
>  
> +/*
> + * Livepatch-specific symbols and relocation
> + * sections are prefixed with a tag:
> + * .klp.rel. for relocation sections
> + * .klp.sym. for livepatch symbols
> + */
> +#define KLP_TAG_LEN 9
> +/*
> + * Livepatch-specific bits for specifying symbol
> + * positions in the Elf_Sym st_other field
> + */
> +#define KLP_SYMPOS(o) (o >> 2) & 0xff
> +

Can st_value be used instead?  I think we ended up deciding that would
be better:

  https://lkml.kernel.org/g/20151210213328.GA6553-N0bYjD2NfQ6k4hzjq3hgyGTy53QMssKEsp+A89P3RPuQWHG76I6BsA@public.gmane.org

Because:

- st_value is easily viewable in readelf
- st_other has some arch-specific uses

And another reason not previously discussed:

- st_other is an unsigned char, which limits sympos to values < 64

>  #define klp_for_each_object(patch, obj) \
>  	for (obj = patch->objs; obj->funcs; obj++)
>  
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..64536a4 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 <linux/string.h>
> +#include <linux/moduleloader.h>
>  #include <asm/cacheflush.h>
>  
>  /**
> @@ -204,74 +207,70 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;
> +
> +		len = strcspn(symname + KLP_TAG_LEN, ".");

It would be nice to have a comment here similar to the below one:

		/* .klp.sym.[objname].symbol_name */

> +		sym_objname = strncmp(symname + KLP_TAG_LEN, "vmlinux", len) ?

I think there's a bug here in the (unlikely) case where the module's
name is a subset of "vmlinux", e.g.  "vm".

> +			kstrndup(symname + KLP_TAG_LEN, len, GFP_KERNEL) : NULL;
> +		/* .klp.sym.objname.[symbol_name] */
> +		symname += KLP_TAG_LEN + len + 1;
> +
> +		ret = klp_find_object_symbol(sym_objname, symname,
> +					     KLP_SYMPOS(sym->st_other),
> +					     (unsigned long *) &sym->st_value);
> +		kfree(sym_objname);
> +		if (ret)
> +			return ret;
>  	}
> -	preempt_enable();
>  
> -	/*
> -	 * Check if it's in another .o within the patch module. This also
> -	 * checks that the external symbol is unique.
> -	 */
> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
> +	return ret;
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, len, ret = 0;
> +	char *secname;
> +	const char *objname;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp rela section for this object */
> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;
> +
> +		secname = pmod->info->secstrings + pmod->info->sechdrs[i].sh_name;
> +		/* .klp.rel.[objname].section_name */
> +		len = strcspn(secname + KLP_TAG_LEN, ".");
> +
> +		if (strncmp(objname, secname + KLP_TAG_LEN, len))
> +			continue;

Same problem here.  For ".klp.rel.foo.sym", an objname of "foobar" will
give a false match.

> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		/* discover the address of the referenced symbol */
> -		if (reloc->external) {
> -			if (reloc->sympos > 0) {
> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> -				       reloc->name);
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> -		} else
> -			ret = klp_find_object_symbol(obj->name,
> -						     reloc->name,
> -						     reloc->sympos,
> -						     &val);
> +		ret = klp_resolve_symbols(pmod->info->sechdrs + i, pmod);
>  		if (ret)
>  			goto out;
>  
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, val, ret);
> +		ret = apply_relocate_add(pmod->info->sechdrs, pmod->core_strtab,
> +					 pmod->info->index.sym, i, pmod);
> +		if (ret)
>  			goto out;
> -		}
>  	}
> -
>  out:
>  	module_enable_ro(pmod);
>  	return ret;
> @@ -703,11 +702,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  	struct klp_func *func;
>  	int ret;
>  
> -	if (obj->relocs) {
> -		ret = klp_write_object_relocations(patch->mod, obj);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = klp_write_object_relocations(patch->mod, obj);
> +	if (ret)
> +		return ret;
>  
>  	klp_for_each_func(obj, func) {
>  		ret = klp_find_object_symbol(obj->name, func->old_name,
> -- 
> 2.4.3
> 

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [11/01/16 15:33 -0600]:
>On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>>  include/linux/livepatch.h        |  33 +++++--------
>>  kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>>  5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
>> --- a/arch/x86/kernel/livepatch.c
>> +++ /dev/null
>> @@ -1,70 +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/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)
>> -{
>> -	size_t size = 4;
>> -	unsigned long val;
>> -	unsigned long core = (unsigned long)mod->core_layout.base;
>> -	unsigned long core_size = mod->core_layout.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;
>> -
>> -	return probe_kernel_write((void *)loc, &val, size);
>> -}
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index a882865..2f12ce7 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -65,27 +65,8 @@ struct klp_func {
>>  };
>>
>>  /**
>> - * struct klp_reloc - relocation structure for live patching
>> - * @loc:	address where the relocation will be written
>> - * @sympos:	position in kallsyms to disambiguate symbols (optional)
>> - * @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 {
>> -	unsigned long loc;
>> -	unsigned long sympos;
>> -	unsigned long type;
>> -	const char *name;
>> -	int addend;
>> -	int external;
>> -};
>> -
>> -/**
>>   * 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
>>   * @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 +76,6 @@ struct klp_reloc {
>>  struct klp_object {
>>  	/* external */
>>  	const char *name;
>> -	struct klp_reloc *relocs;
>>  	struct klp_func *funcs;
>>
>>  	/* internal */
>> @@ -123,6 +103,19 @@ struct klp_patch {
>>  	enum klp_state state;
>>  };
>>
>> +/*
>> + * Livepatch-specific symbols and relocation
>> + * sections are prefixed with a tag:
>> + * .klp.rel. for relocation sections
>> + * .klp.sym. for livepatch symbols
>> + */
>> +#define KLP_TAG_LEN 9
>> +/*
>> + * Livepatch-specific bits for specifying symbol
>> + * positions in the Elf_Sym st_other field
>> + */
>> +#define KLP_SYMPOS(o) (o >> 2) & 0xff
>> +
>
>Can st_value be used instead?  I think we ended up deciding that would
>be better:
>
>  https://lkml.kernel.org/g/20151210213328.GA6553@packer-debian-8-amd64.digitalocean.com
>
>Because:
>
>- st_value is easily viewable in readelf
>- st_other has some arch-specific uses
>
>And another reason not previously discussed:
>
>- st_other is an unsigned char, which limits sympos to values < 64

I originally wanted to encode the symbol position in st_value, but
I've discovered that since st_value is overwritten once the
symbol is (first) resolved, we no longer have the symbol position if we
need to resolve the symbols again (so we wouldn't be able to resolve
them). This could happen when we patch a module that loads and
unloads more than once for example.

I chose st_other since it isn't touched in s390x kernel code nor in
x86 kernel code (it does get used in the x86 userspace reloc tool in
arch/x86/tools, which is why I left the first two bits alone for
ELF_ST_VISIBILITY, and the rest had undefined usage). However this
isn't the best approach and I'm afraid of stepping on other arch's
toes in future patches. Perhaps there is another field where we can
stuff the sympos in? st_size for instance doesn't seem to be touched
anywhere in the module loader. I don't know if I'd want to stuff
sympos in the sym name, there's enough going on there..

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [11/01/16 15:33 -0600]:
>On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>>  include/linux/livepatch.h        |  33 +++++--------
>>  kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>>  5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
>> --- a/arch/x86/kernel/livepatch.c
>> +++ /dev/null
>> @@ -1,70 +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/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)
>> -{
>> -	size_t size = 4;
>> -	unsigned long val;
>> -	unsigned long core = (unsigned long)mod->core_layout.base;
>> -	unsigned long core_size = mod->core_layout.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;
>> -
>> -	return probe_kernel_write((void *)loc, &val, size);
>> -}
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index a882865..2f12ce7 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -65,27 +65,8 @@ struct klp_func {
>>  };
>>
>>  /**
>> - * struct klp_reloc - relocation structure for live patching
>> - * @loc:	address where the relocation will be written
>> - * @sympos:	position in kallsyms to disambiguate symbols (optional)
>> - * @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 {
>> -	unsigned long loc;
>> -	unsigned long sympos;
>> -	unsigned long type;
>> -	const char *name;
>> -	int addend;
>> -	int external;
>> -};
>> -
>> -/**
>>   * 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
>>   * @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 +76,6 @@ struct klp_reloc {
>>  struct klp_object {
>>  	/* external */
>>  	const char *name;
>> -	struct klp_reloc *relocs;
>>  	struct klp_func *funcs;
>>
>>  	/* internal */
>> @@ -123,6 +103,19 @@ struct klp_patch {
>>  	enum klp_state state;
>>  };
>>
>> +/*
>> + * Livepatch-specific symbols and relocation
>> + * sections are prefixed with a tag:
>> + * .klp.rel. for relocation sections
>> + * .klp.sym. for livepatch symbols
>> + */
>> +#define KLP_TAG_LEN 9
>> +/*
>> + * Livepatch-specific bits for specifying symbol
>> + * positions in the Elf_Sym st_other field
>> + */
>> +#define KLP_SYMPOS(o) (o >> 2) & 0xff
>> +
>
>Can st_value be used instead?  I think we ended up deciding that would
>be better:
>
>  https://lkml.kernel.org/g/20151210213328.GA6553-N0bYjD2NfQ6k4hzjq3hgyGTy53QMssKEsp+A89P3RPuQWHG76I6BsA@public.gmane.org
>
>Because:
>
>- st_value is easily viewable in readelf
>- st_other has some arch-specific uses
>
>And another reason not previously discussed:
>
>- st_other is an unsigned char, which limits sympos to values < 64

I originally wanted to encode the symbol position in st_value, but
I've discovered that since st_value is overwritten once the
symbol is (first) resolved, we no longer have the symbol position if we
need to resolve the symbols again (so we wouldn't be able to resolve
them). This could happen when we patch a module that loads and
unloads more than once for example.

I chose st_other since it isn't touched in s390x kernel code nor in
x86 kernel code (it does get used in the x86 userspace reloc tool in
arch/x86/tools, which is why I left the first two bits alone for
ELF_ST_VISIBILITY, and the rest had undefined usage). However this
isn't the best approach and I'm afraid of stepping on other arch's
toes in future patches. Perhaps there is another field where we can
stuff the sympos in? st_size for instance doesn't seem to be touched
anywhere in the module loader. I don't know if I'd want to stuff
sympos in the sym name, there's enough going on there..

Jessica

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
> >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
> >> include/linux/livepatch.h        |  33 +++++--------
> >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
> >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> >>--- a/arch/x86/kernel/livepatch.c
> >>+++ /dev/null
> >>@@ -1,70 +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/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)
> >>-{
> >>-	size_t size = 4;
> >>-	unsigned long val;
> >>-	unsigned long core = (unsigned long)mod->core_layout.base;
> >>-	unsigned long core_size = mod->core_layout.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;
> >>-
> >>-	return probe_kernel_write((void *)loc, &val, size);
> >>-}
> >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> >>index a882865..2f12ce7 100644
> >>--- a/include/linux/livepatch.h
> >>+++ b/include/linux/livepatch.h
> >>@@ -65,27 +65,8 @@ struct klp_func {
> >> };
> >>
> >> /**
> >>- * struct klp_reloc - relocation structure for live patching
> >>- * @loc:	address where the relocation will be written
> >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
> >>- * @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 {
> >>-	unsigned long loc;
> >>-	unsigned long sympos;
> >>-	unsigned long type;
> >>-	const char *name;
> >>-	int addend;
> >>-	int external;
> >>-};
> >>-
> >>-/**
> >>  * 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
> >>  * @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 +76,6 @@ struct klp_reloc {
> >> struct klp_object {
> >> 	/* external */
> >> 	const char *name;
> >>-	struct klp_reloc *relocs;
> >> 	struct klp_func *funcs;
> >>
> >> 	/* internal */
> >>@@ -123,6 +103,19 @@ struct klp_patch {
> >> 	enum klp_state state;
> >> };
> >>
> >>+/*
> >>+ * Livepatch-specific symbols and relocation
> >>+ * sections are prefixed with a tag:
> >>+ * .klp.rel. for relocation sections
> >>+ * .klp.sym. for livepatch symbols
> >>+ */
> >>+#define KLP_TAG_LEN 9
> >>+/*
> >>+ * Livepatch-specific bits for specifying symbol
> >>+ * positions in the Elf_Sym st_other field
> >>+ */
> >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
> >>+
> >
> >Can st_value be used instead?  I think we ended up deciding that would
> >be better:
> >
> > https://lkml.kernel.org/g/20151210213328.GA6553@packer-debian-8-amd64.digitalocean.com
> >
> >Because:
> >
> >- st_value is easily viewable in readelf
> >- st_other has some arch-specific uses
> >
> >And another reason not previously discussed:
> >
> >- st_other is an unsigned char, which limits sympos to values < 64
> 
> I originally wanted to encode the symbol position in st_value, but
> I've discovered that since st_value is overwritten once the
> symbol is (first) resolved, we no longer have the symbol position if we
> need to resolve the symbols again (so we wouldn't be able to resolve
> them). This could happen when we patch a module that loads and
> unloads more than once for example.

Ah, good point.

> I chose st_other since it isn't touched in s390x kernel code nor in
> x86 kernel code (it does get used in the x86 userspace reloc tool in
> arch/x86/tools, which is why I left the first two bits alone for
> ELF_ST_VISIBILITY, and the rest had undefined usage). However this
> isn't the best approach and I'm afraid of stepping on other arch's
> toes in future patches. Perhaps there is another field where we can
> stuff the sympos in? st_size for instance doesn't seem to be touched
> anywhere in the module loader. I don't know if I'd want to stuff
> sympos in the sym name, there's enough going on there..

I think st_other still isn't going to work because of possible arch
conflicts and because of its small size (6 bits) otherwise.

st_size could work.  It doesn't _seem_ to be used by the module code,
though it's hard to confirm that for all arches.  But it makes me a
little nervous to override that field.  For example, might it confuse
some user-space tools out there, or the linker?

Maybe encoding it in the sym name is the safest bet.  Why not, we've
already got a bunch of other stuff there anyway :-)  One idea would be
to append it with ',<sympos>' to be consistent with the sysfs entries:

  .klp.sym.vmlinux.symname,1

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Josh Poimboeuf]

On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
> >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
> >> include/linux/livepatch.h        |  33 +++++--------
> >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
> >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> >>--- a/arch/x86/kernel/livepatch.c
> >>+++ /dev/null
> >>@@ -1,70 +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/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)
> >>-{
> >>-	size_t size = 4;
> >>-	unsigned long val;
> >>-	unsigned long core = (unsigned long)mod->core_layout.base;
> >>-	unsigned long core_size = mod->core_layout.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;
> >>-
> >>-	return probe_kernel_write((void *)loc, &val, size);
> >>-}
> >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> >>index a882865..2f12ce7 100644
> >>--- a/include/linux/livepatch.h
> >>+++ b/include/linux/livepatch.h
> >>@@ -65,27 +65,8 @@ struct klp_func {
> >> };
> >>
> >> /**
> >>- * struct klp_reloc - relocation structure for live patching
> >>- * @loc:	address where the relocation will be written
> >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
> >>- * @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 {
> >>-	unsigned long loc;
> >>-	unsigned long sympos;
> >>-	unsigned long type;
> >>-	const char *name;
> >>-	int addend;
> >>-	int external;
> >>-};
> >>-
> >>-/**
> >>  * 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
> >>  * @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 +76,6 @@ struct klp_reloc {
> >> struct klp_object {
> >> 	/* external */
> >> 	const char *name;
> >>-	struct klp_reloc *relocs;
> >> 	struct klp_func *funcs;
> >>
> >> 	/* internal */
> >>@@ -123,6 +103,19 @@ struct klp_patch {
> >> 	enum klp_state state;
> >> };
> >>
> >>+/*
> >>+ * Livepatch-specific symbols and relocation
> >>+ * sections are prefixed with a tag:
> >>+ * .klp.rel. for relocation sections
> >>+ * .klp.sym. for livepatch symbols
> >>+ */
> >>+#define KLP_TAG_LEN 9
> >>+/*
> >>+ * Livepatch-specific bits for specifying symbol
> >>+ * positions in the Elf_Sym st_other field
> >>+ */
> >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
> >>+
> >
> >Can st_value be used instead?  I think we ended up deciding that would
> >be better:
> >
> > https://lkml.kernel.org/g/20151210213328.GA6553-N0bYjD2NfQ6k4hzjq3hgyGTy53QMssKEsp+A89P3RPuQWHG76I6BsA@public.gmane.org
> >
> >Because:
> >
> >- st_value is easily viewable in readelf
> >- st_other has some arch-specific uses
> >
> >And another reason not previously discussed:
> >
> >- st_other is an unsigned char, which limits sympos to values < 64
> 
> I originally wanted to encode the symbol position in st_value, but
> I've discovered that since st_value is overwritten once the
> symbol is (first) resolved, we no longer have the symbol position if we
> need to resolve the symbols again (so we wouldn't be able to resolve
> them). This could happen when we patch a module that loads and
> unloads more than once for example.

Ah, good point.

> I chose st_other since it isn't touched in s390x kernel code nor in
> x86 kernel code (it does get used in the x86 userspace reloc tool in
> arch/x86/tools, which is why I left the first two bits alone for
> ELF_ST_VISIBILITY, and the rest had undefined usage). However this
> isn't the best approach and I'm afraid of stepping on other arch's
> toes in future patches. Perhaps there is another field where we can
> stuff the sympos in? st_size for instance doesn't seem to be touched
> anywhere in the module loader. I don't know if I'd want to stuff
> sympos in the sym name, there's enough going on there..

I think st_other still isn't going to work because of possible arch
conflicts and because of its small size (6 bits) otherwise.

st_size could work.  It doesn't _seem_ to be used by the module code,
though it's hard to confirm that for all arches.  But it makes me a
little nervous to override that field.  For example, might it confuse
some user-space tools out there, or the linker?

Maybe encoding it in the sym name is the safest bet.  Why not, we've
already got a bunch of other stuff there anyway :-)  One idea would be
to append it with ',<sympos>' to be consistent with the sysfs entries:

  .klp.sym.vmlinux.symname,1

-- 
Josh

Re: livepatch: reuse module loader code to write relocations

[Petr Mladek]

On Mon 2016-01-11 21:05:52, Josh Poimboeuf wrote:
> On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
> > >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
> > >> include/linux/livepatch.h        |  33 +++++--------
> > >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
> > >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> > >>--- a/arch/x86/kernel/livepatch.c
> > >>+++ /dev/null
> > >>@@ -1,70 +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/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)
> > >>-{
> > >>-	size_t size = 4;
> > >>-	unsigned long val;
> > >>-	unsigned long core = (unsigned long)mod->core_layout.base;
> > >>-	unsigned long core_size = mod->core_layout.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;
> > >>-
> > >>-	return probe_kernel_write((void *)loc, &val, size);
> > >>-}
> > >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > >>index a882865..2f12ce7 100644
> > >>--- a/include/linux/livepatch.h
> > >>+++ b/include/linux/livepatch.h
> > >>@@ -65,27 +65,8 @@ struct klp_func {
> > >> };
> > >>
> > >> /**
> > >>- * struct klp_reloc - relocation structure for live patching
> > >>- * @loc:	address where the relocation will be written
> > >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
> > >>- * @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 {
> > >>-	unsigned long loc;
> > >>-	unsigned long sympos;
> > >>-	unsigned long type;
> > >>-	const char *name;
> > >>-	int addend;
> > >>-	int external;
> > >>-};
> > >>-
> > >>-/**
> > >>  * 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
> > >>  * @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 +76,6 @@ struct klp_reloc {
> > >> struct klp_object {
> > >> 	/* external */
> > >> 	const char *name;
> > >>-	struct klp_reloc *relocs;
> > >> 	struct klp_func *funcs;
> > >>
> > >> 	/* internal */
> > >>@@ -123,6 +103,19 @@ struct klp_patch {
> > >> 	enum klp_state state;
> > >> };
> > >>
> > >>+/*
> > >>+ * Livepatch-specific symbols and relocation
> > >>+ * sections are prefixed with a tag:
> > >>+ * .klp.rel. for relocation sections
> > >>+ * .klp.sym. for livepatch symbols
> > >>+ */
> > >>+#define KLP_TAG_LEN 9
> > >>+/*
> > >>+ * Livepatch-specific bits for specifying symbol
> > >>+ * positions in the Elf_Sym st_other field
> > >>+ */
> > >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
> > >>+
> > >Can st_value be used instead?  I think we ended up deciding that would
> > >be better:
>
> Maybe encoding it in the sym name is the safest bet.  Why not, we've
> already got a bunch of other stuff there anyway :-)  One idea would be
> to append it with ',<sympos>' to be consistent with the sysfs entries:
>
>   .klp.sym.vmlinux.symname,1

This looks like the most safe solution to me as well.

Best Regards,
Petr

Re: livepatch: reuse module loader code to write relocations

[Petr Mladek]

On Mon 2016-01-11 21:05:52, Josh Poimboeuf wrote:
> On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
> > +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
> > >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
> > >> include/linux/livepatch.h        |  33 +++++--------
> > >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
> > >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
> > >>--- a/arch/x86/kernel/livepatch.c
> > >>+++ /dev/null
> > >>@@ -1,70 +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/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)
> > >>-{
> > >>-	size_t size = 4;
> > >>-	unsigned long val;
> > >>-	unsigned long core = (unsigned long)mod->core_layout.base;
> > >>-	unsigned long core_size = mod->core_layout.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;
> > >>-
> > >>-	return probe_kernel_write((void *)loc, &val, size);
> > >>-}
> > >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > >>index a882865..2f12ce7 100644
> > >>--- a/include/linux/livepatch.h
> > >>+++ b/include/linux/livepatch.h
> > >>@@ -65,27 +65,8 @@ struct klp_func {
> > >> };
> > >>
> > >> /**
> > >>- * struct klp_reloc - relocation structure for live patching
> > >>- * @loc:	address where the relocation will be written
> > >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
> > >>- * @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 {
> > >>-	unsigned long loc;
> > >>-	unsigned long sympos;
> > >>-	unsigned long type;
> > >>-	const char *name;
> > >>-	int addend;
> > >>-	int external;
> > >>-};
> > >>-
> > >>-/**
> > >>  * 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
> > >>  * @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 +76,6 @@ struct klp_reloc {
> > >> struct klp_object {
> > >> 	/* external */
> > >> 	const char *name;
> > >>-	struct klp_reloc *relocs;
> > >> 	struct klp_func *funcs;
> > >>
> > >> 	/* internal */
> > >>@@ -123,6 +103,19 @@ struct klp_patch {
> > >> 	enum klp_state state;
> > >> };
> > >>
> > >>+/*
> > >>+ * Livepatch-specific symbols and relocation
> > >>+ * sections are prefixed with a tag:
> > >>+ * .klp.rel. for relocation sections
> > >>+ * .klp.sym. for livepatch symbols
> > >>+ */
> > >>+#define KLP_TAG_LEN 9
> > >>+/*
> > >>+ * Livepatch-specific bits for specifying symbol
> > >>+ * positions in the Elf_Sym st_other field
> > >>+ */
> > >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
> > >>+
> > >Can st_value be used instead?  I think we ended up deciding that would
> > >be better:
>
> Maybe encoding it in the sym name is the safest bet.  Why not, we've
> already got a bunch of other stuff there anyway :-)  One idea would be
> to append it with ',<sympos>' to be consistent with the sysfs entries:
>
>   .klp.sym.vmlinux.symname,1

This looks like the most safe solution to me as well.

Best Regards,
Petr

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

[Petr Mladek]

Hi Jessica,

first, thanks a lot for writing a documentation. It is really
appreciated!

To be honest, I am not sure if it makes sense to give feedback
at this stage. It seems that there still will be some changes
in the elf format.

On Fri 2016-01-08 14:28:24, Jessica Yu wrote:
> Document the special Elf sections and constants livepatch modules use.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  Documentation/livepatch/patch-module-format.txt | 106 ++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/livepatch/patch-module-format.txt

I would call this symbol-relocation.txt or so. It describes only this
part of the patch format.

> 
> diff --git a/Documentation/livepatch/patch-module-format.txt b/Documentation/livepatch/patch-module-format.txt
> new file mode 100644
> index 0000000..d825629
> --- /dev/null
> +++ b/Documentation/livepatch/patch-module-format.txt
> @@ -0,0 +1,106 @@
> +---------------------------
> +Livepatch module Elf format
> +---------------------------

I would start with a description what symbols are relocated and why
it needs to be done a special way.

Also I would switch a bit the order of the sections below.
We have relocation sections for each patched object. They are
used when the object is loaded and patched. Each section has
relocation information for symbols that are accessed from
new versions of functions for the patched object.
Hmm, we should add some generic documentation about
LivePatching that would describe the design and used terms.


> +This document outlines the special Elf constants and sections livepatch
> +uses to patch both modules and the kernel (vmlinux).

> +--------------------------
> +1. Livepatch modinfo field
> +--------------------------
> +

Please, express that livepatch must set the "livepatch" field to be
indentified and loaded correctly.

> +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

s/kpatch/livepatch/ ;-)

> +filename:		kpatch-meminfo.ko

same here ;-)

> +livepatch:		Y
> +license:		GPL
> +depends:
> +vermagic:		4.3.0+ SMP mod_unload
> +
> +--------------------
> +2. Livepatch symbols
> +--------------------

I would be more precise here. These are symbols that will get
relocated by the livepatch framework.

> +
> +These are symbols marked with SHN_LIVEPATCH and their names are prefixed
> +with the string ".klp.sym.${objname}.", where ${objname} is the

We should describe the entire format. I mean
.klp.sym.${objname}.[symname]. Also we should explain why symname is optional.

> name of the
> +"object" where symbol stems from (the name of a module, for example).
> +A symbol's position (used to differentiate duplicate symbols within the
> +same object) in its object is encoded in the Elf_Sym st_other field
> +and accessed with the KLP_SYMPOS macro (see include/linux/livepatch.h)
> +
> +Livepatch symbols are manually resolved by livepatch, and are used in cases

Please, what do you mean by "manually" resolved?

> +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 symbol tables, and the symbol table is made accessible
> +through module->core_symtab. For livepatch modules, core_symtab will
> +contain an exact copy of the original symbol table as opposed to a stripped
> +down version containing just the "core" symbols.
> +
> +-----------------------------------
> +3. ".klp.rel." 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.rel.${objname}.", where ${objname} is the name of the
> "object" being

It seems that the full format is ".klp.rel.[objname].section_name"
and section_name is not explained here.


> +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.rel.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
> +  [30] .klp.rel.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
> +  ...
> +  [34] .klp.rel.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
> +  [35] .klp.rel.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
> +  [36] .klp.rel.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
> +  [37] .klp.rel.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

Hmm, it is SHF_RELA... I wonder if we want to rather use
.klp.rela. prefix to be consistent.

> +loader can identify them and avoid treating them as regular SHT_RELA
> +sections, since they are manually managed by livepatch.
> +
> +Since Elf information is preserved for livepatch modules (see Section 4), a
> +klp relocation section can be applied simply by passing in the appropriate
> +section index to apply_relocate_add() (in the module loader code), which
> +then uses it to access the relocation section and apply the relocations.
> +
> +--------------------------------------------------------
> +4. 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.

We should add some notice also to the source code or commit message
aboud why we preserve all the symbols for live patch.

Great work.

Thanks,
Petr

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

[Josh Poimboeuf]

On Tue, Jan 12, 2016 at 01:09:51PM +0100, Petr Mladek wrote:
> Hi Jessica,
> 
> first, thanks a lot for writing a documentation. It is really
> appreciated!
> 
> To be honest, I am not sure if it makes sense to give feedback
> at this stage. It seems that there still will be some changes
> in the elf format.
> 
> On Fri 2016-01-08 14:28:24, Jessica Yu wrote:
> > Document the special Elf sections and constants livepatch modules use.
> > 
> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > ---
> >  Documentation/livepatch/patch-module-format.txt | 106 ++++++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >  create mode 100644 Documentation/livepatch/patch-module-format.txt
> 
> I would call this symbol-relocation.txt or so. It describes only this
> part of the patch format.
> 
> > 
> > diff --git a/Documentation/livepatch/patch-module-format.txt b/Documentation/livepatch/patch-module-format.txt
> > new file mode 100644
> > index 0000000..d825629
> > --- /dev/null
> > +++ b/Documentation/livepatch/patch-module-format.txt
> > @@ -0,0 +1,106 @@
> > +---------------------------
> > +Livepatch module Elf format
> > +---------------------------
> 
> I would start with a description what symbols are relocated and why
> it needs to be done a special way.

The cover letter has this information, and a lot of other useful
background.  It would be good to add all that background to this
document as an introduction.

-- 
Josh

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

[Miroslav Benes]

Hi Jessica,

I walked through the series and it looks really nice. Others have already 
pointed out the issues I also found, so only few minor things below.

First thing, could you copy&paste the information and reasoning from the 
cover letter to the changelogs where appropriate? It is very detailed and 
it would be a pity to lost it.

On Fri, 8 Jan 2016, Jessica Yu wrote:

> 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);

You left klp_write_module_reloc() in arch/s390/include/asm/livepatch.h I'm 
afraid. Anyway it would be really great if you managed to test the series 
on s390 somehow. Just to know that all the roadblocks are really gone.

> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;

Maybe it would be better to use pmod->symtab and pmod->strtab everywhere. 
It should be the same, but core_* versions are only helpers used in 
load_module and friends. There is even a comment in 
include/linux/module.h.

	/*
	 * We keep the symbol and string tables for kallsyms.
	 * The core_* fields below are temporary, loader-only (they
	 * could really be discarded after module init).
	 */

We should respect that.

Thanks,
Miroslav

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

[Miroslav Benes]

Hi Jessica,

I walked through the series and it looks really nice. Others have already 
pointed out the issues I also found, so only few minor things below.

First thing, could you copy&paste the information and reasoning from the 
cover letter to the changelogs where appropriate? It is very detailed and 
it would be a pity to lost it.

On Fri, 8 Jan 2016, Jessica Yu wrote:

> 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);

You left klp_write_module_reloc() in arch/s390/include/asm/livepatch.h I'm 
afraid. Anyway it would be really great if you managed to test the series 
on s390 somehow. Just to know that all the roadblocks are really gone.

> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;

Maybe it would be better to use pmod->symtab and pmod->strtab everywhere. 
It should be the same, but core_* versions are only helpers used in 
load_module and friends. There is even a comment in 
include/linux/module.h.

	/*
	 * We keep the symbol and string tables for kallsyms.
	 * The core_* fields below are temporary, loader-only (they
	 * could really be discarded after module init).
	 */

We should respect that.

Thanks,
Miroslav

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

[Miroslav Benes]

On Fri, 8 Jan 2016, Jessica Yu wrote:

>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, len, ret = 0;
> +	char *secname;
> +	const char *objname;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp rela section for this object */
> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;

One more thing. If the module does not specify it is a live patch module 
in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live 
patch module otherwise (it calls klp_register_patch in its init function), 
the kernel crashes here. pmod->info is not initialized at all. This should 
be fixed. Perhaps the easiest would be to call 
klp_write_object_relocations() in klp_init_object_loaded() only if 
is_livepatch_module() returns true. Similar to a check for obj->relocs 
before.

Miroslav

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

[Jiri Kosina]

On Wed, 13 Jan 2016, Miroslav Benes wrote:

> >  {
> > -	int ret = 0;
> > -	unsigned long val;
> > -	struct klp_reloc *reloc;
> > +	int i, len, ret = 0;
> > +	char *secname;
> > +	const char *objname;
> >  
> >  	if (WARN_ON(!klp_is_object_loaded(obj)))
> >  		return -EINVAL;
> >  
> > -	if (WARN_ON(!obj->relocs))
> > -		return -EINVAL;
> > +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> >  
> >  	module_disable_ro(pmod);
> > +	/* For each klp rela section for this object */
> > +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> > +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> > +			continue;
> 
> One more thing. If the module does not specify it is a live patch module 
> in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live 
> patch module otherwise (it calls klp_register_patch in its init function), 

Side note: I think we should at least issue some light warning in such 
case anyway.

-- 
Jiri Kosina
SUSE Labs

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Miroslav Benes [13/01/16 10:19 +0100]:
>On Fri, 8 Jan 2016, Jessica Yu wrote:
>
>>  static int klp_write_object_relocations(struct module *pmod,
>>  					struct klp_object *obj)
>>  {
>> -	int ret = 0;
>> -	unsigned long val;
>> -	struct klp_reloc *reloc;
>> +	int i, len, ret = 0;
>> +	char *secname;
>> +	const char *objname;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>>
>>  	module_disable_ro(pmod);
>> +	/* For each klp rela section for this object */
>> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
>> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
>> +			continue;
>
>One more thing. If the module does not specify it is a live patch module
>in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live
>patch module otherwise (it calls klp_register_patch in its init function),
>the kernel crashes here. pmod->info is not initialized at all. This should
>be fixed. Perhaps the easiest would be to call
>klp_write_object_relocations() in klp_init_object_loaded() only if
>is_livepatch_module() returns true. Similar to a check for obj->relocs
>before.

Hm yes, that's a problem. To remedy this, I think it makes sense to
require all livepatch modules to identify themselves with the modinfo
attribute, since it is a very simple requirement. If some module calls
klp_register_patch() and it does not have the livepatch attribute,
klp_register_patch() can just return an error. We can call
is_livepatch_module() at the beginning of klp_register_patch(), and
proceed only if the check succeeds, since we'll then know that the
required structures have been properly initialized in the module
loader. What do you think?

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Miroslav Benes [13/01/16 10:19 +0100]:
>On Fri, 8 Jan 2016, Jessica Yu wrote:
>
>>  static int klp_write_object_relocations(struct module *pmod,
>>  					struct klp_object *obj)
>>  {
>> -	int ret = 0;
>> -	unsigned long val;
>> -	struct klp_reloc *reloc;
>> +	int i, len, ret = 0;
>> +	char *secname;
>> +	const char *objname;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>>
>>  	module_disable_ro(pmod);
>> +	/* For each klp rela section for this object */
>> +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
>> +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
>> +			continue;
>
>One more thing. If the module does not specify it is a live patch module
>in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live
>patch module otherwise (it calls klp_register_patch in its init function),
>the kernel crashes here. pmod->info is not initialized at all. This should
>be fixed. Perhaps the easiest would be to call
>klp_write_object_relocations() in klp_init_object_loaded() only if
>is_livepatch_module() returns true. Similar to a check for obj->relocs
>before.

Hm yes, that's a problem. To remedy this, I think it makes sense to
require all livepatch modules to identify themselves with the modinfo
attribute, since it is a very simple requirement. If some module calls
klp_register_patch() and it does not have the livepatch attribute,
klp_register_patch() can just return an error. We can call
is_livepatch_module() at the beginning of klp_register_patch(), and
proceed only if the check succeeds, since we'll then know that the
required structures have been properly initialized in the module
loader. What do you think?

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Miroslav Benes [12/01/16 17:40 +0100]:
>
>Hi Jessica,
>
>I walked through the series and it looks really nice. Others have already
>pointed out the issues I also found, so only few minor things below.
>
>First thing, could you copy&paste the information and reasoning from the
>cover letter to the changelogs where appropriate? It is very detailed and
>it would be a pity to lost it.

Thanks Miroslav! I'll do that.

>On Fri, 8 Jan 2016, Jessica Yu wrote:
>
>> 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);
>
>You left klp_write_module_reloc() in arch/s390/include/asm/livepatch.h I'm
>afraid. Anyway it would be really great if you managed to test the series
>on s390 somehow. Just to know that all the roadblocks are really gone.

Ah, thanks for catching that. I will also try testing the patchset on
s390x and report back.

>> -/*
>> - * external symbols are located outside the parent object (where the parent
>> - * object is either vmlinux or the kmod being patched).
>> - */
>> -static int klp_find_external_symbol(struct module *pmod, const char *name,
>> -				    unsigned long *addr)
>> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>>  {
>> -	const struct kernel_symbol *sym;
>> +	int i, len, ret = 0;
>> +	Elf_Rela *relas;
>> +	Elf_Sym *sym;
>> +	char *symname, *sym_objname;
>>
>> -	/* first, check if it's an exported symbol */
>> -	preempt_disable();
>> -	sym = find_symbol(name, NULL, NULL, true, true);
>> -	if (sym) {
>> -		*addr = sym->value;
>> -		preempt_enable();
>> -		return 0;
>> +	relas = (Elf_Rela *) relsec->sh_addr;
>> +	/* For each rela in this .klp.rel. section */
>> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
>> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
>> +		symname = pmod->core_strtab + sym->st_name;
>
>Maybe it would be better to use pmod->symtab and pmod->strtab everywhere.
>It should be the same, but core_* versions are only helpers used in
>load_module and friends. There is even a comment in
>include/linux/module.h.
>
>	/*
>	 * We keep the symbol and string tables for kallsyms.
>	 * The core_* fields below are temporary, loader-only (they
>	 * could really be discarded after module init).
>	 */
>
>We should respect that.

I admit I'm a bit confused by the comment, I can't seem to find where
core_symtab and core_strtab are purportedly discarded after module
init (perhaps I'm missing something?). IMO it sounds more like it's
describing mod->symtab and mod->strtab instead, because these are in
module init memory and are freed later. In any case, my reason for using
core_symtab is that the original symbol table (mod->symtab) is marked
with INIT_OFFSET_MASK in layout_symtab() (see kernel/module.c), and is
therefore in init memory. This memory is freed near the end of
do_init_module() with do_free_init(). Since core_symtab is in module core
memory, for livepatch modules I simply used core_symtab to hold a
full copy of the symbol table instead of the slimmed down version that
it was originally intended to hold.

Alternatively, we can tweak layout_symtab() to *not* mark the symtab
with INIT_OFFSET_MASK and put it in core memory instead. I think
either way will work, but maybe it is cleaner to do it this way
instead.

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Rusty Russell [11/01/16 11:55 +1030]:
>Hi Jessica,
>
>        Nice patch series.  Minor issues below, but they're really
>just nits which can be patched afterwards if you're getting sick of
>rebasing :)

Thanks Rusty!

>> +#ifdef CONFIG_LIVEPATCH
>> +/*
>> + * copy_module_elf - preserve Elf information about a module
>> + *
>> + * Copy relevant Elf information from the load_info struct.
>> + * Note: not all fields from the original load_info are
>> + * copied into mod->info.
>
>This makes me nervous, to have a struct which is half-initialized.
>
>Better would be to have a separate type for this, eg:
>
>struct livepatch_modinfo {
>        Elf_Ehdr hdr;
>        Elf_Shdr *sechdrs;
>        char *secstrings;
>        /* FIXME: Which of these do you need? */
>        struct {
>        	unsigned int sym, str, mod, vers, info, pcpu;
>        } index;
>};
>
>This also avoids an extra allocation as hdr is no longer a ptr.

Sure, sounds good.

>> +	/*
>> +	 * Update symtab's sh_addr to point to a valid
>> +	 * symbol table, as the temporary symtab in module
>> +	 * init memory will be freed
>> +	 */
>> +	mod->info->sechdrs[mod->info->index.sym].sh_addr = (unsigned long)mod->core_symtab;
>
>This comment is a bit misleading: it's actually pointing into the
>temporary module copy, which will be discarded.  The init section is
>slightly different...

Ah, perhaps I'm misunderstanding something..

Since copy_module_elf() is called after move_module(), my
understanding is that all the section sh_addr's should be pointing
to either module core memory or module init memory (instead of the
initial temporary copy of the module in info->hdr). Since the symbol
table section is marked with INIT_OFFSET_MASK, it will reside in
module init memory (and freed near the end of do_init_module()),
which is why I am updating the sh_addr here to point to core_symtab
instead. For livepatch modules, the core_symtab will be a complete
copy of the symbol table instead of the slimmed down version. Please
correct me if my understanding is incorrect.

Thanks for the patch review,
Jessica

Re: Documentation: livepatch: outline the Elf format of a livepatch module

[Jessica Yu]

+++ Petr Mladek [12/01/16 13:09 +0100]:
>Hi Jessica,
>
>first, thanks a lot for writing a documentation. It is really
>appreciated!
>
>To be honest, I am not sure if it makes sense to give feedback
>at this stage. It seems that there still will be some changes
>in the elf format.

Your feedback is appreciated regardless! :-)

>On Fri 2016-01-08 14:28:24, Jessica Yu wrote:
>> Document the special Elf sections and constants livepatch modules use.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  Documentation/livepatch/patch-module-format.txt | 106 ++++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>>  create mode 100644 Documentation/livepatch/patch-module-format.txt
>
>I would call this symbol-relocation.txt or so. It describes only this
>part of the patch format.
>
>>
>> diff --git a/Documentation/livepatch/patch-module-format.txt b/Documentation/livepatch/patch-module-format.txt
>> new file mode 100644
>> index 0000000..d825629
>> --- /dev/null
>> +++ b/Documentation/livepatch/patch-module-format.txt
>> @@ -0,0 +1,106 @@
>> +---------------------------
>> +Livepatch module Elf format
>> +---------------------------
>
>I would start with a description what symbols are relocated and why
>it needs to be done a special way.
>
>Also I would switch a bit the order of the sections below.
>We have relocation sections for each patched object. They are
>used when the object is loaded and patched. Each section has
>relocation information for symbols that are accessed from
>new versions of functions for the patched object.
>Hmm, we should add some generic documentation about
>LivePatching that would describe the design and used terms.
>
>
>> +This document outlines the special Elf constants and sections livepatch
>> +uses to patch both modules and the kernel (vmlinux).
>
>> +--------------------------
>> +1. Livepatch modinfo field
>> +--------------------------
>> +
>
>Please, express that livepatch must set the "livepatch" field to be
>indentified and loaded correctly.
>
>> +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
>
>s/kpatch/livepatch/ ;-)
>
>> +filename:		kpatch-meminfo.ko
>
>same here ;-)
>
>> +livepatch:		Y
>> +license:		GPL
>> +depends:
>> +vermagic:		4.3.0+ SMP mod_unload
>> +
>> +--------------------
>> +2. Livepatch symbols
>> +--------------------
>
>I would be more precise here. These are symbols that will get
>relocated by the livepatch framework.
>
>> +
>> +These are symbols marked with SHN_LIVEPATCH and their names are prefixed
>> +with the string ".klp.sym.${objname}.", where ${objname} is the
>
>We should describe the entire format. I mean
>.klp.sym.${objname}.[symname]. Also we should explain why symname is optional.
>
>> name of the
>> +"object" where symbol stems from (the name of a module, for example).
>> +A symbol's position (used to differentiate duplicate symbols within the
>> +same object) in its object is encoded in the Elf_Sym st_other field
>> +and accessed with the KLP_SYMPOS macro (see include/linux/livepatch.h)
>> +
>> +Livepatch symbols are manually resolved by livepatch, and are used in cases
>
>Please, what do you mean by "manually" resolved?

I think I was trying to emphasize that livepatch will take care of
symbol resolution (i.e. using klp_find_object_symbol()), but it
is sufficient enough to just say "livepatch resolves SHN_LIVEPATCH
symbols."

>> +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 symbol tables, and the symbol table is made accessible
>> +through module->core_symtab. For livepatch modules, core_symtab will
>> +contain an exact copy of the original symbol table as opposed to a stripped
>> +down version containing just the "core" symbols.
>> +
>> +-----------------------------------
>> +3. ".klp.rel." 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.rel.${objname}.", where ${objname} is the name of the
>> "object" being
>
>It seems that the full format is ".klp.rel.[objname].section_name"
>and section_name is not explained here.

Ah, thanks for catching that. I meant to say that they are *prefixed*
by the string ".klp.rel.[objname].", but like you said, it is much
better to explain the full format ".klp.rel.[objname].section_name"
here.

>
>> +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.rel.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
>> +  [30] .klp.rel.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
>> +  ...
>> +  [34] .klp.rel.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
>> +  [35] .klp.rel.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
>> +  [36] .klp.rel.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
>> +  [37] .klp.rel.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
>
>Hmm, it is SHF_RELA... I wonder if we want to rather use
>.klp.rela. prefix to be consistent.

Yeah, that looks better. I was actually just trying to make the
prefixes ".klp.sym." and ".klp.rel." the same length so the string
parsing code would be slightly simpler using KLP_TAG_LEN.

>> +loader can identify them and avoid treating them as regular SHT_RELA
>> +sections, since they are manually managed by livepatch.
>> +
>> +Since Elf information is preserved for livepatch modules (see Section 4), a
>> +klp relocation section can be applied simply by passing in the appropriate
>> +section index to apply_relocate_add() (in the module loader code), which
>> +then uses it to access the relocation section and apply the relocations.
>> +
>> +--------------------------------------------------------
>> +4. 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.
>
>We should add some notice also to the source code or commit message
>aboud why we preserve all the symbols for live patch.
>
>Great work.

Thanks for the review Petr!

Jessica

Re: Documentation: livepatch: outline the Elf format of a livepatch module

[Jessica Yu]

+++ Petr Mladek [12/01/16 13:09 +0100]:
>Hi Jessica,
>
>first, thanks a lot for writing a documentation. It is really
>appreciated!
>
>To be honest, I am not sure if it makes sense to give feedback
>at this stage. It seems that there still will be some changes
>in the elf format.

Your feedback is appreciated regardless! :-)

>On Fri 2016-01-08 14:28:24, Jessica Yu wrote:
>> Document the special Elf sections and constants livepatch modules use.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  Documentation/livepatch/patch-module-format.txt | 106 ++++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>>  create mode 100644 Documentation/livepatch/patch-module-format.txt
>
>I would call this symbol-relocation.txt or so. It describes only this
>part of the patch format.
>
>>
>> diff --git a/Documentation/livepatch/patch-module-format.txt b/Documentation/livepatch/patch-module-format.txt
>> new file mode 100644
>> index 0000000..d825629
>> --- /dev/null
>> +++ b/Documentation/livepatch/patch-module-format.txt
>> @@ -0,0 +1,106 @@
>> +---------------------------
>> +Livepatch module Elf format
>> +---------------------------
>
>I would start with a description what symbols are relocated and why
>it needs to be done a special way.
>
>Also I would switch a bit the order of the sections below.
>We have relocation sections for each patched object. They are
>used when the object is loaded and patched. Each section has
>relocation information for symbols that are accessed from
>new versions of functions for the patched object.
>Hmm, we should add some generic documentation about
>LivePatching that would describe the design and used terms.
>
>
>> +This document outlines the special Elf constants and sections livepatch
>> +uses to patch both modules and the kernel (vmlinux).
>
>> +--------------------------
>> +1. Livepatch modinfo field
>> +--------------------------
>> +
>
>Please, express that livepatch must set the "livepatch" field to be
>indentified and loaded correctly.
>
>> +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
>
>s/kpatch/livepatch/ ;-)
>
>> +filename:		kpatch-meminfo.ko
>
>same here ;-)
>
>> +livepatch:		Y
>> +license:		GPL
>> +depends:
>> +vermagic:		4.3.0+ SMP mod_unload
>> +
>> +--------------------
>> +2. Livepatch symbols
>> +--------------------
>
>I would be more precise here. These are symbols that will get
>relocated by the livepatch framework.
>
>> +
>> +These are symbols marked with SHN_LIVEPATCH and their names are prefixed
>> +with the string ".klp.sym.${objname}.", where ${objname} is the
>
>We should describe the entire format. I mean
>.klp.sym.${objname}.[symname]. Also we should explain why symname is optional.
>
>> name of the
>> +"object" where symbol stems from (the name of a module, for example).
>> +A symbol's position (used to differentiate duplicate symbols within the
>> +same object) in its object is encoded in the Elf_Sym st_other field
>> +and accessed with the KLP_SYMPOS macro (see include/linux/livepatch.h)
>> +
>> +Livepatch symbols are manually resolved by livepatch, and are used in cases
>
>Please, what do you mean by "manually" resolved?

I think I was trying to emphasize that livepatch will take care of
symbol resolution (i.e. using klp_find_object_symbol()), but it
is sufficient enough to just say "livepatch resolves SHN_LIVEPATCH
symbols."

>> +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 symbol tables, and the symbol table is made accessible
>> +through module->core_symtab. For livepatch modules, core_symtab will
>> +contain an exact copy of the original symbol table as opposed to a stripped
>> +down version containing just the "core" symbols.
>> +
>> +-----------------------------------
>> +3. ".klp.rel." 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.rel.${objname}.", where ${objname} is the name of the
>> "object" being
>
>It seems that the full format is ".klp.rel.[objname].section_name"
>and section_name is not explained here.

Ah, thanks for catching that. I meant to say that they are *prefixed*
by the string ".klp.rel.[objname].", but like you said, it is much
better to explain the full format ".klp.rel.[objname].section_name"
here.

>
>> +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.rel.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
>> +  [30] .klp.rel.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
>> +  ...
>> +  [34] .klp.rel.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
>> +  [35] .klp.rel.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
>> +  [36] .klp.rel.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
>> +  [37] .klp.rel.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
>
>Hmm, it is SHF_RELA... I wonder if we want to rather use
>.klp.rela. prefix to be consistent.

Yeah, that looks better. I was actually just trying to make the
prefixes ".klp.sym." and ".klp.rel." the same length so the string
parsing code would be slightly simpler using KLP_TAG_LEN.

>> +loader can identify them and avoid treating them as regular SHT_RELA
>> +sections, since they are manually managed by livepatch.
>> +
>> +Since Elf information is preserved for livepatch modules (see Section 4), a
>> +klp relocation section can be applied simply by passing in the appropriate
>> +section index to apply_relocate_add() (in the module loader code), which
>> +then uses it to access the relocation section and apply the relocations.
>> +
>> +--------------------------------------------------------
>> +4. 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.
>
>We should add some notice also to the source code or commit message
>aboud why we preserve all the symbols for live patch.
>
>Great work.

Thanks for the review Petr!

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [11/01/16 21:05 -0600]:
>On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
>> >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>> >> include/linux/livepatch.h        |  33 +++++--------
>> >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>> >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
>> >>--- a/arch/x86/kernel/livepatch.c
>> >>+++ /dev/null
>> >>@@ -1,70 +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/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)
>> >>-{
>> >>-	size_t size = 4;
>> >>-	unsigned long val;
>> >>-	unsigned long core = (unsigned long)mod->core_layout.base;
>> >>-	unsigned long core_size = mod->core_layout.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;
>> >>-
>> >>-	return probe_kernel_write((void *)loc, &val, size);
>> >>-}
>> >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> >>index a882865..2f12ce7 100644
>> >>--- a/include/linux/livepatch.h
>> >>+++ b/include/linux/livepatch.h
>> >>@@ -65,27 +65,8 @@ struct klp_func {
>> >> };
>> >>
>> >> /**
>> >>- * struct klp_reloc - relocation structure for live patching
>> >>- * @loc:	address where the relocation will be written
>> >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
>> >>- * @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 {
>> >>-	unsigned long loc;
>> >>-	unsigned long sympos;
>> >>-	unsigned long type;
>> >>-	const char *name;
>> >>-	int addend;
>> >>-	int external;
>> >>-};
>> >>-
>> >>-/**
>> >>  * 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
>> >>  * @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 +76,6 @@ struct klp_reloc {
>> >> struct klp_object {
>> >> 	/* external */
>> >> 	const char *name;
>> >>-	struct klp_reloc *relocs;
>> >> 	struct klp_func *funcs;
>> >>
>> >> 	/* internal */
>> >>@@ -123,6 +103,19 @@ struct klp_patch {
>> >> 	enum klp_state state;
>> >> };
>> >>
>> >>+/*
>> >>+ * Livepatch-specific symbols and relocation
>> >>+ * sections are prefixed with a tag:
>> >>+ * .klp.rel. for relocation sections
>> >>+ * .klp.sym. for livepatch symbols
>> >>+ */
>> >>+#define KLP_TAG_LEN 9
>> >>+/*
>> >>+ * Livepatch-specific bits for specifying symbol
>> >>+ * positions in the Elf_Sym st_other field
>> >>+ */
>> >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
>> >>+
>> >
>> >Can st_value be used instead?  I think we ended up deciding that would
>> >be better:
>> >
>> > https://lkml.kernel.org/g/20151210213328.GA6553@packer-debian-8-amd64.digitalocean.com
>> >
>> >Because:
>> >
>> >- st_value is easily viewable in readelf
>> >- st_other has some arch-specific uses
>> >
>> >And another reason not previously discussed:
>> >
>> >- st_other is an unsigned char, which limits sympos to values < 64
>>
>> I originally wanted to encode the symbol position in st_value, but
>> I've discovered that since st_value is overwritten once the
>> symbol is (first) resolved, we no longer have the symbol position if we
>> need to resolve the symbols again (so we wouldn't be able to resolve
>> them). This could happen when we patch a module that loads and
>> unloads more than once for example.
>
>Ah, good point.
>
>> I chose st_other since it isn't touched in s390x kernel code nor in
>> x86 kernel code (it does get used in the x86 userspace reloc tool in
>> arch/x86/tools, which is why I left the first two bits alone for
>> ELF_ST_VISIBILITY, and the rest had undefined usage). However this
>> isn't the best approach and I'm afraid of stepping on other arch's
>> toes in future patches. Perhaps there is another field where we can
>> stuff the sympos in? st_size for instance doesn't seem to be touched
>> anywhere in the module loader. I don't know if I'd want to stuff
>> sympos in the sym name, there's enough going on there..
>
>I think st_other still isn't going to work because of possible arch
>conflicts and because of its small size (6 bits) otherwise.
>
>st_size could work.  It doesn't _seem_ to be used by the module code,
>though it's hard to confirm that for all arches.  But it makes me a
>little nervous to override that field.  For example, might it confuse
>some user-space tools out there, or the linker?
>
>Maybe encoding it in the sym name is the safest bet.  Why not, we've
>already got a bunch of other stuff there anyway :-)  One idea would be
>to append it with ',<sympos>' to be consistent with the sysfs entries:
>
>  .klp.sym.vmlinux.symname,1

Sure, as long as commas aren't allowed in symbol names, that will work. :-)

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [11/01/16 21:05 -0600]:
>On Mon, Jan 11, 2016 at 05:35:13PM -0500, Jessica Yu wrote:
>> +++ Josh Poimboeuf [11/01/16 15:33 -0600]:
>> >On Fri, Jan 08, 2016 at 02:28:22PM -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      |  70 ---------------------------
>> >> include/linux/livepatch.h        |  33 +++++--------
>> >> kernel/livepatch/core.c          | 101 +++++++++++++++++++--------------------
>> >> 5 files changed, 62 insertions(+), 145 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 92fc1a5..0000000
>> >>--- a/arch/x86/kernel/livepatch.c
>> >>+++ /dev/null
>> >>@@ -1,70 +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/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)
>> >>-{
>> >>-	size_t size = 4;
>> >>-	unsigned long val;
>> >>-	unsigned long core = (unsigned long)mod->core_layout.base;
>> >>-	unsigned long core_size = mod->core_layout.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;
>> >>-
>> >>-	return probe_kernel_write((void *)loc, &val, size);
>> >>-}
>> >>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> >>index a882865..2f12ce7 100644
>> >>--- a/include/linux/livepatch.h
>> >>+++ b/include/linux/livepatch.h
>> >>@@ -65,27 +65,8 @@ struct klp_func {
>> >> };
>> >>
>> >> /**
>> >>- * struct klp_reloc - relocation structure for live patching
>> >>- * @loc:	address where the relocation will be written
>> >>- * @sympos:	position in kallsyms to disambiguate symbols (optional)
>> >>- * @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 {
>> >>-	unsigned long loc;
>> >>-	unsigned long sympos;
>> >>-	unsigned long type;
>> >>-	const char *name;
>> >>-	int addend;
>> >>-	int external;
>> >>-};
>> >>-
>> >>-/**
>> >>  * 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
>> >>  * @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 +76,6 @@ struct klp_reloc {
>> >> struct klp_object {
>> >> 	/* external */
>> >> 	const char *name;
>> >>-	struct klp_reloc *relocs;
>> >> 	struct klp_func *funcs;
>> >>
>> >> 	/* internal */
>> >>@@ -123,6 +103,19 @@ struct klp_patch {
>> >> 	enum klp_state state;
>> >> };
>> >>
>> >>+/*
>> >>+ * Livepatch-specific symbols and relocation
>> >>+ * sections are prefixed with a tag:
>> >>+ * .klp.rel. for relocation sections
>> >>+ * .klp.sym. for livepatch symbols
>> >>+ */
>> >>+#define KLP_TAG_LEN 9
>> >>+/*
>> >>+ * Livepatch-specific bits for specifying symbol
>> >>+ * positions in the Elf_Sym st_other field
>> >>+ */
>> >>+#define KLP_SYMPOS(o) (o >> 2) & 0xff
>> >>+
>> >
>> >Can st_value be used instead?  I think we ended up deciding that would
>> >be better:
>> >
>> > https://lkml.kernel.org/g/20151210213328.GA6553-N0bYjD2NfQ6k4hzjq3hgyGTy53QMssKEsp+A89P3RPuQWHG76I6BsA@public.gmane.org
>> >
>> >Because:
>> >
>> >- st_value is easily viewable in readelf
>> >- st_other has some arch-specific uses
>> >
>> >And another reason not previously discussed:
>> >
>> >- st_other is an unsigned char, which limits sympos to values < 64
>>
>> I originally wanted to encode the symbol position in st_value, but
>> I've discovered that since st_value is overwritten once the
>> symbol is (first) resolved, we no longer have the symbol position if we
>> need to resolve the symbols again (so we wouldn't be able to resolve
>> them). This could happen when we patch a module that loads and
>> unloads more than once for example.
>
>Ah, good point.
>
>> I chose st_other since it isn't touched in s390x kernel code nor in
>> x86 kernel code (it does get used in the x86 userspace reloc tool in
>> arch/x86/tools, which is why I left the first two bits alone for
>> ELF_ST_VISIBILITY, and the rest had undefined usage). However this
>> isn't the best approach and I'm afraid of stepping on other arch's
>> toes in future patches. Perhaps there is another field where we can
>> stuff the sympos in? st_size for instance doesn't seem to be touched
>> anywhere in the module loader. I don't know if I'd want to stuff
>> sympos in the sym name, there's enough going on there..
>
>I think st_other still isn't going to work because of possible arch
>conflicts and because of its small size (6 bits) otherwise.
>
>st_size could work.  It doesn't _seem_ to be used by the module code,
>though it's hard to confirm that for all arches.  But it makes me a
>little nervous to override that field.  For example, might it confuse
>some user-space tools out there, or the linker?
>
>Maybe encoding it in the sym name is the safest bet.  Why not, we've
>already got a bunch of other stuff there anyway :-)  One idea would be
>to append it with ',<sympos>' to be consistent with the sysfs entries:
>
>  .klp.sym.vmlinux.symname,1

Sure, as long as commas aren't allowed in symbol names, that will work. :-)

Re: livepatch: reuse module loader code to write relocations

[Miroslav Benes]

On Wed, 13 Jan 2016, Jessica Yu wrote:

> > Maybe it would be better to use pmod->symtab and pmod->strtab everywhere.
> > It should be the same, but core_* versions are only helpers used in
> > load_module and friends. There is even a comment in
> > include/linux/module.h.
> > 
> > 	/*
> > 	 * We keep the symbol and string tables for kallsyms.
> > 	 * The core_* fields below are temporary, loader-only (they
> > 	 * could really be discarded after module init).
> > 	 */
> > 
> > We should respect that.
> 
> I admit I'm a bit confused by the comment, I can't seem to find where
> core_symtab and core_strtab are purportedly discarded after module
> init (perhaps I'm missing something?). IMO it sounds more like it's
> describing mod->symtab and mod->strtab instead, because these are in
> module init memory and are freed later. 

I think it just says that core_* symbols are used as temporary tables 
during module loading. They are not discarded anywhere but they could be 
(and maybe they'll be in the future). So it is better not to depend on 
them.

> In any case, my reason for using
> core_symtab is that the original symbol table (mod->symtab) is marked
> with INIT_OFFSET_MASK in layout_symtab() (see kernel/module.c), and is
> therefore in init memory. This memory is freed near the end of
> do_init_module() with do_free_init(). Since core_symtab is in module core
> memory, for livepatch modules I simply used core_symtab to hold a
> full copy of the symbol table instead of the slimmed down version that
> it was originally intended to hold.

But both mod->symtab and mod->strtab are changed to point to their core_ 
versions right before do_free_init is called in do_init_module. So they 
should be the same. My remark was more of an academic question. I believe 
it is not a functional thing, just the matter of taste. But maybe I am 
missing something.

> Alternatively, we can tweak layout_symtab() to *not* mark the symtab
> with INIT_OFFSET_MASK and put it in core memory instead. I think
> either way will work, but maybe it is cleaner to do it this way
> instead.

Yeah, I wouldn't do this. core_* symbols are ok from functional point of 
view.

Thanks,
Miroslav

Re: livepatch: reuse module loader code to write relocations

[Miroslav Benes]

On Wed, 13 Jan 2016, Jessica Yu wrote:

> +++ Miroslav Benes [13/01/16 10:19 +0100]:
> > On Fri, 8 Jan 2016, Jessica Yu wrote:
> > 
> > >  static int klp_write_object_relocations(struct module *pmod,
> > >  					struct klp_object *obj)
> > >  {
> > > -	int ret = 0;
> > > -	unsigned long val;
> > > -	struct klp_reloc *reloc;
> > > +	int i, len, ret = 0;
> > > +	char *secname;
> > > +	const char *objname;
> > > 
> > >  	if (WARN_ON(!klp_is_object_loaded(obj)))
> > >  		return -EINVAL;
> > > 
> > > -	if (WARN_ON(!obj->relocs))
> > > -		return -EINVAL;
> > > +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > 
> > >  	module_disable_ro(pmod);
> > > +	/* For each klp rela section for this object */
> > > +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> > > +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> > > +			continue;
> > 
> > One more thing. If the module does not specify it is a live patch module
> > in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live
> > patch module otherwise (it calls klp_register_patch in its init function),
> > the kernel crashes here. pmod->info is not initialized at all. This should
> > be fixed. Perhaps the easiest would be to call
> > klp_write_object_relocations() in klp_init_object_loaded() only if
> > is_livepatch_module() returns true. Similar to a check for obj->relocs
> > before.
> 
> Hm yes, that's a problem. To remedy this, I think it makes sense to
> require all livepatch modules to identify themselves with the modinfo
> attribute, since it is a very simple requirement. If some module calls
> klp_register_patch() and it does not have the livepatch attribute,
> klp_register_patch() can just return an error. We can call
> is_livepatch_module() at the beginning of klp_register_patch(), and
> proceed only if the check succeeds, since we'll then know that the
> required structures have been properly initialized in the module
> loader. What do you think?

This is similar to what Jiri proposed in his mail. It is up to you. Both 
ways (the warning and the check, or what you propose) are fine.

Miroslav

Re: module: preserve Elf information for livepatch modules

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> +++ Rusty Russell [11/01/16 11:55 +1030]:
>>This comment is a bit misleading: it's actually pointing into the
>>temporary module copy, which will be discarded.  The init section is
>>slightly different...
>
> Ah, perhaps I'm misunderstanding something..
>
> Since copy_module_elf() is called after move_module(), my
> understanding is that all the section sh_addr's should be pointing
> to either module core memory or module init memory (instead of the
> initial temporary copy of the module in info->hdr).

You're absolutely correct; I'd forgotten that particular kallsyms twist.

Sorry for the noise!
Rusty.

Re: module: preserve Elf information for livepatch modules

[Rusty Russell]

Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> +++ Rusty Russell [11/01/16 11:55 +1030]:
>>This comment is a bit misleading: it's actually pointing into the
>>temporary module copy, which will be discarded.  The init section is
>>slightly different...
>
> Ah, perhaps I'm misunderstanding something..
>
> Since copy_module_elf() is called after move_module(), my
> understanding is that all the section sh_addr's should be pointing
> to either module core memory or module init memory (instead of the
> initial temporary copy of the module in info->hdr).

You're absolutely correct; I'd forgotten that particular kallsyms twist.

Sorry for the noise!
Rusty.