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

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

[Jessica Yu]

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

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

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

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

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

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

Patches based on linux-next.

Previous patchset (v3) found here:
https://lkml.kernel.org/g/1452281304-28618-1-git-send-email-jeyu@redhat.com

v4:
 - Way more error checking for all the string manipulation on
   livepatch symbol names and sections.
 - Handle error conditions such as loading a klp module on a
   !CONFIG_LIVEPATCH kernel
 - Don't encode sympos in a symbol's st_other field. Instead, append it
   to the symbol name in the form .klp.sym.objname.symname,sympos
 - Instead of a half initialized copy of the load_info struct in
   mod->info, define a livepatch specific struct (klp_modinfo) instead
   that contains just the needed Elf info.
 - Much more detailed documentation about patch module requirements
   and the module elf format.

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 Elf format and requirements for
    patch modules

 Documentation/livepatch/module-elf-format.txt | 311 ++++++++++++++++++++++++++
 arch/s390/include/asm/livepatch.h             |   7 -
 arch/s390/kernel/module.c                     |   7 +-
 arch/x86/include/asm/livepatch.h              |   2 -
 arch/x86/kernel/Makefile                      |   1 -
 arch/x86/kernel/livepatch.c                   |  70 ------
 include/linux/livepatch.h                     |  30 +--
 include/linux/module.h                        |  25 +++
 include/uapi/linux/elf.h                      |  10 +-
 kernel/livepatch/core.c                       | 283 ++++++++++++++++++-----
 kernel/module.c                               | 133 ++++++++++-
 samples/livepatch/livepatch-sample.c          |   1 +
 12 files changed, 712 insertions(+), 168 deletions(-)
 create mode 100644 Documentation/livepatch/module-elf-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

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

[Jessica Yu]

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

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

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

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

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

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

Patches based on linux-next.

Previous patchset (v3) found here:
https://lkml.kernel.org/g/1452281304-28618-1-git-send-email-jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

v4:
 - Way more error checking for all the string manipulation on
   livepatch symbol names and sections.
 - Handle error conditions such as loading a klp module on a
   !CONFIG_LIVEPATCH kernel
 - Don't encode sympos in a symbol's st_other field. Instead, append it
   to the symbol name in the form .klp.sym.objname.symname,sympos
 - Instead of a half initialized copy of the load_info struct in
   mod->info, define a livepatch specific struct (klp_modinfo) instead
   that contains just the needed Elf info.
 - Much more detailed documentation about patch module requirements
   and the module elf format.

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 Elf format and requirements for
    patch modules

 Documentation/livepatch/module-elf-format.txt | 311 ++++++++++++++++++++++++++
 arch/s390/include/asm/livepatch.h             |   7 -
 arch/s390/kernel/module.c                     |   7 +-
 arch/x86/include/asm/livepatch.h              |   2 -
 arch/x86/kernel/Makefile                      |   1 -
 arch/x86/kernel/livepatch.c                   |  70 ------
 include/linux/livepatch.h                     |  30 +--
 include/linux/module.h                        |  25 +++
 include/uapi/linux/elf.h                      |  10 +-
 kernel/livepatch/core.c                       | 283 ++++++++++++++++++-----
 kernel/module.c                               | 133 ++++++++++-
 samples/livepatch/livepatch-sample.c          |   1 +
 12 files changed, 712 insertions(+), 168 deletions(-)
 create mode 100644 Documentation/livepatch/module-elf-format.txt
 delete mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

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

[Jessica Yu]

Livepatch manages its own relocation sections and symbols in order to be
able to reuse module loader code to write relocations. This removes
livepatch's dependence on separate "dynrela" sections to write relocations
and also allows livepatch to patch modules that are not yet loaded.

The livepatch Elf relocation section flag (SHF_RELA_LIVEPATCH),
and symbol section index (SHN_LIVEPATCH) allow both livepatch and the
module loader to identity livepatch relocation sections and livepatch
symbols.

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

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

The values of these Elf constants were selected from OS-specific
ranges according to the definitions from glibc.

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 v4 1/6] Elf: add livepatch-specific Elf constants

[Jessica Yu]

Livepatch manages its own relocation sections and symbols in order to be
able to reuse module loader code to write relocations. This removes
livepatch's dependence on separate "dynrela" sections to write relocations
and also allows livepatch to patch modules that are not yet loaded.

The livepatch Elf relocation section flag (SHF_RELA_LIVEPATCH),
and symbol section index (SHN_LIVEPATCH) allow both livepatch and the
module loader to identity livepatch relocation sections and livepatch
symbols.

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

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

The values of these Elf constants were selected from OS-specific
ranges according to the definitions from glibc.

Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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 v4 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. Persist copies of the
original symbol table and string table.

Livepatch manages its own relocation sections in order to reuse module
loader code to write relocations. Livepatch modules must preserve Elf
information such as section indices in order to apply livepatch relocation
sections using the module loader's apply_relocate_add() function.

In order to apply livepatch relocation sections, livepatch modules must
keep a complete copy of their original symbol table in memory. Normally, a
stripped down copy of a module's symbol table (containing only "core"
symbols) is made available through module->core_symtab. But for livepatch
modules, the symbol table copied into memory on module load must be exactly
the same as the symbol table produced when the patch module was compiled.
This is because the relocations in each livepatch relocation section refer
to their respective symbols with their symbol indices, and the original
symbol indices (and thus the symtab ordering) must be preserved in order
for apply_relocate_add() to find the right symbol.

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

diff --git a/include/linux/module.h b/include/linux/module.h
index 4560d8f..58e6200 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -324,6 +324,15 @@ struct module_layout {
 #define __module_layout_align
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+struct klp_modinfo {
+	Elf_Ehdr hdr;
+	Elf_Shdr *sechdrs;
+	char *secstrings;
+	unsigned int symndx;
+};
+#endif
+
 struct module {
 	enum module_state state;
 
@@ -455,7 +464,11 @@ struct module {
 #endif
 
 #ifdef CONFIG_LIVEPATCH
+	bool klp; /* Is this a livepatch module? */
 	bool klp_alive;
+
+	/* Elf information */
+	struct klp_modinfo *klp_info;
 #endif
 
 #ifdef CONFIG_MODULE_UNLOAD
@@ -629,6 +642,18 @@ static inline bool module_requested_async_probing(struct module *module)
 	return module && module->async_probe_requested;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return mod->klp;
+}
+#else /* !CONFIG_LIVEPATCH */
+static inline bool is_livepatch_module(struct module *mod)
+{
+	return false;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 #else /* !CONFIG_MODULES... */
 
 /* Given an address, look for it in the exception tables. */
diff --git a/kernel/module.c b/kernel/module.c
index 71c77ed..9c16eb2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1970,6 +1970,82 @@ static void module_enable_nx(const struct module *mod) { }
 static void module_disable_nx(const struct module *mod) { }
 #endif
 
+#ifdef CONFIG_LIVEPATCH
+/*
+ * Persist Elf information about a module. Copy the Elf header,
+ * section header table, section string table, and symtab section
+ * index from info to mod->klp_info.
+ */
+static int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	unsigned int size, symndx;
+	int ret = 0;
+
+	size = sizeof(*mod->klp_info);
+	mod->klp_info = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info == NULL)
+		return -ENOMEM;
+
+	/* Elf header */
+	size = sizeof(Elf_Ehdr);
+	memcpy(&mod->klp_info->hdr, info->hdr, size);
+
+	/* Elf section header table */
+	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
+	mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info->sechdrs == NULL) {
+		ret = -ENOMEM;
+		goto free_info;
+	}
+	memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
+
+	/* Elf section name string table */
+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+	mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info->secstrings == NULL) {
+		ret = -ENOMEM;
+		goto free_sechdrs;
+	}
+	memcpy(mod->klp_info->secstrings, info->secstrings, size);
+
+	/* Elf symbol section index */
+	symndx = info->index.sym;
+	mod->klp_info->symndx = symndx;
+
+	/*
+	 * For livepatch modules, core_symtab is a complete copy
+	 * of the original symbol table. Adjust sh_addr to point
+	 * to core_symtab since the copy of the symtab in module
+	 * init memory is freed at the end of do_init_module().
+	 */
+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_symtab;
+
+	return ret;
+
+free_sechdrs:
+	kfree(mod->klp_info->sechdrs);
+free_info:
+	kfree(mod->klp_info);
+	return ret;
+}
+
+static void free_module_elf(struct module *mod)
+{
+	kfree(mod->klp_info->sechdrs);
+	kfree(mod->klp_info->secstrings);
+	kfree(mod->klp_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);
@@ -2008,6 +2084,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. */
@@ -2123,6 +2203,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			       (long)sym[i].st_value);
 			break;
 
+		case SHN_LIVEPATCH:
+			/* Livepatch symbols are resolved by livepatch */
+			break;
+
 		case SHN_UNDEF:
 			ksym = resolve_symbol_wait(mod, info, name);
 			/* Ok if resolved.  */
@@ -2171,6 +2255,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
+		/* Livepatch 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);
@@ -2466,7 +2554,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;
@@ -2509,7 +2597,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];
@@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 	return 0;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	mod->klp = get_modinfo(info, "livepatch") ? true : false;
+
+	return 0;
+}
+#else /* !CONFIG_LIVEPATCH */
+static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
+{
+	if (get_modinfo(info, "livepatch"))
+		return -ENOEXEC;
+
+	return 0;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 /* Sets info->hdr and info->len. */
 static int copy_module_from_fd(int fd, struct load_info *info)
 {
@@ -2859,6 +2964,10 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+	err = find_livepatch_modinfo(mod, info);
+	if (err)
+		return err;
+
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
 
@@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
 	 */
 	current->flags &= ~PF_USED_ASYNC;
 
+#ifdef CONFIG_KALLSYMS
+	/* Make symtab and strtab available prior to module init call */
+	mod->num_symtab = mod->core_num_syms;
+	mod->symtab = mod->core_symtab;
+	mod->strtab = mod->core_strtab;
+#endif
 	do_mod_ctors(mod);
 	/* Start the module */
 	if (mod->init != NULL)
@@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
 	/* Drop initial reference. */
 	module_put(mod);
 	trim_init_extable(mod);
-#ifdef CONFIG_KALLSYMS
-	mod->num_symtab = mod->core_num_syms;
-	mod->symtab = mod->core_symtab;
-	mod->strtab = mod->core_strtab;
-#endif
 	mod_tree_remove_init(mod);
 	disable_ro_nx(&mod->init_layout);
 	module_arch_freeing_init(mod);
@@ -3522,6 +3632,13 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto bug_cleanup;
 
+	/* For livepatch modules, save Elf info from load_info struct */
+	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);
 
@@ -3530,6 +3647,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 v4 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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index 7873e17..2bb6c68 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -51,6 +51,11 @@ 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 +430,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 v4 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. Specifically, reuse
the apply_relocate_add() function in the module loader to write relocations
instead of duplicating functionality in livepatch's arch-dependent
klp_write_module_reloc() function.

In order to accomplish this, livepatch modules manage their own relocation
sections (marked with the SHF_RELA_LIVEPATCH section flag) and
livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
index). To apply livepatch relocation sections, 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 and the s390
klp_write_module_reloc() function stub. They are no longer needed since
relocation work has been offloaded to module loader.

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

diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
index a52b6cc..350a751 100644
--- a/arch/s390/include/asm/livepatch.h
+++ b/arch/s390/include/asm/livepatch.h
@@ -25,13 +25,6 @@ static inline int klp_check_compiler_support(void)
 	return 0;
 }
 
-static inline int klp_write_module_reloc(struct module *mod, unsigned long
-		type, unsigned long loc, unsigned long value)
-{
-	/* not supported yet */
-	return -ENOSYS;
-}
-
 static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 {
 	regs->psw.addr = ip;
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index e795f52..d7c2b57 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 fdd5f1c..1a40a72 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,16 @@ struct klp_patch {
 	enum klp_state state;
 };
 
+/*
+ * Livepatch symbol and relocation section prefixes:
+ * ".klp.rela." for relocation sections
+ * ".klp.sym." for livepatch symbols
+ */
+#define KLP_SYM_PREFIX ".klp.sym."
+#define KLP_SYM_PREFIX_LEN 9
+#define KLP_RELASEC_PREFIX ".klp.rela."
+#define KLP_RELASEC_PREFIX_LEN 10
+
 #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 7aa975d..c1fe57c 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>
 
 /**
@@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
 	return !obj->name || obj->mod;
 }
 
+/*
+ * Check if a livepatch symbol is formatted properly.
+ *
+ * See Documentation/livepatch/module-elf-format.txt for a
+ * detailed outline of requirements.
+ */
+static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
+{
+	size_t len;
+	char *s, *objname, *symname;
+
+	if (sym->st_shndx != SHN_LIVEPATCH)
+		return -EINVAL;
+
+	/*
+	 * Livepatch symbol names must follow this format:
+	 * .klp.sym.objname.symbol_name,sympos
+	 */
+	s = pmod->strtab + sym->st_name;
+	/* [.klp.sym.]objname.symbol_name,sympos */
+	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
+		return -EINVAL;
+
+	/* .klp.sym.[objname].symbol_name,sympos */
+	objname = s + KLP_SYM_PREFIX_LEN;
+	len = strcspn(objname, ".");
+	if (!(len > 0))
+		return -EINVAL;
+
+	/* .klp.sym.objname.symbol_name,[sympos] */
+	if (!strchr(s, ','))
+		return -EINVAL;
+
+	/* .klp.sym.objname.[symbol_name],sympos */
+	symname = objname + len + 1;
+	len = strcspn(symname, ",");
+	if (!(len > 0))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Check if a livepatch relocation section is formatted properly.
+ *
+ * See Documentation/livepatch/module-elf-format.txt for a
+ * detailed outline of requirements.
+ */
+static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
+{
+	char *secname;
+	size_t len;
+
+	secname = pmod->klp_info->secstrings + relasec->sh_name;
+	/* [.klp.rela.]objname.section_name */
+	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
+				KLP_RELASEC_PREFIX_LEN))
+		return -EINVAL;
+
+	/* .klp.rela.[objname].section_name */
+	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
+	if (!(len > 0))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Check if obj->name matches the objname encoded in the rela
+ * section name (.klp.rela.[objname].section_name)
+ *
+ * Must pass klp_check_relasec_format() before calling this.
+ */
+static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
+				       struct klp_object *obj)
+{
+	size_t len;
+	const char *obj_objname, *sec_objname, *secname;
+
+	secname = pmod->klp_info->secstrings + relasec->sh_name;
+	/* .klp.rela.[objname].section_name */
+	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
+	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
+
+	/* Get length of the objname encoded in the section name */
+	len = strcspn(sec_objname, ".");
+
+	if (strlen(obj_objname) != len)
+		return false;
+
+	return strncmp(sec_objname, obj_objname, len) ? false : true;
+}
+
+/*
+ * klp_get_* helper functions
+ *
+ * klp_get_* functions extract different components of the name
+ * of a livepatch symbol. The full symbol name from the strtab
+ * is passed in as parameter @s, and @result is filled in with
+ * the extracted component.
+ *
+ * These functions assume a correctly formatted symbol and the
+ * klp_check_symbol_format() test *must* pass before calling any
+ * of these functions.
+ */
+
+/* .klp.sym.[objname].symbol_name,sympos */
+static int klp_get_sym_objname(char *s, char **result)
+{
+	size_t len;
+	char *objname, *objname_start;
+
+	/* .klp.sym.[objname].symbol_name,sympos */
+	objname_start = s + KLP_SYM_PREFIX_LEN;
+	len = strcspn(objname_start, ".");
+	objname = kstrndup(objname_start, len, GFP_KERNEL);
+	if (objname == NULL)
+		return -ENOMEM;
+
+	/* klp_find_object_symbol() treats NULL as vmlinux */
+	if (!strcmp(objname, "vmlinux")) {
+		*result = NULL;
+		kfree(objname);
+	} else
+		*result = objname;
+
+	return 0;
+}
+
+/* .klp.sym.objname.[symbol_name],sympos */
+static int klp_get_symbol_name(char *s, char **result)
+{
+	size_t len;
+	char *objname, *symname;
+
+	/* .klp.sym.[objname].symbol_name,sympos */
+	objname = s + KLP_SYM_PREFIX_LEN;
+	len = strcspn(objname, ".");
+
+	/* .klp.sym.objname.[symbol_name],sympos */
+	symname = objname + len + 1;
+	len = strcspn(symname, ",");
+
+	*result = kstrndup(symname, len, GFP_KERNEL);
+	if (*result == NULL)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/* .klp.sym.objname.symbol_name,[sympos] */
+static int klp_get_sympos(char *s, unsigned long *result)
+{
+	char *sympos;
+
+	/* .klp.sym.symbol_name,[sympos] */
+	sympos = strchr(s, ',') + 1;
+	return kstrtol(sympos, 10, result);
+}
+
 /* sets obj->mod if object is not vmlinux and module is found */
 static void klp_find_object_module(struct klp_object *obj)
 {
@@ -204,74 +367,83 @@ 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 *relasec, struct module *pmod)
 {
-	const struct kernel_symbol *sym;
-
-	/* 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;
+	int i, ret = 0;
+	Elf_Rela *relas;
+	Elf_Sym *sym;
+	char *s, *symbol_name, *sym_objname;
+	unsigned long sympos;
+
+	relas = (Elf_Rela *) relasec->sh_addr;
+	/* For each rela in this .klp.rela. section */
+	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
+		sym = pmod->symtab + ELF_R_SYM(relas[i].r_info);
+
+		/* Check if the symbol is formatted correctly */
+		ret = klp_check_symbol_format(pmod, sym);
+		if (ret)
+			goto out;
+		/* Format: .klp.sym.objname.symbol_name,sympos */
+		s = pmod->strtab + sym->st_name;
+		ret = klp_get_symbol_name(s, &symbol_name);
+		if (ret)
+			goto out;
+		ret = klp_get_sym_objname(s, &sym_objname);
+		if (ret)
+			goto free_symbol_name;
+		ret = klp_get_sympos(s, &sympos);
+		if (ret)
+			goto free_objname;
+
+		ret = klp_find_object_symbol(sym_objname, symbol_name, sympos,
+					     (unsigned long *) &sym->st_value);
+free_objname:
+		kfree(sym_objname);
+free_symbol_name:
+		kfree(symbol_name);
+		if (ret)
+			goto out;
 	}
-	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);
+out:
+	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, ret = 0;
+	Elf_Shdr *sec;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	if (WARN_ON(!obj->relocs))
-		return -EINVAL;
-
 	module_disable_ro(pmod);
