Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1464865 > unrolled thread

[PATCH v3 0/3] Fix issue with alternatives/paravirt patches

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] Fix issue with alternatives/paravirt patches Jessica Yu <jeyu@redhat.com> - 2016-08-18 03:00 +0200
    [PATCH v3 2/3] livepatch/x86: apply alternatives and paravirt patches after relocations Jessica Yu <jeyu@redhat.com> - 2016-08-18 03:00 +0200
      Re: [PATCH v3 2/3] livepatch/x86: apply alternatives and paravirt  patches after relocations Petr Mladek <pmladek@suse.com> - 2016-08-18 12:00 +0200
        Re: livepatch/x86: apply alternatives and paravirt patches after  relocations Jessica Yu <jeyu@redhat.com> - 2016-08-19 03:20 +0200
          Re: livepatch/x86: apply alternatives and paravirt patches after  relocations Petr Mladek <pmladek@suse.com> - 2016-08-19 10:40 +0200
    [PATCH v3 1/3] livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks Jessica Yu <jeyu@redhat.com> - 2016-08-18 03:00 +0200
      Re: [PATCH v3 1/3] livepatch: use arch_klp_init_object_loaded() to  finish arch-specific tasks Petr Mladek <pmladek@suse.com> - 2016-08-18 12:00 +0200
    Re: [PATCH v3 0/3] Fix issue with alternatives/paravirt patches Miroslav Benes <mbenes@suse.cz> - 2016-08-18 14:50 +0200
    Re: [PATCH v3 0/3] Fix issue with alternatives/paravirt patches Jiri Kosina <jikos@kernel.org> - 2016-08-19 03:30 +0200

#1464865 — [PATCH v3 0/3] Fix issue with alternatives/paravirt patches

Hi,

A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.

One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.

To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.

Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).

I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.

Based on linux-next.

v2 here:
http://lkml.kernel.org/g/1469078640-26798-1-git-send-email-jeyu@redhat.com

v3: 
 - Add documentation about arch-specific code
 - Make sure to call module_enable_ro() when returning on error

v2:
 - add BUILD_BUG_ON() check in arch_klp_init_object_loaded (x86)

[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580

Jessica Yu (3):
  livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
  livepatch/x86: apply alternatives and paravirt patches after relocations
  Documentation: livepatch: add section about arch-specific code

 Documentation/livepatch/module-elf-format.txt | 20 +++++++--
 arch/x86/kernel/Makefile                      |  1 +
 arch/x86/kernel/livepatch.c                   | 65 +++++++++++++++++++++++++++
 include/linux/livepatch.h                     |  3 ++
 kernel/livepatch/core.c                       | 16 +++++--
 5 files changed, 98 insertions(+), 7 deletions(-)
 create mode 100644 arch/x86/kernel/livepatch.c

-- 
2.5.5

[toc] | [next] | [standalone]

#1464867 — [PATCH v3 2/3] livepatch/x86: apply alternatives and paravirt patches after relocations

Implement arch_klp_init_object_loaded() for x86, which applies
alternatives/paravirt patches. This fixes the order in which relocations
and alternatives/paravirt patches are applied.

Previously, if a patch module had alternatives or paravirt patches,
these were applied first by the module loader before livepatch can apply
per-object relocations. The (buggy) sequence of events was:

(1) Load patch module
(2) Apply alternatives and paravirt patches to patch module
    * Note that these are applied to the new functions in the patch module
(3) Apply per-object relocations to patch module when target module loads.
    * This clobbers what was written in step 2

This lead to crashes and corruption in general, since livepatch would
overwrite or step on previously applied alternative/paravirt patches.
The correct sequence of events should be:

(1) Load patch module
(2) Apply per-object relocations to patch module
(3) Apply alternatives and paravirt patches to patch module

This is fixed by delaying paravirt/alternatives patching until after
relocations are applied. Any .altinstructions or .parainstructions
sections are prefixed with ".klp.arch.${objname}" and applied in
arch_klp_init_object_loaded().

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 arch/x86/kernel/Makefile    |  1 +
 arch/x86/kernel/livepatch.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
 create mode 100644 arch/x86/kernel/livepatch.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index d3f49c3..92fd50c 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -81,6 +81,7 @@ 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
new file mode 100644
index 0000000..e9d252d
--- /dev/null
+++ b/arch/x86/kernel/livepatch.c
@@ -0,0 +1,65 @@
+/*
+ * livepatch.c - x86-specific Kernel Live Patching Core
+ *
+ * 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/kallsyms.h>
+#include <linux/livepatch.h>
+#include <asm/text-patching.h>
+
+/* Apply per-object alternatives. Based on x86 module_finalize() */
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+				 struct klp_object *obj)
+{
+	int cnt;
+	struct klp_modinfo *info;
+	Elf_Shdr *s, *alt = NULL, *para = NULL;
+	void *aseg, *pseg;
+	const char *objname;
+	char sec_objname[MODULE_NAME_LEN];
+	char secname[KSYM_NAME_LEN];
+
+	info = patch->mod->klp_info;
+	objname = obj->name ? obj->name : "vmlinux";
+
+	/* See livepatch core code for BUILD_BUG_ON() explanation */
+	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
+
+	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
+		/* Apply per-object .klp.arch sections */
+		cnt = sscanf(info->secstrings + s->sh_name,
+			     ".klp.arch.%55[^.].%127s",
+			     sec_objname, secname);
+		if (cnt != 2)
+			continue;
+		if (strcmp(sec_objname, objname))
+			continue;
+		if (!strcmp(".altinstructions", secname))
+			alt = s;
+		if (!strcmp(".parainstructions", secname))
+			para = s;
+	}
+
+	if (alt) {
+		aseg = (void *) alt->sh_addr;
+		apply_alternatives(aseg, aseg + alt->sh_size);
+	}
+
+	if (para) {
+		pseg = (void *) para->sh_addr;
+		apply_paravirt(pseg, pseg + para->sh_size);
+	}
+}
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1465064 — Re: [PATCH v3 2/3] livepatch/x86: apply alternatives and paravirt patches after relocations

