Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1326330 > unrolled thread
| Started by | Jessica Yu <jeyu@redhat.com> |
|---|---|
| First post | 2016-02-04 02:20 +0100 |
| Last post | 2016-02-09 17:00 +0100 |
| Articles | 13 — 4 participants |
Back to article view | Back to linux.kernel
[RFC PATCH v4 0/6] (mostly) Arch-independent livepatch Jessica Yu <jeyu@redhat.com> - 2016-02-04 02:20 +0100
[RFC PATCH v4 5/6] samples: livepatch: mark as livepatch module Jessica Yu <jeyu@redhat.com> - 2016-02-04 02:20 +0100
[RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2016-02-04 02:20 +0100
Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2016-02-08 16:10 +0100
Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2016-02-09 14:40 +0100
Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-08 21:30 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2016-02-10 02:00 +0100
Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Petr Mladek <pmladek@suse.com> - 2016-02-09 15:10 +0100
Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2016-02-09 17:00 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2016-02-10 02:30 +0100
Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch Miroslav Benes <mbenes@suse.cz> - 2016-02-08 16:00 +0100
Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-08 21:30 +0100
Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch Petr Mladek <pmladek@suse.com> - 2016-02-09 17:00 +0100
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2016-02-04 02:20 +0100 |
| Subject | [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch |
| Message-ID | <qYgKt-1Kh-7@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2016-02-04 02:20 +0100 |
| Subject | [RFC PATCH v4 5/6] samples: livepatch: mark as livepatch module |
| Message-ID | <qYgKu-1Kh-23@gated-at.bofh.it> |
| In reply to | #1326330 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2016-02-04 02:20 +0100 |
| Subject | [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <qYgKu-1Kh-25@gated-at.bofh.it> |
| In reply to | #1326330 |
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
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-02-08 16:10 +0100 |
| Subject | Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <qZVBT-70K-5@gated-at.bofh.it> |
| In reply to | #1326332 |
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
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-02-09 14:40 +0100 |
| Subject | Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <r0gGm-4Dt-21@gated-at.bofh.it> |
| In reply to | #1329148 |
> > +/* .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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-08 21:30 +0100 |
| Subject | Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <r00BA-1PS-7@gated-at.bofh.it> |
| In reply to | #1326332 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2016-02-10 02:00 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <r0riq-3hl-13@gated-at.bofh.it> |
| In reply to | #1329532 |
+++ 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
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-02-09 15:10 +0100 |
| Subject | Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <r0h9p-56j-33@gated-at.bofh.it> |
| In reply to | #1326332 |
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
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-02-09 17:00 +0100 |
| Subject | Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations |
| Message-ID | <r0iRQ-678-11@gated-at.bofh.it> |
| In reply to | #1330269 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jessica Yu <jeyu@redhat.com> |
|---|---|
| Date | 2016-02-10 02:30 +0100 |
| Subject | Re: livepatch: reuse module loader code to write relocations |
| Message-ID | <r0rLs-3Iq-19@gated-at.bofh.it> |
| In reply to | #1330269 |
+++ 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
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-02-08 16:00 +0100 |
| Message-ID | <qZVse-6HZ-11@gated-at.bofh.it> |
| In reply to | #1326330 |
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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-08 21:30 +0100 |
| Message-ID | <r00BA-1PS-13@gated-at.bofh.it> |
| In reply to | #1329140 |
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
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-02-09 17:00 +0100 |
| Message-ID | <r0iRQ-678-9@gated-at.bofh.it> |
| In reply to | #1326330 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web