+	/* For each klp relocation section */
+	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
+		sec = pmod->klp_info->sechdrs + i;
+		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
+			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);
+		/* Check if the klp section is formatted correctly */
+		ret = klp_check_relasec_format(pmod, sec);
 		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);
+		/* Check if the klp section belongs to obj */
+		if (!klp_relasec_matches_object(pmod, sec, obj))
+			continue;
+
+		/* Resolve all livepatch syms referenced in this rela section */
+		ret = klp_resolve_symbols(sec, pmod);
+		if (ret)
 			goto out;
-		}
-	}
 
+		ret = apply_relocate_add(pmod->klp_info->sechdrs, pmod->strtab,
+					 pmod->klp_info->symndx, i, pmod);
+		if (ret)
+			goto out;
+	}
 out:
 	module_enable_ro(pmod);
 	return ret;
@@ -703,11 +875,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,
@@ -842,6 +1012,9 @@ int klp_register_patch(struct klp_patch *patch)
 {
 	int ret;
 
+	if (!is_livepatch_module(patch->mod))
+		return -EINVAL;
+
 	if (!klp_initialized())
 		return -ENODEV;
 
-- 
2.4.3

[RFC PATCH v4 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 v4 6/6] Documentation: livepatch: outline Elf format and requirements for patch modules

[Jessica Yu]

Document livepatch module requirements and the special Elf constants patch
modules use.

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

diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
new file mode 100644
index 0000000..736b620
--- /dev/null
+++ b/Documentation/livepatch/module-elf-format.txt
@@ -0,0 +1,311 @@
+===========================
+Livepatch module Elf format
+===========================
+
+This document outlines the Elf format requirements that livepatch modules must follow.
+
+-----------------
+Table of Contents
+-----------------
+0. Background and motivation
+1. Livepatch modinfo field
+2. Livepatch relocation sections
+   2.1 What are livepatch relocation sections?
+   2.2 Livepatch relocation section format
+       2.2.1 Required flags
+       2.2.2 Required name format
+       2.2.3 Example livepatch relocation section names
+       2.2.4 Example `readelf --sections` output
+       2.2.5 Example `readelf --relocs` output
+3. Livepatch symbols
+   3.1 What are livepatch symbols?
+   3.2 A livepatch module's symbol table
+   3.3 Livepatch symbol format
+       3.3.1 Required flags
+       3.3.2 Required name format
+       3.3.3 Example livepatch symbol names
+       3.3.4 Example `readelf --symbols` output
+4. Symbol table and Elf section access
+
+----------------------------
+0. Background and motivation
+----------------------------
+
+Formerly, livepatch required separate architecture-specific code to write
+relocations. However, arch-specific code to write relocations already
+exists in the module loader, so this former approach produced redundant
+code. So, instead of duplicating code and re-implementing what the module
+loader can already do, livepatch leverages existing code in the module
+loader to perform the all the arch-specific relocation work. Specifically,
+livepatch reuses the apply_relocate_add() function in the module loader to
+write relocations. The patch module Elf format described in this document
+enables livepatch to be able to do this. The hope is that this will make
+livepatch more easily portable to other architectures and reduce the amount
+of arch-specific code required to port livepatch to a particular
+architecture.
+
+Since apply_relocate_add() requires access to a module's section header
+table, symbol table, and relocation section indices, Elf information is
+preserved for livepatch modules (see section 4). Livepatch manages its own
+relocation sections and symbols, which are described in this document. The
+Elf constants used to mark livepatch symbols and relocation sections were
+selected from OS-specific ranges according to the definitions from glibc.
+
+0.1 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 yet loaded at patch module load time (e.g. a patch to a
+driver that is not loaded). Formerly, livepatch solved this problem by
+embedding special "dynrela" (dynamic rela) sections in the resulting patch
+module Elf output. Using these dynrela sections, livepatch could resolve
+symbols while taking into account its scope and what module the symbol
+belongs to, and then manually apply the dynamic relocations. However this
+approach required livepatch to supply arch-specific code in order to write
+these relocations. In the new format, livepatch manages its own SHT_RELA
+relocation sections in place of dynrela sections, and the symbols that the
+relas reference are special livepatch symbols (see section 2 and 3). The
+arch-specific livepatch relocation code is replaced by a call to
+apply_relocate_add().
+
+================================
+PATCH MODULE FORMAT REQUIREMENTS
+================================
+
+--------------------------
+1. Livepatch modinfo field
+--------------------------
+
+Livepatch modules are required to have the "livepatch" modinfo attribute.
+See the sample livepatch module in samples/livepatch/ for how this is done.
+
+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 livepatch-meminfo.ko
+filename:		livepatch-meminfo.ko
+livepatch:		Y
+license:		GPL
+depends:
+vermagic:		4.3.0+ SMP mod_unload
+
+--------------------------------
+2. Livepatch relocation sections
+--------------------------------
+
+-------------------------------------------
+2.1 What are livepatch relocation sections?
+-------------------------------------------
+A livepatch module manages its own Elf relocation sections to apply
+relocations to modules as well as to the kernel (vmlinux) at the
+appropriate time. For example, if a patch module patches a driver that is
+not currently loaded, livepatch will apply the corresponding livepatch
+relocation section(s) to the driver once it loads.
+
+Each "object" (e.g. vmlinux, or a module) within a patch module may have
+multiple livepatch relocation sections associated with it (e.g. patches to
+multiple functions within the same object). There is a 1-1 correspondence
+between a livepatch relocation section and the target section (usually the
+text section of a function) to which the relocation(s) apply. It is
+also possible for a livepatch module to have no livepatch relocation
+sections, as in the case of the sample livepatch module (see
+samples/livepatch).
+
+Since Elf information is preserved for livepatch modules (see Section 4), a
+livepatch relocation section can be applied simply by passing in the
+appropriate section index to apply_relocate_add(), which then uses it to
+access the relocation section and apply the relocations.
+
+Every symbol referenced by a rela in a livepatch relocation section is a
+livepatch symbol. These must be resolved before livepatch can call
+apply_relocate_add(). See Section 3 for more information.
+
+---------------------------------------
+2.2 Livepatch relocation section format
+---------------------------------------
+
+2.2.1 Required flags
+--------------------
+Livepatch relocation sections must be marked with the SHF_RELA_LIVEPATCH
+section flag. See include/uapi/linux/elf.h for the definition. The module
+loader recognizes this flag and will avoid applying those relocation sections
+at patch module load time. These sections must also be marked with SHF_ALLOC,
+so that the module loader doesn't discard them on module load (i.e. they will
+be copied into memory along with the other SHF_ALLOC sections).
+
+2.2.2 Required name format
+--------------------------
+The name of a livepatch relocation section must conform to the following format:
+
+.klp.rela.objname.section_name
+^        ^^     ^ ^          ^
+|________||_____| |__________|
+   [A]      [B]        [C]
+
+[A] The relocation section name is prefixed with the string ".klp.rela."
+[B] The name of the object (i.e. "vmlinux" or name of module) to
+    which the relocation section belongs follows immediately after the prefix.
+[C] The actual name of the section to which this relocation section applies.
+
+2.2.3 Example livepatch relocation section names:
+-------------------------------------------------
+.klp.rela.ext4.text.ext4_attr_store
+.klp.rela.vmlinux.text.cmdline_proc_show
+
+2.2.4 Example `readelf --sections` output for a patch
+module that patches vmlinux and modules 9p, btrfs, ext4:
+--------------------------------------------------------
+  Section Headers:
+  [Nr] Name                          Type                    Address          Off    Size   ES Flg Lk Inf Al
+  [ snip ]
+  [29] .klp.rela.9p.text.caches.show RELA                    0000000000000000 002d58 0000c0 18 AIo 64   9  8
+  [30] .klp.rela.btrfs.text.btrfs.feature.attr.show RELA     0000000000000000 002e18 000060 18 AIo 64  11  8
+  [ snip ]
+  [34] .klp.rela.ext4.text.ext4.attr.store RELA              0000000000000000 002fd8 0000d8 18 AIo 64  13  8
+  [35] .klp.rela.ext4.text.ext4.attr.show RELA               0000000000000000 0030b0 000150 18 AIo 64  15  8
+  [36] .klp.rela.vmlinux.text.cmdline.proc.show RELA         0000000000000000 003200 000018 18 AIo 64  17  8
+  [37] .klp.rela.vmlinux.text.meminfo.proc.show RELA         0000000000000000 003218 0000f0 18 AIo 64  19  8
+  [ snip ]                                       ^                                             ^
+                                                 |                                             |
+                                                [*]                                           [*]
+[*] Livepatch 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).
+
+2.2.5 Example `readelf --relocs` output for a patch module:
+-----------------------------------------------------------
+Relocation section '.klp.rela.btrfs.text.btrfs_feature_attr_show' at offset 0x2ba0 contains 4 entries:
+    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+000000000000001f  0000005e00000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.printk,0 - 4
+0000000000000028  0000003d0000000b R_X86_64_32S           0000000000000000 .klp.sym.btrfs.btrfs_ktype,0 + 0
+0000000000000036  0000003b00000002 R_X86_64_PC32          0000000000000000 .klp.sym.btrfs.can_modify_feature.isra.3,0 - 4
+000000000000004c  0000004900000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.snprintf,0 - 4
+[ snip ]                                                                   ^
+                                                                           |
+                                                                          [*]
+[*] Every symbol referenced by a relocation is a livepatch symbol.
+
+--------------------
+3. Livepatch symbols
+--------------------
+
+-------------------------------
+3.1 What are livepatch symbols?
+-------------------------------
+Livepatch symbols are symbols referred to by livepatch relocation sections.
+These are symbols accessed from new versions of functions for patched
+objects, whose addresses cannot be resolved by the module loader (because
+they are local or unexported global syms). Since the module loader only
+resolves exported syms, and not every symbol referenced by the new patched
+functions is exported, livepatch symbols were introduced. They are used
+also in cases where we cannot immediately know the address of a symbol when
+a patch module loads. For example, this is the case when livepatch patches
+a module that is not loaded yet. In this case, the relevant livepatch
+symbols are resolved simply when the target module loads. In any case, for
+any livepatch relocation section, all livepatch symbols referenced by that
+section must be resolved before livepatch can call apply_relocate_add() for
+that reloc section.
+
+Livepatch symbols must be marked with SHN_LIVEPATCH so that the module
+loader can identify and ignore them. Livepatch modules keep these symbols
+in their symbol tables, and the symbol table is made accessible through
+module->symtab.
+
+-------------------------------------
+3.2 A livepatch module's symbol table
+-------------------------------------
+Normally, a stripped down copy of a module's symbol table (containing only
+"core" symbols) is made available through module->symtab (See layout_symtab()
+in kernel/module.c). For livepatch modules, the symbol table copied into memory
+on module load must be exactly the same as the symbol table produced when the
+patch module was compiled. This is because the relocations in each livepatch
+relocation section refer to their respective symbols with their symbol indices,
+and the original symbol indices (and thus the symtab ordering) must be
+preserved in order for apply_relocate_add() to find the right symbol.
+
+For example, take this particular rela from a livepatch module:
+Relocation section '.klp.rela.btrfs.text.btrfs_feature_attr_show' at offset 0x2ba0 contains 4 entries:
+    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+000000000000001f  0000005e00000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.printk,0 - 4
+
+This rela refers to the symbol '.klp.sym.vmlinux.printk,0', and the symbol index is encoded
+in 'Info'. Here its symbol index is 0x5e, which is 94 in decimal, which refers to the
+symbol index 94.
+And in this patch module's corresponding symbol table, symbol index 94 refers to that very symbol:
+[ snip ]
+94: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.printk,0
+[ snip ]
+
+---------------------------
+3.3 Livepatch symbol format
+---------------------------
+
+3.3.1 Required flags
+--------------------
+Livepatch symbols must have their section index marked as SHN_LIVEPATCH, so
+that the module loader can identify them and not attempt to resolve them.
+See include/uapi/linux/elf.h for the actual definitions.
+
+3.3.2 Required name format
+--------------------------
+Livepatch symbol names must conform to the following format:
+
+.klp.sym.objname.symbol_name,sympos
+^       ^^     ^ ^         ^ ^
+|_______||_____| |_________| |
+   [A]     [B]       [C]    [D]
+
+[A] The symbol name is prefixed with the string ".klp.sym."
+[B] The name of the object (i.e. "vmlinux" or name of module) to
+    which the symbol belongs follows immediately after the prefix.
+[C] The actual name of the symbol.
+[D] The position of the symbol in the object (as according to kallsyms)
+    This is used to differentiate duplicate symbols within the same
+    object. The symbol position is expressed numerically (0, 1, 2...).
+    The symbol position of a unique symbol is 0.
+
+3.3.3 Example livepatch symbol names:
+-------------------------------------
+.klp.sym.vmlinux.snprintf,0
+.klp.sym.vmlinux.printk,0
+.klp.sym.btrfs.btrfs_ktype,0
+
+3.3.4 Example `readelf --symbols` output for a patch module:
+------------------------------------------------------------
+Symbol table '.symtab' contains 127 entries:
+   Num:    Value          Size Type    Bind   Vis     Ndx         Name
+   [ snip ]
+    73: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.snprintf,0
+    74: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.capable,0
+    75: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.find_next_bit,0
+    76: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT OS [0xff20] .klp.sym.vmlinux.si_swapinfo,0
+  [ snip ]                                               ^
+                                                         |
+                                                        [*]
+[*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
+    "OS" means OS-specific.
+
+--------------------------------------
+4. Symbol table and Elf section access
+--------------------------------------
+A livepatch module's symbol table is accessible through module->symtab.
+
+Since apply_relocate_add() requires access to a module's section headers,
+symbol table, and relocation section indices, Elf information is preserved for
+livepatch modules and is made accessible by the module loader through
+module->klp_info, which is a klp_modinfo struct. When a livepatch module loads,
+this struct is filled in by the module loader. Its fields are documented below:
+
+struct klp_modinfo {
+	Elf_Ehdr hdr; /* Elf header */
+	Elf_Shdr *sechdrs; /* Section header table */
+	char *secstrings; /* String table for the section headers */
+	unsigned int symndx; /* The symbol table section index */
+};
-- 
2.4.3

Re: module: s390: keep mod_arch_specific for livepatch modules

[Jessica Yu]

+++ Jessica Yu [03/02/16 20:11 -0500]:
>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 | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)