On Wed 2016-08-17 20:58:29, Jessica Yu wrote:
> Implement arch_klp_init_object_loaded() for x86, which applies
> alternatives/paravirt patches. This fixes the order in which relocations
> and alternatives/paravirt patches are applied.
> 
> Previously, if a patch module had alternatives or paravirt patches,
> these were applied first by the module loader before livepatch can apply
> per-object relocations. The (buggy) sequence of events was:
> 
> (1) Load patch module
> (2) Apply alternatives and paravirt patches to patch module
>     * Note that these are applied to the new functions in the patch module
> (3) Apply per-object relocations to patch module when target module loads.
>     * This clobbers what was written in step 2
> 
> This lead to crashes and corruption in general, since livepatch would
> overwrite or step on previously applied alternative/paravirt patches.
> The correct sequence of events should be:
> 
> (1) Load patch module
> (2) Apply per-object relocations to patch module
> (3) Apply alternatives and paravirt patches to patch module
> 
> This is fixed by delaying paravirt/alternatives patching until after
> relocations are applied. Any .altinstructions or .parainstructions
> sections are prefixed with ".klp.arch.${objname}" and applied in
> arch_klp_init_object_loaded().
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  arch/x86/kernel/Makefile    |  1 +
>  arch/x86/kernel/livepatch.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
>  create mode 100644 arch/x86/kernel/livepatch.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index d3f49c3..92fd50c 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -81,6 +81,7 @@ 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
> new file mode 100644
> index 0000000..e9d252d
> --- /dev/null
> +++ b/arch/x86/kernel/livepatch.c
> @@ -0,0 +1,65 @@
> +/*
> + * livepatch.c - x86-specific Kernel Live Patching Core
> + *
> + * 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/kallsyms.h>
> +#include <linux/livepatch.h>
> +#include <asm/text-patching.h>
> +
> +/* Apply per-object alternatives. Based on x86 module_finalize() */
> +void arch_klp_init_object_loaded(struct klp_patch *patch,
> +				 struct klp_object *obj)
> +{
> +	int cnt;
> +	struct klp_modinfo *info;
> +	Elf_Shdr *s, *alt = NULL, *para = NULL;
> +	void *aseg, *pseg;
> +	const char *objname;
> +	char sec_objname[MODULE_NAME_LEN];
> +	char secname[KSYM_NAME_LEN];
> +
> +	info = patch->mod->klp_info;
> +	objname = obj->name ? obj->name : "vmlinux";
> +
> +	/* See livepatch core code for BUILD_BUG_ON() explanation */
> +	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
> +
> +	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
> +		/* Apply per-object .klp.arch sections */
> +		cnt = sscanf(info->secstrings + s->sh_name,
> +			     ".klp.arch.%55[^.].%127s",
> +			     sec_objname, secname);
> +		if (cnt != 2)
> +			continue;
> +		if (strcmp(sec_objname, objname))
> +			continue;
> +		if (!strcmp(".altinstructions", secname))

The previous version of the patch compared against "altinstructions"
(without the dot). I admit that I haven't tested it but the dot
looks suspicious here.

> +			alt = s;
> +		if (!strcmp(".parainstructions", secname))

Same here.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1465746 — Re: livepatch/x86: apply alternatives and paravirt patches after relocations

+++ Petr Mladek [18/08/16 11:51 +0200]:
>On Wed 2016-08-17 20:58:29, Jessica Yu wrote:
>> Implement arch_klp_init_object_loaded() for x86, which applies
>> alternatives/paravirt patches. This fixes the order in which relocations
>> and alternatives/paravirt patches are applied.
>>
>> Previously, if a patch module had alternatives or paravirt patches,
>> these were applied first by the module loader before livepatch can apply
>> per-object relocations. The (buggy) sequence of events was:
>>
>> (1) Load patch module
>> (2) Apply alternatives and paravirt patches to patch module
>>     * Note that these are applied to the new functions in the patch module
>> (3) Apply per-object relocations to patch module when target module loads.
>>     * This clobbers what was written in step 2
>>
>> This lead to crashes and corruption in general, since livepatch would
>> overwrite or step on previously applied alternative/paravirt patches.
>> The correct sequence of events should be:
>>
>> (1) Load patch module
>> (2) Apply per-object relocations to patch module
>> (3) Apply alternatives and paravirt patches to patch module
>>
>> This is fixed by delaying paravirt/alternatives patching until after
>> relocations are applied. Any .altinstructions or .parainstructions
>> sections are prefixed with ".klp.arch.${objname}" and applied in
>> arch_klp_init_object_loaded().
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  arch/x86/kernel/Makefile    |  1 +
>>  arch/x86/kernel/livepatch.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+)
>>  create mode 100644 arch/x86/kernel/livepatch.c
>>
>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>> index d3f49c3..92fd50c 100644
>> --- a/arch/x86/kernel/Makefile
>> +++ b/arch/x86/kernel/Makefile
>> @@ -81,6 +81,7 @@ 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
>> new file mode 100644
>> index 0000000..e9d252d
>> --- /dev/null
>> +++ b/arch/x86/kernel/livepatch.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * livepatch.c - x86-specific Kernel Live Patching Core
>> + *
>> + * 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/kallsyms.h>
>> +#include <linux/livepatch.h>
>> +#include <asm/text-patching.h>
>> +
>> +/* Apply per-object alternatives. Based on x86 module_finalize() */
>> +void arch_klp_init_object_loaded(struct klp_patch *patch,
>> +				 struct klp_object *obj)
>> +{
>> +	int cnt;
>> +	struct klp_modinfo *info;
>> +	Elf_Shdr *s, *alt = NULL, *para = NULL;
>> +	void *aseg, *pseg;
>> +	const char *objname;
>> +	char sec_objname[MODULE_NAME_LEN];
>> +	char secname[KSYM_NAME_LEN];
>> +
>> +	info = patch->mod->klp_info;
>> +	objname = obj->name ? obj->name : "vmlinux";
>> +
>> +	/* See livepatch core code for BUILD_BUG_ON() explanation */
>> +	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
>> +
>> +	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
>> +		/* Apply per-object .klp.arch sections */
>> +		cnt = sscanf(info->secstrings + s->sh_name,
>> +			     ".klp.arch.%55[^.].%127s",
>> +			     sec_objname, secname);
>> +		if (cnt != 2)
>> +			continue;
>> +		if (strcmp(sec_objname, objname))
>> +			continue;
>> +		if (!strcmp(".altinstructions", secname))
>
>The previous version of the patch compared against "altinstructions"
>(without the dot). I admit that I haven't tested it but the dot
>looks suspicious here.

Good eye, I should have explained why the dot is needed in the strcmp..
So, the new documentation states that any arch-specific sections to
be applied by livepatch are to be prefixed with the string
".klp.arch.$objname.", note the required dot at the end of this prefix.

So for example, if we have a .parainstructions section with a patch
for the kvm module, the prefixed section name would look like:

   .klp.arch.kvm..parainstructions
   ^   prefix   ^^ original name ^

That extra dot looks weird, but it is needed when we have section names
like "__ftr_fixup" on powerpc. Without the extra dot at the end of
".klp.arch.$objname." We'd get names like ".klp.arch.$objname__ftr_fixup",
and we wouldn't be able to tell where the objname ends and where the
section name begins. But with ".klp.arch.$objname.__ftr_fixup", we
have a hard delimeter and know that after the dot after $objname comes
the original section name.

Hope that helps!

Jessica

[toc] | [prev] | [next] | [standalone]

#1466226 — Re: livepatch/x86: apply alternatives and paravirt patches after relocations

On Thu 2016-08-18 14:03:13, Jessica Yu wrote:
> +++ Petr Mladek [18/08/16 11:51 +0200]:
> >On Wed 2016-08-17 20:58:29, Jessica Yu wrote:
> >>Implement arch_klp_init_object_loaded() for x86, which applies
> >>alternatives/paravirt patches. This fixes the order in which relocations
> >>and alternatives/paravirt patches are applied.
> >>
> >>--- /dev/null
> >>+++ b/arch/x86/kernel/livepatch.c
> >>+	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
> >>+		/* Apply per-object .klp.arch sections */
> >>+		cnt = sscanf(info->secstrings + s->sh_name,
> >>+			     ".klp.arch.%55[^.].%127s",
> >>+			     sec_objname, secname);
> >>+		if (cnt != 2)
> >>+			continue;
> >>+		if (strcmp(sec_objname, objname))
> >>+			continue;
> >>+		if (!strcmp(".altinstructions", secname))
> >
> >The previous version of the patch compared against "altinstructions"
> >(without the dot). I admit that I haven't tested it but the dot
> >looks suspicious here.
> 
> Good eye, I should have explained why the dot is needed in the strcmp..
> So, the new documentation states that any arch-specific sections to
> be applied by livepatch are to be prefixed with the string
> ".klp.arch.$objname.", note the required dot at the end of this prefix.
> 
> So for example, if we have a .parainstructions section with a patch
> for the kvm module, the prefixed section name would look like:
> 
>   .klp.arch.kvm..parainstructions
>   ^   prefix   ^^ original name ^
> 
> That extra dot looks weird, but it is needed when we have section names
> like "__ftr_fixup" on powerpc. Without the extra dot at the end of
> ".klp.arch.$objname." We'd get names like ".klp.arch.$objname__ftr_fixup",
> and we wouldn't be able to tell where the objname ends and where the
> section name begins. But with ".klp.arch.$objname.__ftr_fixup", we
> have a hard delimeter and know that after the dot after $objname comes
> the original section name.

That is a bit unfortunate but it makes perfect sense.
Thanks a lot for explanation.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1464872 — [PATCH v3 1/3] livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks

Introduce arch_klp_init_object_loaded() to complete any additional
arch-specific tasks during patching. Architecture code may override this
function.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/livepatch.h |  3 +++
 kernel/livepatch/core.c   | 16 +++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a93a0b2..9072f04 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -116,6 +116,9 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+				 struct klp_object *obj);