I must note that I have verified that the patchset boots on s390 and
that the sample livepatch module still works ...so that's good, but
not saying much since what we really want is to test the relocation
code. The kpatch build scripts however currently only support x86, so
the next step is for me to port the kpatch scripts to s390 before I
can really test this patchset. This in itself might take a while, so
in the meantime I'd like to just collect another round of comments and
feedback for v4.

Jessica

Re: module: s390: keep mod_arch_specific for livepatch modules

[Josh Poimboeuf]

On Wed, Feb 03, 2016 at 08:37:52PM -0500, Jessica Yu wrote:
> +++ Jessica Yu [03/02/16 20:11 -0500]:
> >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 | 7 +++++--
> >1 file changed, 5 insertions(+), 2 deletions(-)
> 
> I must note that I have verified that the patchset boots on s390 and
> that the sample livepatch module still works ...so that's good, but
> not saying much since what we really want is to test the relocation
> code. The kpatch build scripts however currently only support x86, so
> the next step is for me to port the kpatch scripts to s390 before I
> can really test this patchset. This in itself might take a while, so
> in the meantime I'd like to just collect another round of comments and
> feedback for v4.

I haven't reviewed the code yet, but otherwise I'm thinking it would
actually be fine to merge this patch set before testing with s390
relocations.  They aren't implemented on s390 today anyway, so there
can't be a regression if nobody is using it.

-- 
Josh

Re: module: s390: keep mod_arch_specific for livepatch modules

[Miroslav Benes]

On Thu, 4 Feb 2016, Josh Poimboeuf wrote:

> On Wed, Feb 03, 2016 at 08:37:52PM -0500, Jessica Yu wrote:
> > +++ Jessica Yu [03/02/16 20:11 -0500]:
> > >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 | 7 +++++--
> > >1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > I must note that I have verified that the patchset boots on s390 and
> > that the sample livepatch module still works ...so that's good, but
> > not saying much since what we really want is to test the relocation
> > code. The kpatch build scripts however currently only support x86, so
> > the next step is for me to port the kpatch scripts to s390 before I
> > can really test this patchset. This in itself might take a while, so
> > in the meantime I'd like to just collect another round of comments and
> > feedback for v4.
> 
> I haven't reviewed the code yet, but otherwise I'm thinking it would
> actually be fine to merge this patch set before testing with s390
> relocations.  They aren't implemented on s390 today anyway, so there
> can't be a regression if nobody is using it.

I asked Jessica to test it with s390 relocations. It would be great to 
know it works on an architecture which originally inspired this patch set. 
However it should not be a blocker. There are advantages apart from this 
and we can happily merge it before testing.

I am going to review the patch set early next week...

Miroslav

Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch

[Miroslav Benes]

On Wed, 3 Feb 2016, Jessica Yu wrote:

> 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 Elf format and requirements for
>     patch modules

Hi,

I walked through the code and it looks good except for several minor 
things in the fourth patch (livepatch: reuse module loader code to write 
relocations). I'd propose to send the next version as a regular PATCH set 
and not RFC. We can start collecting Reviews and Acks. Hopefully it won't 
take more than one or two iterations. Would that be ok with everyone?

Thanks,
Miroslav

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

[Miroslav Benes]

Hi,

several minor things and nits below. Otherwise it is ok.

On Wed, 3 Feb 2016, Jessica Yu wrote:

> + * Check if a livepatch symbol is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> +{
> +	size_t len;
> +	char *s, *objname, *symname;
> +
> +	if (sym->st_shndx != SHN_LIVEPATCH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Livepatch symbol names must follow this format:
> +	 * .klp.sym.objname.symbol_name,sympos
> +	 */
> +	s = pmod->strtab + sym->st_name;
> +	/* [.klp.sym.]objname.symbol_name,sympos */
> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +	if (!(len > 0))
> +		return -EINVAL;

strcspn() returns size_t, so we can check only for 0

if (!len)
	return -EINVAL

> +
> +	/* .klp.sym.objname.symbol_name,[sympos] */
> +	if (!strchr(s, ','))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +	if (!(len > 0))
> +		return -EINVAL;

Same here.

> +
> +	return 0;
> +}
> +
> +/*
> + * Check if a livepatch relocation section is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> +{
> +	char *secname;
> +	size_t len;
> +

This is really a nitpick, but you have a comment about mandatory format of 
the name here in klp_check_symbol_format().

> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* [.klp.rela.]objname.section_name */
> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> +				KLP_RELASEC_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.rela.[objname].section_name */
> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> +	if (!(len > 0))
> +		return -EINVAL;

You don't check if section_name part is non-empty.

[...]

> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{

Do we need result to be a double-pointer? If I am not mistaken just 'char 
*result' could be sufficient. You check the return value, so result could 
be NULL or objname as found. No?

> +	size_t len;
> +	char *objname, *objname_start;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname_start = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname_start, ".");
> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> +	if (objname == NULL)
> +		return -ENOMEM;
> +
> +	/* klp_find_object_symbol() treats NULL as vmlinux */
> +	if (!strcmp(objname, "vmlinux")) {
> +		*result = NULL;
> +		kfree(objname);
> +	} else
> +		*result = objname;

According to CodingStyle there should be curly braces even for else 
branch.

> +	return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)

Same here.

> +{
> +	size_t len;
> +	char *objname, *symname;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +
> +	*result = kstrndup(symname, len, GFP_KERNEL);
> +	if (*result == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

[...]
> +static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> -
> -	/* 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;
> +	int i, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *s, *symbol_name, *sym_objname;
> +	unsigned long sympos;
> +
> +	relas = (Elf_Rela *) relasec->sh_addr;
> +	/* For each rela in this .klp.rela. section */
> +	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->symtab + ELF_R_SYM(relas[i].r_info);
> +
> +		/* Check if the symbol is formatted correctly */
> +		ret = klp_check_symbol_format(pmod, sym);
> +		if (ret)
> +			goto out;
> +		/* Format: .klp.sym.objname.symbol_name,sympos */
> +		s = pmod->strtab + sym->st_name;
> +		ret = klp_get_symbol_name(s, &symbol_name);
> +		if (ret)
> +			goto out;
> +		ret = klp_get_sym_objname(s, &sym_objname);
> +		if (ret)
> +			goto free_symbol_name;
> +		ret = klp_get_sympos(s, &sympos);
> +		if (ret)
> +			goto free_objname;
> +
> +		ret = klp_find_object_symbol(sym_objname, symbol_name, sympos,
> +					     (unsigned long *) &sym->st_value);
> +free_objname:
> +		kfree(sym_objname);
> +free_symbol_name:
> +		kfree(symbol_name);
> +		if (ret)
> +			goto out;
>  	}
> -	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);
> +out:
> +	return ret;
>  }

I wonder if 'break;' instead of 'goto out;' would generate 
different/better/more readable code. Anyway out label is not necessary and 
we can achieve the same with break. It is up to you though.

>  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, ret = 0;
> +	Elf_Shdr *sec;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> -
>  	module_disable_ro(pmod);
> +	/* For each klp relocation section */
> +	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> +		sec = pmod->klp_info->sechdrs + i;
> +		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> +			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);
> +		/* Check if the klp section is formatted correctly */
> +		ret = klp_check_relasec_format(pmod, sec);
>  		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);
> +		/* Check if the klp section belongs to obj */
> +		if (!klp_relasec_matches_object(pmod, sec, obj))
> +			continue;
> +
> +		/* Resolve all livepatch syms referenced in this rela section */
> +		ret = klp_resolve_symbols(sec, pmod);
> +		if (ret)
>  			goto out;
> -		}
> -	}
>  
> +		ret = apply_relocate_add(pmod->klp_info->sechdrs, pmod->strtab,
> +					 pmod->klp_info->symndx, i, pmod);
> +		if (ret)
> +			goto out;
> +	}
>  out:
>  	module_enable_ro(pmod);
>  	return ret;

Same thing with break instead of all gotos.

Thanks,
Miroslav

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

[Josh Poimboeuf]

On Wed, Feb 03, 2016 at 08:11:07PM -0500, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader. Persist copies of the
> original symbol table and string table.
> 
> Livepatch manages its own relocation sections in order to reuse module
> loader code to write relocations. Livepatch modules must preserve Elf
> information such as section indices in order to apply livepatch relocation
> sections using the module loader's apply_relocate_add() function.
> 
> In order to apply livepatch relocation sections, livepatch modules must
> keep a complete copy of their original symbol table in memory. Normally, a
> stripped down copy of a module's symbol table (containing only "core"
> symbols) is made available through module->core_symtab. But for livepatch
> modules, the symbol table copied into memory on module load must be exactly
> the same as the symbol table produced when the patch module was compiled.
> This is because the relocations in each livepatch relocation section refer
> to their respective symbols with their symbol indices, and the original
> symbol indices (and thus the symtab ordering) must be preserved in order
> for apply_relocate_add() to find the right symbol.

This patch didn't apply clean to linux-next/master.  I didn't
investigate why, but maybe it depends on the other patch set which
removes the notifiers?  (If so, that should be mentioned in the cover
letter.)

A couple of minor comments below...

> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/linux/module.h |  25 ++++++++++
>  kernel/module.c        | 133 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 151 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 4560d8f..58e6200 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -324,6 +324,15 @@ struct module_layout {
>  #define __module_layout_align
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +struct klp_modinfo {
> +	Elf_Ehdr hdr;
> +	Elf_Shdr *sechdrs;
> +	char *secstrings;
> +	unsigned int symndx;
> +};
> +#endif
> +
>  struct module {
>  	enum module_state state;
>  
> @@ -455,7 +464,11 @@ struct module {
>  #endif
>  
>  #ifdef CONFIG_LIVEPATCH
> +	bool klp; /* Is this a livepatch module? */
>  	bool klp_alive;
> +
> +	/* Elf information */
> +	struct klp_modinfo *klp_info;
>  #endif
>  
>  #ifdef CONFIG_MODULE_UNLOAD
> @@ -629,6 +642,18 @@ static inline bool module_requested_async_probing(struct module *module)
>  	return module && module->async_probe_requested;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static inline bool is_livepatch_module(struct module *mod)
> +{
> +	return mod->klp;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static inline bool is_livepatch_module(struct module *mod)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  #else /* !CONFIG_MODULES... */
>  
>  /* Given an address, look for it in the exception tables. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1970,6 +1970,82 @@ static void module_enable_nx(const struct module *mod) { }
>  static void module_disable_nx(const struct module *mod) { }
>  #endif
>  
> +#ifdef CONFIG_LIVEPATCH
> +/*
> + * Persist Elf information about a module. Copy the Elf header,
> + * section header table, section string table, and symtab section
> + * index from info to mod->klp_info.
> + */
> +static int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	unsigned int size, symndx;
> +	int ret = 0;
> +
> +	size = sizeof(*mod->klp_info);
> +	mod->klp_info = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info == NULL)
> +		return -ENOMEM;
> +
> +	/* Elf header */
> +	size = sizeof(Elf_Ehdr);
> +	memcpy(&mod->klp_info->hdr, info->hdr, size);
> +
> +	/* Elf section header table */
> +	size = sizeof(Elf_Shdr) * info->hdr->e_shnum;
> +	mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info->sechdrs == NULL) {
> +		ret = -ENOMEM;
> +		goto free_info;
> +	}
> +	memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
> +
> +	/* Elf section name string table */
> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> +	mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info->secstrings == NULL) {
> +		ret = -ENOMEM;
> +		goto free_sechdrs;
> +	}
> +	memcpy(mod->klp_info->secstrings, info->secstrings, size);
> +
> +	/* Elf symbol section index */
> +	symndx = info->index.sym;
> +	mod->klp_info->symndx = symndx;
> +
> +	/*
> +	 * For livepatch modules, core_symtab is a complete copy
> +	 * of the original symbol table. Adjust sh_addr to point
> +	 * to core_symtab since the copy of the symtab in module
> +	 * init memory is freed at the end of do_init_module().
> +	 */
> +	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_symtab;
> +
> +	return ret;
> +
> +free_sechdrs:
> +	kfree(mod->klp_info->sechdrs);
> +free_info:
> +	kfree(mod->klp_info);
> +	return ret;
> +}
> +
> +static void free_module_elf(struct module *mod)
> +{
> +	kfree(mod->klp_info->sechdrs);
> +	kfree(mod->klp_info->secstrings);
> +	kfree(mod->klp_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);
> @@ -2008,6 +2084,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);
> +

I think this code is self-evident, so the comment isn't necessary.


>  	/* Now we can delete it from the lists */
>  	mutex_lock(&module_mutex);
>  	/* Unlink carefully: kallsyms could be walking list. */
> @@ -2123,6 +2203,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			       (long)sym[i].st_value);
>  			break;
>  
> +		case SHN_LIVEPATCH:
> +			/* Livepatch symbols are resolved by livepatch */
> +			break;
> +
>  		case SHN_UNDEF:
>  			ksym = resolve_symbol_wait(mod, info, name);
>  			/* Ok if resolved.  */
> @@ -2171,6 +2255,10 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>  			continue;
>  
> +		/* Livepatch 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);
> @@ -2466,7 +2554,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;
> @@ -2509,7 +2597,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];
> @@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))
> +		return -ENOEXEC;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  /* Sets info->hdr and info->len. */
>  static int copy_module_from_fd(int fd, struct load_info *info)
>  {
> @@ -2859,6 +2964,10 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>  			"is unknown, you have been warned.\n", mod->name);
>  	}
>  
> +	err = find_livepatch_modinfo(mod, info);
> +	if (err)
> +		return err;
> +
>  	/* Set up license info based on the info section */
>  	set_license(mod, get_modinfo(info, "license"));
>  
> @@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
>  	 */
>  	current->flags &= ~PF_USED_ASYNC;
>  
> +#ifdef CONFIG_KALLSYMS
> +	/* Make symtab and strtab available prior to module init call */
> +	mod->num_symtab = mod->core_num_syms;
> +	mod->symtab = mod->core_symtab;
> +	mod->strtab = mod->core_strtab;
> +#endif
>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> @@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
>  	/* Drop initial reference. */
>  	module_put(mod);
>  	trim_init_extable(mod);
> -#ifdef CONFIG_KALLSYMS
> -	mod->num_symtab = mod->core_num_syms;
> -	mod->symtab = mod->core_symtab;
> -	mod->strtab = mod->core_strtab;
> -#endif
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);
> @@ -3522,6 +3632,13 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto bug_cleanup;
>  
> +	/* For livepatch modules, save Elf info from load_info struct */
> +	if (is_livepatch_module(mod)) {
> +		err = copy_module_elf(mod, info);
> +		if (err < 0)
> +			goto sysfs_cleanup;
> +	}
> +

Same here, unecessary comment IMO.

>  	/* Get rid of temporary copy. */
>  	free_copy(info);
>  
> @@ -3530,6 +3647,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
> 

-- 
Josh

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

[Josh Poimboeuf]

On Wed, Feb 03, 2016 at 08:11:09PM -0500, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the need
> for architecture specific relocation code in livepatch. Specifically, reuse
> the apply_relocate_add() function in the module loader to write relocations
> instead of duplicating functionality in livepatch's arch-dependent
> klp_write_module_reloc() function.
> 
> In order to accomplish this, livepatch modules manage their own relocation
> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> index). To apply livepatch relocation sections, 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 and the s390
> klp_write_module_reloc() function stub. They are no longer needed since
> relocation work has been offloaded to module loader.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  arch/s390/include/asm/livepatch.h |   7 -
>  arch/x86/include/asm/livepatch.h  |   2 -
>  arch/x86/kernel/Makefile          |   1 -
>  arch/x86/kernel/livepatch.c       |  70 ----------
>  include/linux/livepatch.h         |  30 ++--
>  kernel/livepatch/core.c           | 283 ++++++++++++++++++++++++++++++--------
>  6 files changed, 238 insertions(+), 155 deletions(-)
>  delete mode 100644 arch/x86/kernel/livepatch.c
> 
> diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
> index a52b6cc..350a751 100644
> --- a/arch/s390/include/asm/livepatch.h
> +++ b/arch/s390/include/asm/livepatch.h
> @@ -25,13 +25,6 @@ static inline int klp_check_compiler_support(void)
>  	return 0;
>  }
>  
> -static inline int klp_write_module_reloc(struct module *mod, unsigned long
> -		type, unsigned long loc, unsigned long value)
> -{
> -	/* not supported yet */
> -	return -ENOSYS;
> -}
> -
>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>  {
>  	regs->psw.addr = ip;
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index e795f52..d7c2b57 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 fdd5f1c..1a40a72 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,16 @@ struct klp_patch {
>  	enum klp_state state;
>  };
>  
> +/*
> + * Livepatch symbol and relocation section prefixes:
> + * ".klp.rela." for relocation sections
> + * ".klp.sym." for livepatch symbols
> + */
> +#define KLP_SYM_PREFIX ".klp.sym."
> +#define KLP_SYM_PREFIX_LEN 9
> +#define KLP_RELASEC_PREFIX ".klp.rela."
> +#define KLP_RELASEC_PREFIX_LEN 10
> +
>  #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 7aa975d..c1fe57c 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>
>  
>  /**
> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  	return !obj->name || obj->mod;
>  }
>  
> +/*
> + * Check if a livepatch symbol is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> +{
> +	size_t len;
> +	char *s, *objname, *symname;
> +
> +	if (sym->st_shndx != SHN_LIVEPATCH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Livepatch symbol names must follow this format:
> +	 * .klp.sym.objname.symbol_name,sympos
> +	 */
> +	s = pmod->strtab + sym->st_name;
> +	/* [.klp.sym.]objname.symbol_name,sympos */
> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.symbol_name,[sympos] */
> +	if (!strchr(s, ','))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if a livepatch relocation section is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> +{
> +	char *secname;
> +	size_t len;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* [.klp.rela.]objname.section_name */
> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> +				KLP_RELASEC_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.rela.[objname].section_name */
> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if obj->name matches the objname encoded in the rela
> + * section name (.klp.rela.[objname].section_name)
> + *
> + * Must pass klp_check_relasec_format() before calling this.
> + */
> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> +				       struct klp_object *obj)
> +{
> +	size_t len;
> +	const char *obj_objname, *sec_objname, *secname;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* .klp.rela.[objname].section_name */
> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> +
> +	/* Get length of the objname encoded in the section name */
> +	len = strcspn(sec_objname, ".");
> +
> +	if (strlen(obj_objname) != len)
> +		return false;
> +
> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> +}
> +
> +/*
> + * klp_get_* helper functions
> + *
> + * klp_get_* functions extract different components of the name
> + * of a livepatch symbol. The full symbol name from the strtab
> + * is passed in as parameter @s, and @result is filled in with
> + * the extracted component.
> + *
> + * These functions assume a correctly formatted symbol and the
> + * klp_check_symbol_format() test *must* pass before calling any
> + * of these functions.
> + */
> +
> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *objname_start;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname_start = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname_start, ".");
> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> +	if (objname == NULL)
> +		return -ENOMEM;
> +
> +	/* klp_find_object_symbol() treats NULL as vmlinux */
> +	if (!strcmp(objname, "vmlinux")) {
> +		*result = NULL;
> +		kfree(objname);
> +	} else
> +		*result = objname;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *symname;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +
> +	*result = kstrndup(symname, len, GFP_KERNEL);
> +	if (*result == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.symbol_name,[sympos] */
> +static int klp_get_sympos(char *s, unsigned long *result)
> +{
> +	char *sympos;
> +
> +	/* .klp.sym.symbol_name,[sympos] */
> +	sympos = strchr(s, ',') + 1;
> +	return kstrtol(sympos, 10, result);
> +}
> +
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {

I think all the above string parsing code could be replaced with a
couple of sscanf() calls.

For example:

	char objname[64], symname[256];

	ret = sscanf(s, ".klp.sym.%63[^.].%255[^,],%u", objname, symname, &sympos);
	if (ret < 3)
		// string doesn't match expected format

That would be much simpler.

Only problem is, the kernel version of sscanf() doesn't seem to support
the '[' conversion specifier.  At least not yet ;-)  Adding support for
that would be a win-win: less code overall, and the addition of a useful
scanf feature which could be used by other code.

> @@ -204,74 +367,83 @@ 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 *relasec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> -
> -	/* 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;
> +	int i, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *s, *symbol_name, *sym_objname;
> +	unsigned long sympos;
> +
> +	relas = (Elf_Rela *) relasec->sh_addr;
> +	/* For each rela in this .klp.rela. section */
> +	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->symtab + ELF_R_SYM(relas[i].r_info);
> +
> +		/* Check if the symbol is formatted correctly */
> +		ret = klp_check_symbol_format(pmod, sym);
> +		if (ret)
> +			goto out;
> +		/* Format: .klp.sym.objname.symbol_name,sympos */
> +		s = pmod->strtab + sym->st_name;
> +		ret = klp_get_symbol_name(s, &symbol_name);
> +		if (ret)
> +			goto out;
> +		ret = klp_get_sym_objname(s, &sym_objname);
> +		if (ret)
> +			goto free_symbol_name;
> +		ret = klp_get_sympos(s, &sympos);
> +		if (ret)
> +			goto free_objname;

IMO whitespace after all the gotos would help with readability.

Also, since the "out" label is just a return, the "goto out"'s can all
just be replaced with returns.

> +
> +		ret = klp_find_object_symbol(sym_objname, symbol_name, sympos,
> +					     (unsigned long *) &sym->st_value);
> +free_objname:
> +		kfree(sym_objname);
> +free_symbol_name:
> +		kfree(symbol_name);
> +		if (ret)
> +			goto out;
>  	}
> -	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);
> +out:
> +	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, ret = 0;
> +	Elf_Shdr *sec;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> -
>  	module_disable_ro(pmod);
> +	/* For each klp relocation section */
> +	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> +		sec = pmod->klp_info->sechdrs + i;
> +		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> +			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);
> +		/* Check if the klp section is formatted correctly */
> +		ret = klp_check_relasec_format(pmod, sec);

I think this code is self-evident and the comment isn't needed.

>  		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);
> +		/* Check if the klp section belongs to obj */
> +		if (!klp_relasec_matches_object(pmod, sec, obj))
> +			continue;
> +
> +		/* Resolve all livepatch syms referenced in this rela section */
> +		ret = klp_resolve_symbols(sec, pmod);
> +		if (ret)
>  			goto out;
> -		}
> -	}
>  
> +		ret = apply_relocate_add(pmod->klp_info->sechdrs, pmod->strtab,
> +					 pmod->klp_info->symndx, i, pmod);
> +		if (ret)
> +			goto out;
> +	}
>  out:
>  	module_enable_ro(pmod);
>  	return ret;
> @@ -703,11 +875,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,
> @@ -842,6 +1012,9 @@ int klp_register_patch(struct klp_patch *patch)
>  {
>  	int ret;
>  
> +	if (!is_livepatch_module(patch->mod))
> +		return -EINVAL;
> +
>  	if (!klp_initialized())
>  		return -ENODEV;
>  
> -- 
> 2.4.3
> 

-- 
Josh

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

[Josh Poimboeuf]

On Wed, Feb 03, 2016 at 08:11:09PM -0500, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the need
> for architecture specific relocation code in livepatch. Specifically, reuse
> the apply_relocate_add() function in the module loader to write relocations
> instead of duplicating functionality in livepatch's arch-dependent
> klp_write_module_reloc() function.
> 
> In order to accomplish this, livepatch modules manage their own relocation
> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> index). To apply livepatch relocation sections, 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 and the s390
> klp_write_module_reloc() function stub. They are no longer needed since
> relocation work has been offloaded to module loader.
> 
> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/s390/include/asm/livepatch.h |   7 -
>  arch/x86/include/asm/livepatch.h  |   2 -
>  arch/x86/kernel/Makefile          |   1 -
>  arch/x86/kernel/livepatch.c       |  70 ----------
>  include/linux/livepatch.h         |  30 ++--
>  kernel/livepatch/core.c           | 283 ++++++++++++++++++++++++++++++--------
>  6 files changed, 238 insertions(+), 155 deletions(-)
>  delete mode 100644 arch/x86/kernel/livepatch.c
> 
> diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
> index a52b6cc..350a751 100644
> --- a/arch/s390/include/asm/livepatch.h
> +++ b/arch/s390/include/asm/livepatch.h
> @@ -25,13 +25,6 @@ static inline int klp_check_compiler_support(void)
>  	return 0;
>  }
>  
> -static inline int klp_write_module_reloc(struct module *mod, unsigned long
> -		type, unsigned long loc, unsigned long value)
> -{
> -	/* not supported yet */
> -	return -ENOSYS;
> -}
> -
>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>  {
>  	regs->psw.addr = ip;
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index e795f52..d7c2b57 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 fdd5f1c..1a40a72 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,16 @@ struct klp_patch {
>  	enum klp_state state;
>  };
>  
> +/*
> + * Livepatch symbol and relocation section prefixes:
> + * ".klp.rela." for relocation sections
> + * ".klp.sym." for livepatch symbols
> + */
> +#define KLP_SYM_PREFIX ".klp.sym."
> +#define KLP_SYM_PREFIX_LEN 9
> +#define KLP_RELASEC_PREFIX ".klp.rela."
> +#define KLP_RELASEC_PREFIX_LEN 10
> +
>  #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 7aa975d..c1fe57c 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>
>  
>  /**
> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  	return !obj->name || obj->mod;
>  }
>  
> +/*
> + * Check if a livepatch symbol is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> +{
> +	size_t len;
> +	char *s, *objname, *symname;
> +
> +	if (sym->st_shndx != SHN_LIVEPATCH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Livepatch symbol names must follow this format:
> +	 * .klp.sym.objname.symbol_name,sympos
> +	 */
> +	s = pmod->strtab + sym->st_name;
> +	/* [.klp.sym.]objname.symbol_name,sympos */
> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.symbol_name,[sympos] */
> +	if (!strchr(s, ','))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if a livepatch relocation section is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> +{
> +	char *secname;
> +	size_t len;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* [.klp.rela.]objname.section_name */
> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> +				KLP_RELASEC_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.rela.[objname].section_name */
> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if obj->name matches the objname encoded in the rela
> + * section name (.klp.rela.[objname].section_name)
> + *
> + * Must pass klp_check_relasec_format() before calling this.
> + */
> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> +				       struct klp_object *obj)
> +{
> +	size_t len;
> +	const char *obj_objname, *sec_objname, *secname;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* .klp.rela.[objname].section_name */
> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> +
> +	/* Get length of the objname encoded in the section name */
> +	len = strcspn(sec_objname, ".");
> +
> +	if (strlen(obj_objname) != len)
> +		return false;
> +
> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> +}
> +
> +/*
> + * klp_get_* helper functions
> + *
> + * klp_get_* functions extract different components of the name
> + * of a livepatch symbol. The full symbol name from the strtab
> + * is passed in as parameter @s, and @result is filled in with
> + * the extracted component.
> + *
> + * These functions assume a correctly formatted symbol and the
> + * klp_check_symbol_format() test *must* pass before calling any
> + * of these functions.
> + */
> +
> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *objname_start;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname_start = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname_start, ".");
> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> +	if (objname == NULL)
> +		return -ENOMEM;
> +
> +	/* klp_find_object_symbol() treats NULL as vmlinux */
> +	if (!strcmp(objname, "vmlinux")) {
> +		*result = NULL;
> +		kfree(objname);
> +	} else
> +		*result = objname;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *symname;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +
> +	*result = kstrndup(symname, len, GFP_KERNEL);
> +	if (*result == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.symbol_name,[sympos] */
> +static int klp_get_sympos(char *s, unsigned long *result)
> +{
> +	char *sympos;
> +
> +	/* .klp.sym.symbol_name,[sympos] */
> +	sympos = strchr(s, ',') + 1;
> +	return kstrtol(sympos, 10, result);
> +}
> +
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {

I think all the above string parsing code could be replaced with a
couple of sscanf() calls.

For example:

	char objname[64], symname[256];

	ret = sscanf(s, ".klp.sym.%63[^.].%255[^,],%u", objname, symname, &sympos);
	if (ret < 3)
		// string doesn't match expected format

That would be much simpler.

Only problem is, the kernel version of sscanf() doesn't seem to support
the '[' conversion specifier.  At least not yet ;-)  Adding support for
that would be a win-win: less code overall, and the addition of a useful
scanf feature which could be used by other code.

> @@ -204,74 +367,83 @@ 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 *relasec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> -
> -	/* 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;
> +	int i, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *s, *symbol_name, *sym_objname;
> +	unsigned long sympos;
> +
> +	relas = (Elf_Rela *) relasec->sh_addr;
> +	/* For each rela in this .klp.rela. section */
> +	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->symtab + ELF_R_SYM(relas[i].r_info);
> +
> +		/* Check if the symbol is formatted correctly */
> +		ret = klp_check_symbol_format(pmod, sym);
> +		if (ret)
> +			goto out;
> +		/* Format: .klp.sym.objname.symbol_name,sympos */
> +		s = pmod->strtab + sym->st_name;
> +		ret = klp_get_symbol_name(s, &symbol_name);
> +		if (ret)
> +			goto out;
> +		ret = klp_get_sym_objname(s, &sym_objname);
> +		if (ret)
> +			goto free_symbol_name;
> +		ret = klp_get_sympos(s, &sympos);
> +		if (ret)
> +			goto free_objname;

IMO whitespace after all the gotos would help with readability.

Also, since the "out" label is just a return, the "goto out"'s can all
just be replaced with returns.

> +
> +		ret = klp_find_object_symbol(sym_objname, symbol_name, sympos,
> +					     (unsigned long *) &sym->st_value);
> +free_objname:
> +		kfree(sym_objname);
> +free_symbol_name:
> +		kfree(symbol_name);
> +		if (ret)
> +			goto out;
>  	}
> -	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);
> +out:
> +	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, ret = 0;
> +	Elf_Shdr *sec;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> -
>  	module_disable_ro(pmod);
> +	/* For each klp relocation section */
> +	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> +		sec = pmod->klp_info->sechdrs + i;
> +		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> +			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);
> +		/* Check if the klp section is formatted correctly */
> +		ret = klp_check_relasec_format(pmod, sec);

I think this code is self-evident and the comment isn't needed.