+
 /* Called from the module loader during module coming/going states */
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 8bbe507..5fbabe0 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -274,7 +274,6 @@ static int klp_write_object_relocations(struct module *pmod,
 
 	objname = klp_is_module(obj) ? obj->name : "vmlinux";
 
-	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;
@@ -309,7 +308,6 @@ static int klp_write_object_relocations(struct module *pmod,
 			break;
 	}
 
-	module_enable_ro(pmod, true);
 	return ret;
 }
 
@@ -763,6 +761,12 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 				    func->old_sympos ? func->old_sympos : 1);
 }
 
+/* Arches may override this to finish any remaining arch-specific tasks */
+void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
+					struct klp_object *obj)
+{
+}
+
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
@@ -770,9 +774,15 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	struct klp_func *func;
 	int ret;
 
+	module_disable_ro(patch->mod);
 	ret = klp_write_object_relocations(patch->mod, obj);
-	if (ret)
+	if (ret) {
+		module_enable_ro(patch->mod, true);
 		return ret;
+	}
+
+	arch_klp_init_object_loaded(patch, obj);
+	module_enable_ro(patch->mod, true);
 
 	klp_for_each_func(obj, func) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1465071 — Re: [PATCH v3 1/3] livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks

On Wed 2016-08-17 20:58:28, Jessica Yu wrote:
> Introduce arch_klp_init_object_loaded() to complete any additional
> arch-specific tasks during patching. Architecture code may override this
> function.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

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

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1465228

On Wed, 17 Aug 2016, Jessica Yu wrote:

> Hi,
> 
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
> 
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
> 
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
> 
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
> 
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
> 
> Based on linux-next.

For the whole patch set

Acked-by: Miroslav Benes <mbenes@suse.cz>

Thanks
Miroslav

[toc] | [prev] | [next] | [standalone]

#1465780

I've applied the whole series (with the small Documentation tweak 
suggested by Petr) to livepatching.git#for-4.9/klp-paravirt-alternatives

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web