>  		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);
> +		/* Check if the klp section belongs to obj */
> +		if (!klp_relasec_matches_object(pmod, sec, obj))
> +			continue;
> +
> +		/* Resolve all livepatch syms referenced in this rela section */
> +		ret = klp_resolve_symbols(sec, pmod);
> +		if (ret)
>  			goto out;
> -		}
> -	}
>  
> +		ret = apply_relocate_add(pmod->klp_info->sechdrs, pmod->strtab,
> +					 pmod->klp_info->symndx, i, pmod);
> +		if (ret)
> +			goto out;
> +	}
>  out:
>  	module_enable_ro(pmod);
>  	return ret;
> @@ -703,11 +875,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,
> @@ -842,6 +1012,9 @@ int klp_register_patch(struct klp_patch *patch)
>  {
>  	int ret;
>  
> +	if (!is_livepatch_module(patch->mod))
> +		return -EINVAL;
> +
>  	if (!klp_initialized())
>  		return -ENODEV;
>  
> -- 
> 2.4.3
> 

-- 
Josh

Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch

[Josh Poimboeuf]

On Mon, Feb 08, 2016 at 03:54:22PM +0100, Miroslav Benes wrote:
> On Wed, 3 Feb 2016, Jessica Yu wrote:
> 
> > 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 Elf format and requirements for
> >     patch modules
> 
> Hi,
> 
> I walked through the code and it looks good except for several minor 
> things in the fourth patch (livepatch: reuse module loader code to write 
> relocations). I'd propose to send the next version as a regular PATCH set 
> and not RFC. We can start collecting Reviews and Acks. Hopefully it won't 
> take more than one or two iterations. Would that be ok with everyone?

Sounds good to me...

-- 
Josh

Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch

[Josh Poimboeuf]

On Mon, Feb 08, 2016 at 03:54:22PM +0100, Miroslav Benes wrote:
> On Wed, 3 Feb 2016, Jessica Yu wrote:
> 
> > 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 Elf format and requirements for
> >     patch modules
> 
> Hi,
> 
> I walked through the code and it looks good except for several minor 
> things in the fourth patch (livepatch: reuse module loader code to write 
> relocations). I'd propose to send the next version as a regular PATCH set 
> and not RFC. We can start collecting Reviews and Acks. Hopefully it won't 
> take more than one or two iterations. Would that be ok with everyone?

Sounds good to me...

-- 
Josh

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/02/16 14:10 -0600]:
>On Wed, Feb 03, 2016 at 08:11:07PM -0500, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader. Persist copies of the
>> original symbol table and string table.
>>
>> Livepatch manages its own relocation sections in order to reuse module
>> loader code to write relocations. Livepatch modules must preserve Elf
>> information such as section indices in order to apply livepatch relocation
>> sections using the module loader's apply_relocate_add() function.
>>
>> In order to apply livepatch relocation sections, livepatch modules must
>> keep a complete copy of their original symbol table in memory. Normally, a
>> stripped down copy of a module's symbol table (containing only "core"
>> symbols) is made available through module->core_symtab. But for livepatch
>> modules, the symbol table copied into memory on module load must be exactly
>> the same as the symbol table produced when the patch module was compiled.
>> This is because the relocations in each livepatch relocation section refer
>> to their respective symbols with their symbol indices, and the original
>> symbol indices (and thus the symtab ordering) must be preserved in order
>> for apply_relocate_add() to find the right symbol.
>
>This patch didn't apply clean to linux-next/master.  I didn't
>investigate why, but maybe it depends on the other patch set which
>removes the notifiers?  (If so, that should be mentioned in the cover
>letter.)

A very recent commit (8244062ef) introduced some changes
kernel/module.c that intersect with this patch, so I think this is why
the patch didn't apply cleanly to today's tree...Anyhow I will rebase
again before sending out v5. Thanks for the comments :-)

Jessica

Re: module: preserve Elf information for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [08/02/16 14:10 -0600]:
>On Wed, Feb 03, 2016 at 08:11:07PM -0500, Jessica Yu wrote:
>> For livepatch modules, copy Elf section, symbol, and string information
>> from the load_info struct in the module loader. Persist copies of the
>> original symbol table and string table.
>>
>> Livepatch manages its own relocation sections in order to reuse module
>> loader code to write relocations. Livepatch modules must preserve Elf
>> information such as section indices in order to apply livepatch relocation
>> sections using the module loader's apply_relocate_add() function.
>>
>> In order to apply livepatch relocation sections, livepatch modules must
>> keep a complete copy of their original symbol table in memory. Normally, a
>> stripped down copy of a module's symbol table (containing only "core"
>> symbols) is made available through module->core_symtab. But for livepatch
>> modules, the symbol table copied into memory on module load must be exactly
>> the same as the symbol table produced when the patch module was compiled.
>> This is because the relocations in each livepatch relocation section refer
>> to their respective symbols with their symbol indices, and the original
>> symbol indices (and thus the symtab ordering) must be preserved in order
>> for apply_relocate_add() to find the right symbol.
>
>This patch didn't apply clean to linux-next/master.  I didn't
>investigate why, but maybe it depends on the other patch set which
>removes the notifiers?  (If so, that should be mentioned in the cover
>letter.)

A very recent commit (8244062ef) introduced some changes
kernel/module.c that intersect with this patch, so I think this is why
the patch didn't apply cleanly to today's tree...Anyhow I will rebase
again before sending out v5. Thanks for the comments :-)

Jessica

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

[Petr Mladek]

On Wed 2016-02-03 20:11:07, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader. Persist copies of the
> original symbol table and string table.
> 
> Livepatch manages its own relocation sections in order to reuse module
> loader code to write relocations. Livepatch modules must preserve Elf
> information such as section indices in order to apply livepatch relocation
> sections using the module loader's apply_relocate_add() function.
> 
> In order to apply livepatch relocation sections, livepatch modules must
> keep a complete copy of their original symbol table in memory. Normally, a
> stripped down copy of a module's symbol table (containing only "core"
> symbols) is made available through module->core_symtab. But for livepatch
> modules, the symbol table copied into memory on module load must be exactly
> the same as the symbol table produced when the patch module was compiled.
> This is because the relocations in each livepatch relocation section refer
> to their respective symbols with their symbol indices, and the original
> symbol indices (and thus the symtab ordering) must be preserved in order
> for apply_relocate_add() to find the right symbol.
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
>  	 */
>  	current->flags &= ~PF_USED_ASYNC;
>  
> +#ifdef CONFIG_KALLSYMS
> +	/* Make symtab and strtab available prior to module init call */
> +	mod->num_symtab = mod->core_num_syms;
> +	mod->symtab = mod->core_symtab;
> +	mod->strtab = mod->core_strtab;
> +#endif

This should be done with module_mutex. Otherwise, it looks racy
at least against module_kallsyms_on_each_symbol().

BTW: I wonder why even the original code is not racy
for example against module_get_kallsym. It is called
without the mutex. This code sets the number of entries
before the pointer to the entries.

Note that the module is in the list even in the UNFORMED state.


>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> @@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
>  	/* Drop initial reference. */
>  	module_put(mod);
>  	trim_init_extable(mod);
> -#ifdef CONFIG_KALLSYMS
> -	mod->num_symtab = mod->core_num_syms;
> -	mod->symtab = mod->core_symtab;
> -	mod->strtab = mod->core_strtab;
> -#endif
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);

In each case, it was called with the mutex here.

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2016-02-03 20:11:07, Jessica Yu wrote:
> For livepatch modules, copy Elf section, symbol, and string information
> from the load_info struct in the module loader. Persist copies of the
> original symbol table and string table.
> 
> Livepatch manages its own relocation sections in order to reuse module
> loader code to write relocations. Livepatch modules must preserve Elf
> information such as section indices in order to apply livepatch relocation
> sections using the module loader's apply_relocate_add() function.
> 
> In order to apply livepatch relocation sections, livepatch modules must
> keep a complete copy of their original symbol table in memory. Normally, a
> stripped down copy of a module's symbol table (containing only "core"
> symbols) is made available through module->core_symtab. But for livepatch
> modules, the symbol table copied into memory on module load must be exactly
> the same as the symbol table produced when the patch module was compiled.
> This is because the relocations in each livepatch relocation section refer
> to their respective symbols with their symbol indices, and the original
> symbol indices (and thus the symtab ordering) must be preserved in order
> for apply_relocate_add() to find the right symbol.
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3222,6 +3331,12 @@ static noinline int do_init_module(struct module *mod)
>  	 */
>  	current->flags &= ~PF_USED_ASYNC;
>  
> +#ifdef CONFIG_KALLSYMS
> +	/* Make symtab and strtab available prior to module init call */
> +	mod->num_symtab = mod->core_num_syms;
> +	mod->symtab = mod->core_symtab;
> +	mod->strtab = mod->core_strtab;
> +#endif

This should be done with module_mutex. Otherwise, it looks racy
at least against module_kallsyms_on_each_symbol().

BTW: I wonder why even the original code is not racy
for example against module_get_kallsym. It is called
without the mutex. This code sets the number of entries
before the pointer to the entries.

Note that the module is in the list even in the UNFORMED state.


>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> @@ -3266,11 +3381,6 @@ static noinline int do_init_module(struct module *mod)
>  	/* Drop initial reference. */
>  	module_put(mod);
>  	trim_init_extable(mod);
> -#ifdef CONFIG_KALLSYMS
> -	mod->num_symtab = mod->core_num_syms;
> -	mod->symtab = mod->core_symtab;
> -	mod->strtab = mod->core_strtab;
> -#endif
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);

In each case, it was called with the mutex here.

Best Regards,
Petr

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

[Jiri Kosina]

On Tue, 9 Feb 2016, Petr Mladek wrote:

> > +#ifdef CONFIG_KALLSYMS
> > +	/* Make symtab and strtab available prior to module init call */
> > +	mod->num_symtab = mod->core_num_syms;
> > +	mod->symtab = mod->core_symtab;
> > +	mod->strtab = mod->core_strtab;
> > +#endif
> 
> This should be done with module_mutex. Otherwise, it looks racy at least 
> against module_kallsyms_on_each_symbol().

Hmm, if this is the case, the comment describing the locking rules for 
module_mutex should be updated.

> BTW: I wonder why even the original code is not racy for example against 
> module_get_kallsym. It is called without the mutex. This code sets the 
> number of entries before the pointer to the entries.
> 
> Note that the module is in the list even in the UNFORMED state.

module_kallsyms_on_each_symbol() gives up in such case though.

-- 
Jiri Kosina
SUSE Labs

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

[Petr Mladek]

On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
> On Tue, 9 Feb 2016, Petr Mladek wrote:
> 
> > > +#ifdef CONFIG_KALLSYMS
> > > +	/* Make symtab and strtab available prior to module init call */
> > > +	mod->num_symtab = mod->core_num_syms;
> > > +	mod->symtab = mod->core_symtab;
> > > +	mod->strtab = mod->core_strtab;
> > > +#endif
> > 
> > This should be done with module_mutex. Otherwise, it looks racy at least 
> > against module_kallsyms_on_each_symbol().
> 
> Hmm, if this is the case, the comment describing the locking rules for 
> module_mutex should be updated.
> 
> > BTW: I wonder why even the original code is not racy for example against 
> > module_get_kallsym. It is called without the mutex. This code sets the 
> > number of entries before the pointer to the entries.
> > 
> > Note that the module is in the list even in the UNFORMED state.
> 
> module_kallsyms_on_each_symbol() gives up in such case though.

The problem is that we set the three values above in the COMMING
state. They were even set in the LIVE state before this patch.

IMHO, we should reorder the writes and add a write barrier.


#ifdef CONFIG_KALLSYMS
	/* Make symtab and strtab available prior to module init call */
	mod->strtab = mod->core_strtab;
	mod->symtab = mod->core_symtab;
	/* Tables must be availabe before the number is set */
	smp_wmb();
	mod->num_symtab = mod->core_num_syms;
#endif


Then we should add the corresponding read barrier to
the read functions that are protected only by
the disabled preeption. For example:


static unsigned long mod_find_symname(struct module *mod, const char *name)
{
	unsigned int i;

	for (i = 0; i < mod->num_symtab; i++)
		/* Make sure that tables are set for the read num_symtab */
		smp_rmb();
		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
		    mod->symtab[i].st_info != 'U')
			return mod->symtab[i].st_value;
	return 0;
}


Or am I too paranoid, please?

Best Regards,
Petr

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

[Petr Mladek]

On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
> On Tue, 9 Feb 2016, Petr Mladek wrote:
> 
> > > +#ifdef CONFIG_KALLSYMS
> > > +	/* Make symtab and strtab available prior to module init call */
> > > +	mod->num_symtab = mod->core_num_syms;
> > > +	mod->symtab = mod->core_symtab;
> > > +	mod->strtab = mod->core_strtab;
> > > +#endif
> > 
> > This should be done with module_mutex. Otherwise, it looks racy at least 
> > against module_kallsyms_on_each_symbol().
> 
> Hmm, if this is the case, the comment describing the locking rules for 
> module_mutex should be updated.
> 
> > BTW: I wonder why even the original code is not racy for example against 
> > module_get_kallsym. It is called without the mutex. This code sets the 
> > number of entries before the pointer to the entries.
> > 
> > Note that the module is in the list even in the UNFORMED state.
> 
> module_kallsyms_on_each_symbol() gives up in such case though.

The problem is that we set the three values above in the COMMING
state. They were even set in the LIVE state before this patch.

IMHO, we should reorder the writes and add a write barrier.


#ifdef CONFIG_KALLSYMS
	/* Make symtab and strtab available prior to module init call */
	mod->strtab = mod->core_strtab;
	mod->symtab = mod->core_symtab;
	/* Tables must be availabe before the number is set */
	smp_wmb();
	mod->num_symtab = mod->core_num_syms;
#endif


Then we should add the corresponding read barrier to
the read functions that are protected only by
the disabled preeption. For example:


static unsigned long mod_find_symname(struct module *mod, const char *name)
{
	unsigned int i;

	for (i = 0; i < mod->num_symtab; i++)
		/* Make sure that tables are set for the read num_symtab */
		smp_rmb();
		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
		    mod->symtab[i].st_info != 'U')
			return mod->symtab[i].st_value;
	return 0;
}


Or am I too paranoid, please?

Best Regards,
Petr

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

[Miroslav Benes]

> > +/* .klp.sym.[objname].symbol_name,sympos */
> > +static int klp_get_sym_objname(char *s, char **result)
> > +{
> 
> Do we need result to be a double-pointer? If I am not mistaken just 'char 
> *result' could be sufficient. You check the return value, so result could 
> be NULL or objname as found. No?

Um, no. We need this. Sorry for the noise.

Miroslav

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

[Petr Mladek]

On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the need
> for architecture specific relocation code in livepatch. Specifically, reuse
> the apply_relocate_add() function in the module loader to write relocations
> instead of duplicating functionality in livepatch's arch-dependent
> klp_write_module_reloc() function.
> 
> In order to accomplish this, livepatch modules manage their own relocation
> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> index). To apply livepatch relocation sections, 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 and the s390
> klp_write_module_reloc() function stub. They are no longer needed since
> relocation work has been offloaded to module loader.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 7aa975d..c1fe57c 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>
>  
>  /**
> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  	return !obj->name || obj->mod;
>  }
>  
> +/*
> + * Check if a livepatch symbol is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> +{
> +	size_t len;
> +	char *s, *objname, *symname;
> +
> +	if (sym->st_shndx != SHN_LIVEPATCH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Livepatch symbol names must follow this format:
> +	 * .klp.sym.objname.symbol_name,sympos
> +	 */
> +	s = pmod->strtab + sym->st_name;
> +	/* [.klp.sym.]objname.symbol_name,sympos */
> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.symbol_name,[sympos] */
> +	if (!strchr(s, ','))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if a livepatch relocation section is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> +{
> +	char *secname;
> +	size_t len;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* [.klp.rela.]objname.section_name */
> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> +				KLP_RELASEC_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.rela.[objname].section_name */
> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if obj->name matches the objname encoded in the rela
> + * section name (.klp.rela.[objname].section_name)
> + *
> + * Must pass klp_check_relasec_format() before calling this.
> + */
> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> +				       struct klp_object *obj)
> +{
> +	size_t len;
> +	const char *obj_objname, *sec_objname, *secname;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* .klp.rela.[objname].section_name */
> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> +
> +	/* Get length of the objname encoded in the section name */
> +	len = strcspn(sec_objname, ".");
> +
> +	if (strlen(obj_objname) != len)
> +		return false;
> +
> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> +}
> +
> +/*
> + * klp_get_* helper functions
> + *
> + * klp_get_* functions extract different components of the name
> + * of a livepatch symbol. The full symbol name from the strtab
> + * is passed in as parameter @s, and @result is filled in with
> + * the extracted component.
> + *
> + * These functions assume a correctly formatted symbol and the
> + * klp_check_symbol_format() test *must* pass before calling any
> + * of these functions.
> + */
> +
> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *objname_start;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname_start = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname_start, ".");
> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> +	if (objname == NULL)
> +		return -ENOMEM;
> +
> +	/* klp_find_object_symbol() treats NULL as vmlinux */
> +	if (!strcmp(objname, "vmlinux")) {
> +		*result = NULL;
> +		kfree(objname);
> +	} else
> +		*result = objname;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *symname;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +
> +	*result = kstrndup(symname, len, GFP_KERNEL);
> +	if (*result == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.symbol_name,[sympos] */
> +static int klp_get_sympos(char *s, unsigned long *result)
> +{
> +	char *sympos;
> +
> +	/* .klp.sym.symbol_name,[sympos] */
> +	sympos = strchr(s, ',') + 1;
> +	return kstrtol(sympos, 10, result);
> +}

The usage of the helper function is nicely strightforward.
Also I like a lot all the comments with the [] brackets
that highlight what each check or search is for.

But I think that there is a lot of duplicated code in
the check_ and in the get_ functions. Also there is
a lot of strdup/free games.

The length of the elemets is limited by definition.
The functions are called under klp_mutex.

Therefore it might be easier to maintain a two parse
function that would fill static buffers and return
error in case of wrong value.

I mean something like:

static char mod_name[MODULE_NAME_LEN + 1];
static char symbol_name[KSYM_SYMBOL_LEN + 1];

static int
klp_parse_symbol_format(struct module *pmod, Elf_Sym *sym,
			char *mod_name, char *symbol_name,
			int *sympos)
{
	char *substr;


	/*
	 * Livepatch symbol names must follow this format:
	 * .klp.sym.objname.symbol_name,sympos
	 */
	s = pmod->strtab + sym->st_name;
	/* [.klp.sym.]objname.symbol_name,sympos */
	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
		return -EINVAL;

	/* .klp.sym.[objname].symbol_name,sympos */
	substr = s + KLP_SYM_PREFIX_LEN;
	len = strcspn(substr, ".");
	if (!len || len > MODULE_NAME_LEN)
		return -EINVAL;
	strncpy(mod_name, substr, len);
	mod_name[len] = '\0';

	/* .klp.sym.objname.[symbol_name],sympos */
	substr = substr + len + 1;
	len = strcspn(substr, ",");
	if (!len || len > KSYM_SYMBOL_LEN)
		return -EINVAL;
	strncpy(symbol_name, substr, len);
	mod_name[len] = '\0';

	/* .klp.sym.objname.symbol_name,[sympos] */
	substr = substr + len + 1;
	len = strlen(substr);
	if (!len)
		return -EINVAL;

	return kstrtol(substr, 10, sympos);
}

How does that sound, please?

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
> Reuse module loader code to write relocations, thereby eliminating the need
> for architecture specific relocation code in livepatch. Specifically, reuse
> the apply_relocate_add() function in the module loader to write relocations
> instead of duplicating functionality in livepatch's arch-dependent
> klp_write_module_reloc() function.
> 
> In order to accomplish this, livepatch modules manage their own relocation
> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> index). To apply livepatch relocation sections, 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 and the s390
> klp_write_module_reloc() function stub. They are no longer needed since
> relocation work has been offloaded to module loader.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 7aa975d..c1fe57c 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>
>  
>  /**
> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>  	return !obj->name || obj->mod;
>  }
>  
> +/*
> + * Check if a livepatch symbol is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> +{
> +	size_t len;
> +	char *s, *objname, *symname;
> +
> +	if (sym->st_shndx != SHN_LIVEPATCH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Livepatch symbol names must follow this format:
> +	 * .klp.sym.objname.symbol_name,sympos
> +	 */
> +	s = pmod->strtab + sym->st_name;
> +	/* [.klp.sym.]objname.symbol_name,sympos */
> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.symbol_name,[sympos] */
> +	if (!strchr(s, ','))
> +		return -EINVAL;
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if a livepatch relocation section is formatted properly.
> + *
> + * See Documentation/livepatch/module-elf-format.txt for a
> + * detailed outline of requirements.
> + */
> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> +{
> +	char *secname;
> +	size_t len;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* [.klp.rela.]objname.section_name */
> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> +				KLP_RELASEC_PREFIX_LEN))
> +		return -EINVAL;
> +
> +	/* .klp.rela.[objname].section_name */
> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> +	if (!(len > 0))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Check if obj->name matches the objname encoded in the rela
> + * section name (.klp.rela.[objname].section_name)
> + *
> + * Must pass klp_check_relasec_format() before calling this.
> + */
> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> +				       struct klp_object *obj)
> +{
> +	size_t len;
> +	const char *obj_objname, *sec_objname, *secname;
> +
> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> +	/* .klp.rela.[objname].section_name */
> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> +
> +	/* Get length of the objname encoded in the section name */
> +	len = strcspn(sec_objname, ".");
> +
> +	if (strlen(obj_objname) != len)
> +		return false;
> +
> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> +}
> +
> +/*
> + * klp_get_* helper functions
> + *
> + * klp_get_* functions extract different components of the name
> + * of a livepatch symbol. The full symbol name from the strtab
> + * is passed in as parameter @s, and @result is filled in with
> + * the extracted component.
> + *
> + * These functions assume a correctly formatted symbol and the
> + * klp_check_symbol_format() test *must* pass before calling any
> + * of these functions.
> + */
> +
> +/* .klp.sym.[objname].symbol_name,sympos */
> +static int klp_get_sym_objname(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *objname_start;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname_start = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname_start, ".");
> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> +	if (objname == NULL)
> +		return -ENOMEM;
> +
> +	/* klp_find_object_symbol() treats NULL as vmlinux */
> +	if (!strcmp(objname, "vmlinux")) {
> +		*result = NULL;
> +		kfree(objname);
> +	} else
> +		*result = objname;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.[symbol_name],sympos */
> +static int klp_get_symbol_name(char *s, char **result)
> +{
> +	size_t len;
> +	char *objname, *symname;
> +
> +	/* .klp.sym.[objname].symbol_name,sympos */
> +	objname = s + KLP_SYM_PREFIX_LEN;
> +	len = strcspn(objname, ".");
> +
> +	/* .klp.sym.objname.[symbol_name],sympos */
> +	symname = objname + len + 1;
> +	len = strcspn(symname, ",");
> +
> +	*result = kstrndup(symname, len, GFP_KERNEL);
> +	if (*result == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* .klp.sym.objname.symbol_name,[sympos] */
> +static int klp_get_sympos(char *s, unsigned long *result)
> +{
> +	char *sympos;
> +
> +	/* .klp.sym.symbol_name,[sympos] */
> +	sympos = strchr(s, ',') + 1;
> +	return kstrtol(sympos, 10, result);
> +}

The usage of the helper function is nicely strightforward.
Also I like a lot all the comments with the [] brackets
that highlight what each check or search is for.

But I think that there is a lot of duplicated code in
the check_ and in the get_ functions. Also there is
a lot of strdup/free games.

The length of the elemets is limited by definition.
The functions are called under klp_mutex.

Therefore it might be easier to maintain a two parse
function that would fill static buffers and return
error in case of wrong value.

I mean something like:

static char mod_name[MODULE_NAME_LEN + 1];
static char symbol_name[KSYM_SYMBOL_LEN + 1];

static int
klp_parse_symbol_format(struct module *pmod, Elf_Sym *sym,
			char *mod_name, char *symbol_name,
			int *sympos)
{
	char *substr;


	/*
	 * Livepatch symbol names must follow this format:
	 * .klp.sym.objname.symbol_name,sympos
	 */
	s = pmod->strtab + sym->st_name;
	/* [.klp.sym.]objname.symbol_name,sympos */
	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
		return -EINVAL;

	/* .klp.sym.[objname].symbol_name,sympos */
	substr = s + KLP_SYM_PREFIX_LEN;
	len = strcspn(substr, ".");
	if (!len || len > MODULE_NAME_LEN)
		return -EINVAL;
	strncpy(mod_name, substr, len);
	mod_name[len] = '\0';

	/* .klp.sym.objname.[symbol_name],sympos */
	substr = substr + len + 1;
	len = strcspn(substr, ",");
	if (!len || len > KSYM_SYMBOL_LEN)
		return -EINVAL;
	strncpy(symbol_name, substr, len);
	mod_name[len] = '\0';

	/* .klp.sym.objname.symbol_name,[sympos] */
	substr = substr + len + 1;
	len = strlen(substr);
	if (!len)
		return -EINVAL;

	return kstrtol(substr, 10, sympos);
}

How does that sound, please?

Best Regards,
Petr

Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch

[Petr Mladek]

On Wed 2016-02-03 20:11:05, Jessica Yu wrote:
> 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.
> 
> Jessica Yu (6):
>   Elf: add livepatch-specific Elf constants
>   module: s390: keep mod_arch_specific for livepatch modules
>   samples: livepatch: mark as livepatch module
>   Documentation: livepatch: outline Elf format and requirements for
>     patch modules

For the four above patches:

Reviewed-by: Petr Mladek <pmladek@suse.com>


>   module: preserve Elf information for livepatch modules
>   livepatch: reuse module loader code to write relocations

These two still need some tweaking.

I think that we are getting close. I like the way we go.
The documentation is excellent.

Thanks a lot for working on it.

Best Regards,
Petr

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

[Miroslav Benes]

On Tue, 9 Feb 2016, Petr Mladek wrote:

> On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
> > Reuse module loader code to write relocations, thereby eliminating the need
> > for architecture specific relocation code in livepatch. Specifically, reuse
> > the apply_relocate_add() function in the module loader to write relocations
> > instead of duplicating functionality in livepatch's arch-dependent
> > klp_write_module_reloc() function.
> > 
> > In order to accomplish this, livepatch modules manage their own relocation
> > sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> > livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> > index). To apply livepatch relocation sections, 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 and the s390
> > klp_write_module_reloc() function stub. They are no longer needed since
> > relocation work has been offloaded to module loader.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 7aa975d..c1fe57c 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>
> >  
> >  /**
> > @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
> >  	return !obj->name || obj->mod;
> >  }
> >  
> > +/*
> > + * Check if a livepatch symbol is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> > +{
> > +	size_t len;
> > +	char *s, *objname, *symname;
> > +
> > +	if (sym->st_shndx != SHN_LIVEPATCH)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Livepatch symbol names must follow this format:
> > +	 * .klp.sym.objname.symbol_name,sympos
> > +	 */
> > +	s = pmod->strtab + sym->st_name;
> > +	/* [.klp.sym.]objname.symbol_name,sympos */
> > +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname, ".");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.objname.symbol_name,[sympos] */
> > +	if (!strchr(s, ','))
> > +		return -EINVAL;
> > +
> > +	/* .klp.sym.objname.[symbol_name],sympos */
> > +	symname = objname + len + 1;
> > +	len = strcspn(symname, ",");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check if a livepatch relocation section is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> > +{
> > +	char *secname;
> > +	size_t len;
> > +
> > +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +	/* [.klp.rela.]objname.section_name */
> > +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> > +				KLP_RELASEC_PREFIX_LEN))
> > +		return -EINVAL;
> > +
> > +	/* .klp.rela.[objname].section_name */
> > +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> > +	if (!(len > 0))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check if obj->name matches the objname encoded in the rela
> > + * section name (.klp.rela.[objname].section_name)
> > + *
> > + * Must pass klp_check_relasec_format() before calling this.
> > + */
> > +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
> > +				       struct klp_object *obj)
> > +{
> > +	size_t len;
> > +	const char *obj_objname, *sec_objname, *secname;
> > +
> > +	secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +	/* .klp.rela.[objname].section_name */
> > +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> > +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > +
> > +	/* Get length of the objname encoded in the section name */
> > +	len = strcspn(sec_objname, ".");
> > +
> > +	if (strlen(obj_objname) != len)
> > +		return false;
> > +
> > +	return strncmp(sec_objname, obj_objname, len) ? false : true;
> > +}
> > +
> > +/*
> > + * klp_get_* helper functions
> > + *
> > + * klp_get_* functions extract different components of the name
> > + * of a livepatch symbol. The full symbol name from the strtab
> > + * is passed in as parameter @s, and @result is filled in with
> > + * the extracted component.
> > + *
> > + * These functions assume a correctly formatted symbol and the
> > + * klp_check_symbol_format() test *must* pass before calling any
> > + * of these functions.
> > + */
> > +
> > +/* .klp.sym.[objname].symbol_name,sympos */
> > +static int klp_get_sym_objname(char *s, char **result)
> > +{
> > +	size_t len;
> > +	char *objname, *objname_start;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname_start = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname_start, ".");
> > +	objname = kstrndup(objname_start, len, GFP_KERNEL);
> > +	if (objname == NULL)
> > +		return -ENOMEM;
> > +
> > +	/* klp_find_object_symbol() treats NULL as vmlinux */
> > +	if (!strcmp(objname, "vmlinux")) {
> > +		*result = NULL;
> > +		kfree(objname);
> > +	} else
> > +		*result = objname;
> > +
> > +	return 0;
> > +}
> > +
> > +/* .klp.sym.objname.[symbol_name],sympos */
> > +static int klp_get_symbol_name(char *s, char **result)
> > +{
> > +	size_t len;
> > +	char *objname, *symname;
> > +
> > +	/* .klp.sym.[objname].symbol_name,sympos */
> > +	objname = s + KLP_SYM_PREFIX_LEN;
> > +	len = strcspn(objname, ".");
> > +
> > +	/* .klp.sym.objname.[symbol_name],sympos */
> > +	symname = objname + len + 1;
> > +	len = strcspn(symname, ",");
> > +
> > +	*result = kstrndup(symname, len, GFP_KERNEL);
> > +	if (*result == NULL)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +
> > +/* .klp.sym.objname.symbol_name,[sympos] */
> > +static int klp_get_sympos(char *s, unsigned long *result)
> > +{
> > +	char *sympos;
> > +
> > +	/* .klp.sym.symbol_name,[sympos] */
> > +	sympos = strchr(s, ',') + 1;
> > +	return kstrtol(sympos, 10, result);
> > +}
> 
> The usage of the helper function is nicely strightforward.
> Also I like a lot all the comments with the [] brackets
> that highlight what each check or search is for.
> 
> But I think that there is a lot of duplicated code in
> the check_ and in the get_ functions. Also there is
> a lot of strdup/free games.
> 
> The length of the elemets is limited by definition.
> The functions are called under klp_mutex.
> 
> Therefore it might be easier to maintain a two parse
> function that would fill static buffers and return
> error in case of wrong value.
> 
> I mean something like:
> 
> static char mod_name[MODULE_NAME_LEN + 1];
> static char symbol_name[KSYM_SYMBOL_LEN + 1];
> 
> static int
> klp_parse_symbol_format(struct module *pmod, Elf_Sym *sym,
> 			char *mod_name, char *symbol_name,
> 			int *sympos)
> {
> 	char *substr;
> 
> 
> 	/*
> 	 * Livepatch symbol names must follow this format:
> 	 * .klp.sym.objname.symbol_name,sympos
> 	 */
> 	s = pmod->strtab + sym->st_name;
> 	/* [.klp.sym.]objname.symbol_name,sympos */
> 	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> 		return -EINVAL;
> 
> 	/* .klp.sym.[objname].symbol_name,sympos */
> 	substr = s + KLP_SYM_PREFIX_LEN;
> 	len = strcspn(substr, ".");
> 	if (!len || len > MODULE_NAME_LEN)
> 		return -EINVAL;
> 	strncpy(mod_name, substr, len);
> 	mod_name[len] = '\0';
> 
> 	/* .klp.sym.objname.[symbol_name],sympos */
> 	substr = substr + len + 1;
> 	len = strcspn(substr, ",");
> 	if (!len || len > KSYM_SYMBOL_LEN)
> 		return -EINVAL;
> 	strncpy(symbol_name, substr, len);
> 	mod_name[len] = '\0';
> 
> 	/* .klp.sym.objname.symbol_name,[sympos] */
> 	substr = substr + len + 1;
> 	len = strlen(substr);
> 	if (!len)
> 		return -EINVAL;
> 
> 	return kstrtol(substr, 10, sympos);
> }
> 
> How does that sound, please?

It sounds good to me. And it would be even better with Josh's idea of 
sscanf included.

Miroslav

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

[Rusty Russell]

Petr Mladek <pmladek@suse.com> writes:
> On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
>> On Tue, 9 Feb 2016, Petr Mladek wrote:
>> 
>> > > +#ifdef CONFIG_KALLSYMS
>> > > +	/* Make symtab and strtab available prior to module init call */
>> > > +	mod->num_symtab = mod->core_num_syms;
>> > > +	mod->symtab = mod->core_symtab;
>> > > +	mod->strtab = mod->core_strtab;
>> > > +#endif
>> > 
>> > This should be done with module_mutex. Otherwise, it looks racy at least 
>> > against module_kallsyms_on_each_symbol().
>> 
>> Hmm, if this is the case, the comment describing the locking rules for 
>> module_mutex should be updated.
>> 
>> > BTW: I wonder why even the original code is not racy for example against 
>> > module_get_kallsym. It is called without the mutex. This code sets the 
>> > number of entries before the pointer to the entries.
>> > 
>> > Note that the module is in the list even in the UNFORMED state.
>> 
>> module_kallsyms_on_each_symbol() gives up in such case though.
>
> The problem is that we set the three values above in the COMMING
> state. They were even set in the LIVE state before this patch.

True.  Indeed, I have a fix for exactly this queued in my fixes tree,
and am about to send Linus a pull req.

Below is the fix I went with, for your reference (part of a series, but
you get the idea).

Thanks!
Rusty.

commit 8244062ef1e54502ef55f54cced659913f244c3e
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Wed Feb 3 16:55:26 2016 +1030

    modules: fix longstanding /proc/kallsyms vs module insertion race.
    
    For CONFIG_KALLSYMS, we keep two symbol tables and two string tables.
    There's one full copy, marked SHF_ALLOC and laid out at the end of the
    module's init section.  There's also a cut-down version that only
    contains core symbols and strings, and lives in the module's core
    section.
    
    After module init (and before we free the module memory), we switch
    the mod->symtab, mod->num_symtab and mod->strtab to point to the core
    versions.  We do this under the module_mutex.
    
    However, kallsyms doesn't take the module_mutex: it uses
    preempt_disable() and rcu tricks to walk through the modules, because
    it's used in the oops path.  It's also used in /proc/kallsyms.
    There's nothing atomic about the change of these variables, so we can
    get the old (larger!) num_symtab and the new symtab pointer; in fact
    this is what I saw when trying to reproduce.
    
    By grouping these variables together, we can use a
    carefully-dereferenced pointer to ensure we always get one or the
    other (the free of the module init section is already done in an RCU
    callback, so that's safe).  We allocate the init one at the end of the
    module init section, and keep the core one inside the struct module
    itself (it could also have been allocated at the end of the module
    core, but that's probably overkill).
    
    Reported-by: Weilong Chen <chenweilong@huawei.com>
    Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111541
    Cc: stable@kernel.org
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/module.h b/include/linux/module.h
index 4560d8f1545d..2bb0c3085706 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -324,6 +324,12 @@ struct module_layout {
 #define __module_layout_align
 #endif
 
+struct mod_kallsyms {
+	Elf_Sym *symtab;
+	unsigned int num_symtab;
+	char *strtab;
+};
+
 struct module {
 	enum module_state state;
 
@@ -405,15 +411,10 @@ struct module {
 #endif
 
 #ifdef CONFIG_KALLSYMS
-	/*
-	 * 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).
-	 */
-	Elf_Sym *symtab, *core_symtab;
-	unsigned int num_symtab, core_num_syms;
-	char *strtab, *core_strtab;
-
+	/* Protected by RCU and/or module_mutex: use rcu_dereference() */
+	struct mod_kallsyms *kallsyms;
+	struct mod_kallsyms core_kallsyms;
+	
 	/* Section attributes */
 	struct module_sect_attrs *sect_attrs;
 
diff --git a/kernel/module.c b/kernel/module.c
index 1e79d8157712..9537da37ce87 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -303,6 +303,9 @@ struct load_info {
 	struct _ddebug *debug;
 	unsigned int num_debug;
 	bool sig_ok;
+#ifdef CONFIG_KALLSYMS
+	unsigned long mod_kallsyms_init_off;
+#endif
 	struct {
 		unsigned int sym, str, mod, vers, info, pcpu;
 	} index;
@@ -2480,10 +2483,21 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 	strsect->sh_flags |= SHF_ALLOC;
 	strsect->sh_entsize = get_offset(mod, &mod->init_layout.size, strsect,
 					 info->index.str) | INIT_OFFSET_MASK;
-	mod->init_layout.size = debug_align(mod->init_layout.size);
 	pr_debug("\t%s\n", info->secstrings + strsect->sh_name);
+
+	/* We'll tack temporary mod_kallsyms on the end. */
+	mod->init_layout.size = ALIGN(mod->init_layout.size,
+				      __alignof__(struct mod_kallsyms));
+	info->mod_kallsyms_init_off = mod->init_layout.size;
+	mod->init_layout.size += sizeof(struct mod_kallsyms);
+	mod->init_layout.size = debug_align(mod->init_layout.size);
 }
 
+/*
+ * We use the full symtab and strtab which layout_symtab arranged to
+ * be appended to the init section.  Later we switch to the cut-down
+ * core-only ones.
+ */
 static void add_kallsyms(struct module *mod, const struct load_info *info)
 {
 	unsigned int i, ndst;
@@ -2492,29 +2506,34 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	char *s;
 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
 
-	mod->symtab = (void *)symsec->sh_addr;
-	mod->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
+	/* Set up to point into init section. */
+	mod->kallsyms = mod->init_layout.base + info->mod_kallsyms_init_off;
+
+	mod->kallsyms->symtab = (void *)symsec->sh_addr;
+	mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym);
 	/* Make sure we get permanent strtab: don't use info->strtab. */
-	mod->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
+	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
 
 	/* Set types up while we still have access to sections. */
-	for (i = 0; i < mod->num_symtab; i++)
-		mod->symtab[i].st_info = elf_type(&mod->symtab[i], info);
-
-	mod->core_symtab = dst = mod->core_layout.base + info->symoffs;
-	mod->core_strtab = s = mod->core_layout.base + info->stroffs;
-	src = mod->symtab;
-	for (ndst = i = 0; i < mod->num_symtab; i++) {
+	for (i = 0; i < mod->kallsyms->num_symtab; i++)
+		mod->kallsyms->symtab[i].st_info
+			= elf_type(&mod->kallsyms->symtab[i], info);
+
+	/* Now populate the cut down core kallsyms for after init. */
+	mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
+	mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
+	src = mod->kallsyms->symtab;
+	for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) {
 		if (i == 0 ||
 		    is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			dst[ndst] = src[i];
-			dst[ndst++].st_name = s - mod->core_strtab;
-			s += strlcpy(s, &mod->strtab[src[i].st_name],
+			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
+			s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name],
 				     KSYM_NAME_LEN) + 1;
 		}
 	}
-	mod->core_num_syms = ndst;
+	mod->core_kallsyms.num_symtab = ndst;
 }
 #else
 static inline void layout_symtab(struct module *mod, struct load_info *info)
@@ -3263,9 +3282,8 @@ static noinline int do_init_module(struct module *mod)
 	module_put(mod);
 	trim_init_extable(mod);
 #ifdef CONFIG_KALLSYMS
-	mod->num_symtab = mod->core_num_syms;
-	mod->symtab = mod->core_symtab;
-	mod->strtab = mod->core_strtab;
+	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
+	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
 	mod_tree_remove_init(mod);
 	disable_ro_nx(&mod->init_layout);
@@ -3627,9 +3645,9 @@ static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
-static const char *symname(struct module *mod, unsigned int symnum)
+static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
 {
-	return mod->strtab + mod->symtab[symnum].st_name;
+	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
 }
 
 static const char *get_ksymbol(struct module *mod,
@@ -3639,6 +3657,7 @@ static const char *get_ksymbol(struct module *mod,
 {
 	unsigned int i, best = 0;
 	unsigned long nextval;
+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
 
 	/* At worse, next value is at end of module */
 	if (within_module_init(addr, mod))
@@ -3648,32 +3667,32 @@ static const char *get_ksymbol(struct module *mod,
 
 	/* Scan for closest preceding symbol, and next symbol. (ELF
 	   starts real symbols at 1). */
-	for (i = 1; i < mod->num_symtab; i++) {
-		if (mod->symtab[i].st_shndx == SHN_UNDEF)
+	for (i = 1; i < kallsyms->num_symtab; i++) {
+		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
 			continue;
 
 		/* We ignore unnamed symbols: they're uninformative
 		 * and inserted at a whim. */
-		if (*symname(mod, i) == '\0'
-		    || is_arm_mapping_symbol(symname(mod, i)))
+		if (*symname(kallsyms, i) == '\0'
+		    || is_arm_mapping_symbol(symname(kallsyms, i)))
 			continue;
 
-		if (mod->symtab[i].st_value <= addr
-		    && mod->symtab[i].st_value > mod->symtab[best].st_value)
+		if (kallsyms->symtab[i].st_value <= addr
+		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
 			best = i;
-		if (mod->symtab[i].st_value > addr
-		    && mod->symtab[i].st_value < nextval)
-			nextval = mod->symtab[i].st_value;
+		if (kallsyms->symtab[i].st_value > addr
+		    && kallsyms->symtab[i].st_value < nextval)
+			nextval = kallsyms->symtab[i].st_value;
 	}
 
 	if (!best)
 		return NULL;
 
 	if (size)
-		*size = nextval - mod->symtab[best].st_value;
+		*size = nextval - kallsyms->symtab[best].st_value;
 	if (offset)
-		*offset = addr - mod->symtab[best].st_value;
-	return symname(mod, best);
+		*offset = addr - kallsyms->symtab[best].st_value;
+	return symname(kallsyms, best);
 }
 
 /* For kallsyms to ask for address resolution.  NULL means not found.  Careful
@@ -3763,18 +3782,21 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 
 	preempt_disable();
 	list_for_each_entry_rcu(mod, &modules, list) {
+		struct mod_kallsyms *kallsyms;
+
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
-		if (symnum < mod->num_symtab) {
-			*value = mod->symtab[symnum].st_value;
-			*type = mod->symtab[symnum].st_info;
-			strlcpy(name, symname(mod, symnum), KSYM_NAME_LEN);
+		kallsyms = rcu_dereference_sched(mod->kallsyms);
+		if (symnum < kallsyms->num_symtab) {
+			*value = kallsyms->symtab[symnum].st_value;
+			*type = kallsyms->symtab[symnum].st_info;
+			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
 			preempt_enable();
 			return 0;
 		}
-		symnum -= mod->num_symtab;
+		symnum -= kallsyms->num_symtab;
 	}
 	preempt_enable();
 	return -ERANGE;
@@ -3783,11 +3805,12 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 static unsigned long mod_find_symname(struct module *mod, const char *name)
 {
 	unsigned int i;
+	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
 
-	for (i = 0; i < mod->num_symtab; i++)
-		if (strcmp(name, symname(mod, i)) == 0 &&
-		    mod->symtab[i].st_info != 'U')
-			return mod->symtab[i].st_value;
+	for (i = 0; i < kallsyms->num_symtab; i++)
+		if (strcmp(name, symname(kallsyms, i)) == 0 &&
+		    kallsyms->symtab[i].st_info != 'U')
+			return kallsyms->symtab[i].st_value;
 	return 0;
 }
 
@@ -3826,11 +3849,14 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 	module_assert_mutex();
 
 	list_for_each_entry(mod, &modules, list) {
+		/* We hold module_mutex: no need for rcu_dereference_sched */
+		struct mod_kallsyms *kallsyms = mod->kallsyms;
+
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
-		for (i = 0; i < mod->num_symtab; i++) {
-			ret = fn(data, symname(mod, i),
-				 mod, mod->symtab[i].st_value);
+		for (i = 0; i < kallsyms->num_symtab; i++) {
+			ret = fn(data, symname(kallsyms, i),
+				 mod, kallsyms->symtab[i].st_value);
 			if (ret != 0)
 				return ret;
 		}

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [08/02/16 14:26 -0600]:
>On Wed, Feb 03, 2016 at 08:11:09PM -0500, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the need
>> for architecture specific relocation code in livepatch. Specifically, reuse
>> the apply_relocate_add() function in the module loader to write relocations
>> instead of duplicating functionality in livepatch's arch-dependent
>> klp_write_module_reloc() function.
>>
>> In order to accomplish this, livepatch modules manage their own relocation
>> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
>> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
>> index). To apply livepatch relocation sections, 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 and the s390
>> klp_write_module_reloc() function stub. They are no longer needed since
>> relocation work has been offloaded to module loader.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  arch/s390/include/asm/livepatch.h |   7 -
>>  arch/x86/include/asm/livepatch.h  |   2 -
>>  arch/x86/kernel/Makefile          |   1 -
>>  arch/x86/kernel/livepatch.c       |  70 ----------
>>  include/linux/livepatch.h         |  30 ++--
>>  kernel/livepatch/core.c           | 283 ++++++++++++++++++++++++++++++--------
>>  6 files changed, 238 insertions(+), 155 deletions(-)
>>  delete mode 100644 arch/x86/kernel/livepatch.c
>>
>> diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
>> index a52b6cc..350a751 100644
>> --- a/arch/s390/include/asm/livepatch.h
>> +++ b/arch/s390/include/asm/livepatch.h
>> @@ -25,13 +25,6 @@ static inline int klp_check_compiler_support(void)
>>  	return 0;
>>  }
>>
>> -static inline int klp_write_module_reloc(struct module *mod, unsigned long
>> -		type, unsigned long loc, unsigned long value)
>> -{
>> -	/* not supported yet */
>> -	return -ENOSYS;
>> -}
>> -
>>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>  {
>>  	regs->psw.addr = ip;
>> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>> index e795f52..d7c2b57 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 fdd5f1c..1a40a72 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,16 @@ struct klp_patch {
>>  	enum klp_state state;
>>  };
>>
>> +/*
>> + * Livepatch symbol and relocation section prefixes:
>> + * ".klp.rela." for relocation sections
>> + * ".klp.sym." for livepatch symbols
>> + */
>> +#define KLP_SYM_PREFIX ".klp.sym."
>> +#define KLP_SYM_PREFIX_LEN 9
>> +#define KLP_RELASEC_PREFIX ".klp.rela."
>> +#define KLP_RELASEC_PREFIX_LEN 10
>> +
>>  #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 7aa975d..c1fe57c 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>
>>
>>  /**
>> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>>  	return !obj->name || obj->mod;
>>  }
>>
>> +/*
>> + * Check if a livepatch symbol is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
>> +{
>> +	size_t len;
>> +	char *s, *objname, *symname;
>> +
>> +	if (sym->st_shndx != SHN_LIVEPATCH)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Livepatch symbol names must follow this format:
>> +	 * .klp.sym.objname.symbol_name,sympos
>> +	 */
>> +	s = pmod->strtab + sym->st_name;
>> +	/* [.klp.sym.]objname.symbol_name,sympos */
>> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.symbol_name,[sympos] */
>> +	if (!strchr(s, ','))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if a livepatch relocation section is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
>> +{
>> +	char *secname;
>> +	size_t len;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* [.klp.rela.]objname.section_name */
>> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
>> +				KLP_RELASEC_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.rela.[objname].section_name */
>> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if obj->name matches the objname encoded in the rela
>> + * section name (.klp.rela.[objname].section_name)
>> + *
>> + * Must pass klp_check_relasec_format() before calling this.
>> + */
>> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
>> +				       struct klp_object *obj)
>> +{
>> +	size_t len;
>> +	const char *obj_objname, *sec_objname, *secname;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* .klp.rela.[objname].section_name */
>> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
>> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
>> +
>> +	/* Get length of the objname encoded in the section name */
>> +	len = strcspn(sec_objname, ".");
>> +
>> +	if (strlen(obj_objname) != len)
>> +		return false;
>> +
>> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
>> +}
>> +
>> +/*
>> + * klp_get_* helper functions
>> + *
>> + * klp_get_* functions extract different components of the name
>> + * of a livepatch symbol. The full symbol name from the strtab
>> + * is passed in as parameter @s, and @result is filled in with
>> + * the extracted component.
>> + *
>> + * These functions assume a correctly formatted symbol and the
>> + * klp_check_symbol_format() test *must* pass before calling any
>> + * of these functions.
>> + */
>> +
>> +/* .klp.sym.[objname].symbol_name,sympos */
>> +static int klp_get_sym_objname(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *objname_start;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname_start = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname_start, ".");
>> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
>> +	if (objname == NULL)
>> +		return -ENOMEM;
>> +
>> +	/* klp_find_object_symbol() treats NULL as vmlinux */
>> +	if (!strcmp(objname, "vmlinux")) {
>> +		*result = NULL;
>> +		kfree(objname);
>> +	} else
>> +		*result = objname;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.[symbol_name],sympos */
>> +static int klp_get_symbol_name(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *symname;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +
>> +	*result = kstrndup(symname, len, GFP_KERNEL);
>> +	if (*result == NULL)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.symbol_name,[sympos] */
>> +static int klp_get_sympos(char *s, unsigned long *result)
>> +{
>> +	char *sympos;
>> +
>> +	/* .klp.sym.symbol_name,[sympos] */
>> +	sympos = strchr(s, ',') + 1;
>> +	return kstrtol(sympos, 10, result);
>> +}
>> +
>>  /* sets obj->mod if object is not vmlinux and module is found */
>>  static void klp_find_object_module(struct klp_object *obj)
>>  {
>
>I think all the above string parsing code could be replaced with a
>couple of sscanf() calls.
>
>For example:
>
>	char objname[64], symname[256];
>
>	ret = sscanf(s, ".klp.sym.%63[^.].%255[^,],%u", objname, symname, &sympos);
>	if (ret < 3)
>		// string doesn't match expected format
>
>That would be much simpler.
>
>Only problem is, the kernel version of sscanf() doesn't seem to support
>the '[' conversion specifier.  At least not yet ;-)  Adding support for
>that would be a win-win: less code overall, and the addition of a useful
>scanf feature which could be used by other code.

Ah, I had completely forgotten that we have sscanf() in the kernel :-)
That would look *much* nicer, assuming we can get kernel sscanf() to
support the bracket character classes..

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Josh Poimboeuf [08/02/16 14:26 -0600]:
>On Wed, Feb 03, 2016 at 08:11:09PM -0500, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the need
>> for architecture specific relocation code in livepatch. Specifically, reuse
>> the apply_relocate_add() function in the module loader to write relocations
>> instead of duplicating functionality in livepatch's arch-dependent
>> klp_write_module_reloc() function.
>>
>> In order to accomplish this, livepatch modules manage their own relocation
>> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
>> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
>> index). To apply livepatch relocation sections, 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 and the s390
>> klp_write_module_reloc() function stub. They are no longer needed since
>> relocation work has been offloaded to module loader.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/s390/include/asm/livepatch.h |   7 -
>>  arch/x86/include/asm/livepatch.h  |   2 -
>>  arch/x86/kernel/Makefile          |   1 -
>>  arch/x86/kernel/livepatch.c       |  70 ----------
>>  include/linux/livepatch.h         |  30 ++--
>>  kernel/livepatch/core.c           | 283 ++++++++++++++++++++++++++++++--------
>>  6 files changed, 238 insertions(+), 155 deletions(-)
>>  delete mode 100644 arch/x86/kernel/livepatch.c
>>
>> diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
>> index a52b6cc..350a751 100644
>> --- a/arch/s390/include/asm/livepatch.h
>> +++ b/arch/s390/include/asm/livepatch.h
>> @@ -25,13 +25,6 @@ static inline int klp_check_compiler_support(void)
>>  	return 0;
>>  }
>>
>> -static inline int klp_write_module_reloc(struct module *mod, unsigned long
>> -		type, unsigned long loc, unsigned long value)
>> -{
>> -	/* not supported yet */
>> -	return -ENOSYS;
>> -}
>> -
>>  static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>  {
>>  	regs->psw.addr = ip;
>> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
>> index e795f52..d7c2b57 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 fdd5f1c..1a40a72 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,16 @@ struct klp_patch {
>>  	enum klp_state state;
>>  };
>>
>> +/*
>> + * Livepatch symbol and relocation section prefixes:
>> + * ".klp.rela." for relocation sections
>> + * ".klp.sym." for livepatch symbols
>> + */
>> +#define KLP_SYM_PREFIX ".klp.sym."
>> +#define KLP_SYM_PREFIX_LEN 9
>> +#define KLP_RELASEC_PREFIX ".klp.rela."
>> +#define KLP_RELASEC_PREFIX_LEN 10
>> +
>>  #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 7aa975d..c1fe57c 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>
>>
>>  /**
>> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>>  	return !obj->name || obj->mod;
>>  }
>>
>> +/*
>> + * Check if a livepatch symbol is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
>> +{
>> +	size_t len;
>> +	char *s, *objname, *symname;
>> +
>> +	if (sym->st_shndx != SHN_LIVEPATCH)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Livepatch symbol names must follow this format:
>> +	 * .klp.sym.objname.symbol_name,sympos
>> +	 */
>> +	s = pmod->strtab + sym->st_name;
>> +	/* [.klp.sym.]objname.symbol_name,sympos */
>> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.symbol_name,[sympos] */
>> +	if (!strchr(s, ','))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if a livepatch relocation section is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
>> +{
>> +	char *secname;
>> +	size_t len;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* [.klp.rela.]objname.section_name */
>> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
>> +				KLP_RELASEC_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.rela.[objname].section_name */
>> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if obj->name matches the objname encoded in the rela
>> + * section name (.klp.rela.[objname].section_name)
>> + *
>> + * Must pass klp_check_relasec_format() before calling this.
>> + */
>> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
>> +				       struct klp_object *obj)
>> +{
>> +	size_t len;
>> +	const char *obj_objname, *sec_objname, *secname;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* .klp.rela.[objname].section_name */
>> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
>> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
>> +
>> +	/* Get length of the objname encoded in the section name */
>> +	len = strcspn(sec_objname, ".");
>> +
>> +	if (strlen(obj_objname) != len)
>> +		return false;
>> +
>> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
>> +}
>> +
>> +/*
>> + * klp_get_* helper functions
>> + *
>> + * klp_get_* functions extract different components of the name
>> + * of a livepatch symbol. The full symbol name from the strtab
>> + * is passed in as parameter @s, and @result is filled in with
>> + * the extracted component.
>> + *
>> + * These functions assume a correctly formatted symbol and the
>> + * klp_check_symbol_format() test *must* pass before calling any
>> + * of these functions.
>> + */
>> +
>> +/* .klp.sym.[objname].symbol_name,sympos */
>> +static int klp_get_sym_objname(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *objname_start;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname_start = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname_start, ".");
>> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
>> +	if (objname == NULL)
>> +		return -ENOMEM;
>> +
>> +	/* klp_find_object_symbol() treats NULL as vmlinux */
>> +	if (!strcmp(objname, "vmlinux")) {
>> +		*result = NULL;
>> +		kfree(objname);
>> +	} else
>> +		*result = objname;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.[symbol_name],sympos */
>> +static int klp_get_symbol_name(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *symname;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +
>> +	*result = kstrndup(symname, len, GFP_KERNEL);
>> +	if (*result == NULL)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.symbol_name,[sympos] */
>> +static int klp_get_sympos(char *s, unsigned long *result)
>> +{
>> +	char *sympos;
>> +
>> +	/* .klp.sym.symbol_name,[sympos] */
>> +	sympos = strchr(s, ',') + 1;
>> +	return kstrtol(sympos, 10, result);
>> +}
>> +
>>  /* sets obj->mod if object is not vmlinux and module is found */
>>  static void klp_find_object_module(struct klp_object *obj)
>>  {
>
>I think all the above string parsing code could be replaced with a
>couple of sscanf() calls.
>
>For example:
>
>	char objname[64], symname[256];
>
>	ret = sscanf(s, ".klp.sym.%63[^.].%255[^,],%u", objname, symname, &sympos);
>	if (ret < 3)
>		// string doesn't match expected format
>
>That would be much simpler.
>
>Only problem is, the kernel version of sscanf() doesn't seem to support
>the '[' conversion specifier.  At least not yet ;-)  Adding support for
>that would be a win-win: less code overall, and the addition of a useful
>scanf feature which could be used by other code.

Ah, I had completely forgotten that we have sscanf() in the kernel :-)
That would look *much* nicer, assuming we can get kernel sscanf() to
support the bracket character classes..

Jessica

Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Petr Mladek [09/02/16 15:01 +0100]:
>On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
>> Reuse module loader code to write relocations, thereby eliminating the need
>> for architecture specific relocation code in livepatch. Specifically, reuse
>> the apply_relocate_add() function in the module loader to write relocations
>> instead of duplicating functionality in livepatch's arch-dependent
>> klp_write_module_reloc() function.
>>
>> In order to accomplish this, livepatch modules manage their own relocation
>> sections (marked with the SHF_RELA_LIVEPATCH section flag) and
>> livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
>> index). To apply livepatch relocation sections, 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 and the s390
>> klp_write_module_reloc() function stub. They are no longer needed since
>> relocation work has been offloaded to module loader.
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 7aa975d..c1fe57c 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>
>>
>>  /**
>> @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
>>  	return !obj->name || obj->mod;
>>  }
>>
>> +/*
>> + * Check if a livepatch symbol is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
>> +{
>> +	size_t len;
>> +	char *s, *objname, *symname;
>> +
>> +	if (sym->st_shndx != SHN_LIVEPATCH)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Livepatch symbol names must follow this format:
>> +	 * .klp.sym.objname.symbol_name,sympos
>> +	 */
>> +	s = pmod->strtab + sym->st_name;
>> +	/* [.klp.sym.]objname.symbol_name,sympos */
>> +	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.symbol_name,[sympos] */
>> +	if (!strchr(s, ','))
>> +		return -EINVAL;
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if a livepatch relocation section is formatted properly.
>> + *
>> + * See Documentation/livepatch/module-elf-format.txt for a
>> + * detailed outline of requirements.
>> + */
>> +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
>> +{
>> +	char *secname;
>> +	size_t len;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* [.klp.rela.]objname.section_name */
>> +	if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
>> +				KLP_RELASEC_PREFIX_LEN))
>> +		return -EINVAL;
>> +
>> +	/* .klp.rela.[objname].section_name */
>> +	len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
>> +	if (!(len > 0))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check if obj->name matches the objname encoded in the rela
>> + * section name (.klp.rela.[objname].section_name)
>> + *
>> + * Must pass klp_check_relasec_format() before calling this.
>> + */
>> +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr *relasec,
>> +				       struct klp_object *obj)
>> +{
>> +	size_t len;
>> +	const char *obj_objname, *sec_objname, *secname;
>> +
>> +	secname = pmod->klp_info->secstrings + relasec->sh_name;
>> +	/* .klp.rela.[objname].section_name */
>> +	sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
>> +	obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
>> +
>> +	/* Get length of the objname encoded in the section name */
>> +	len = strcspn(sec_objname, ".");
>> +
>> +	if (strlen(obj_objname) != len)
>> +		return false;
>> +
>> +	return strncmp(sec_objname, obj_objname, len) ? false : true;
>> +}
>> +
>> +/*
>> + * klp_get_* helper functions
>> + *
>> + * klp_get_* functions extract different components of the name
>> + * of a livepatch symbol. The full symbol name from the strtab
>> + * is passed in as parameter @s, and @result is filled in with
>> + * the extracted component.
>> + *
>> + * These functions assume a correctly formatted symbol and the
>> + * klp_check_symbol_format() test *must* pass before calling any
>> + * of these functions.
>> + */
>> +
>> +/* .klp.sym.[objname].symbol_name,sympos */
>> +static int klp_get_sym_objname(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *objname_start;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname_start = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname_start, ".");
>> +	objname = kstrndup(objname_start, len, GFP_KERNEL);
>> +	if (objname == NULL)
>> +		return -ENOMEM;
>> +
>> +	/* klp_find_object_symbol() treats NULL as vmlinux */
>> +	if (!strcmp(objname, "vmlinux")) {
>> +		*result = NULL;
>> +		kfree(objname);
>> +	} else
>> +		*result = objname;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.[symbol_name],sympos */
>> +static int klp_get_symbol_name(char *s, char **result)
>> +{
>> +	size_t len;
>> +	char *objname, *symname;
>> +
>> +	/* .klp.sym.[objname].symbol_name,sympos */
>> +	objname = s + KLP_SYM_PREFIX_LEN;
>> +	len = strcspn(objname, ".");
>> +
>> +	/* .klp.sym.objname.[symbol_name],sympos */
>> +	symname = objname + len + 1;
>> +	len = strcspn(symname, ",");
>> +
>> +	*result = kstrndup(symname, len, GFP_KERNEL);
>> +	if (*result == NULL)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +/* .klp.sym.objname.symbol_name,[sympos] */
>> +static int klp_get_sympos(char *s, unsigned long *result)
>> +{
>> +	char *sympos;
>> +
>> +	/* .klp.sym.symbol_name,[sympos] */
>> +	sympos = strchr(s, ',') + 1;
>> +	return kstrtol(sympos, 10, result);
>> +}
>
>The usage of the helper function is nicely strightforward.
>Also I like a lot all the comments with the [] brackets
>that highlight what each check or search is for.
>
>But I think that there is a lot of duplicated code in
>the check_ and in the get_ functions. Also there is
>a lot of strdup/free games.

I agree, I do not really like the repetition and all
the strdup/free's myself.

>The length of the elemets is limited by definition.
>The functions are called under klp_mutex.
>
>Therefore it might be easier to maintain a two parse
>function that would fill static buffers and return
>error in case of wrong value.
>
>I mean something like:
>
>static char mod_name[MODULE_NAME_LEN + 1];
>static char symbol_name[KSYM_SYMBOL_LEN + 1];
>
>static int
>klp_parse_symbol_format(struct module *pmod, Elf_Sym *sym,
>			char *mod_name, char *symbol_name,
>			int *sympos)
>{
>	char *substr;
>
>
>	/*
>	 * Livepatch symbol names must follow this format:
>	 * .klp.sym.objname.symbol_name,sympos
>	 */
>	s = pmod->strtab + sym->st_name;
>	/* [.klp.sym.]objname.symbol_name,sympos */
>	if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
>		return -EINVAL;
>
>	/* .klp.sym.[objname].symbol_name,sympos */
>	substr = s + KLP_SYM_PREFIX_LEN;
>	len = strcspn(substr, ".");
>	if (!len || len > MODULE_NAME_LEN)
>		return -EINVAL;
>	strncpy(mod_name, substr, len);
>	mod_name[len] = '\0';
>
>	/* .klp.sym.objname.[symbol_name],sympos */
>	substr = substr + len + 1;
>	len = strcspn(substr, ",");
>	if (!len || len > KSYM_SYMBOL_LEN)
>		return -EINVAL;
>	strncpy(symbol_name, substr, len);
>	mod_name[len] = '\0';
>
>	/* .klp.sym.objname.symbol_name,[sympos] */
>	substr = substr + len + 1;
>	len = strlen(substr);
>	if (!len)
>		return -EINVAL;
>
>	return kstrtol(substr, 10, sympos);
>}
>
>How does that sound, please?

I like this idea, it is a lot cleaner and more concise
than using all those helper functions. :-)

Thanks,
Jessica

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

[Petr Mladek]

On Wed 2016-02-03 20:11:07, Jessica Yu wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 71c77ed..9c16eb2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2676,6 +2764,23 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	mod->klp = get_modinfo(info, "livepatch") ? true : false;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_LIVEPATCH */
> +static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
> +{
> +	if (get_modinfo(info, "livepatch"))

One more thing. I suggest to write a friendly message here. For example:

	pr_err("LifePatch support is not available. Rejecting the module: s\n",
	       mod->name);

It might safe some debugging a poor user or developer.

> +		return -ENOEXEC;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +

Best Regards,
Petr