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

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

[Jessica Yu]

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

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

[Jessica Yu]

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

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

[Petr Mladek]

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

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

[Jessica Yu]

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

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

[Petr Mladek]

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

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

[Jessica Yu]

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

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

[Petr Mladek]

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

[PATCH v3 3/3] Documentation: livepatch: add section about arch-specific code

[Jessica Yu]

Document usage of arch-specific elf sections in livepatch as well
as implementation of arch-specific code.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 Documentation/livepatch/module-elf-format.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
index eedbdcf..02bfafa 100644
--- a/Documentation/livepatch/module-elf-format.txt
+++ b/Documentation/livepatch/module-elf-format.txt
@@ -25,7 +25,8 @@ Table of Contents
        3.3.2 Required name format
        3.3.3 Example livepatch symbol names
        3.3.4 Example `readelf --symbols` output
-4. Symbol table and Elf section access
+4. Architecture-specific sections
+5. Symbol table and Elf section access
 
 ----------------------------
 0. Background and motivation
@@ -46,7 +47,7 @@ architecture.
 
 Since apply_relocate_add() requires access to a module's section header
 table, symbol table, and relocation section indices, Elf information is
-preserved for livepatch modules (see section 4). Livepatch manages its own
+preserved for livepatch modules (see section 5). Livepatch manages its own
 relocation sections and symbols, which are described in this document. The
 Elf constants used to mark livepatch symbols and relocation sections were
 selected from OS-specific ranges according to the definitions from glibc.
@@ -117,7 +118,7 @@ also possible for a livepatch module to have no livepatch relocation
 sections, as in the case of the sample livepatch module (see
 samples/livepatch).
 
-Since Elf information is preserved for livepatch modules (see Section 4), a
+Since Elf information is preserved for livepatch modules (see Section 5), a
 livepatch relocation section can be applied simply by passing in the
 appropriate section index to apply_relocate_add(), which then uses it to
 access the relocation section and apply the relocations.
@@ -292,8 +293,19 @@ Symbol table '.symtab' contains 127 entries:
 [*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
     "OS" means OS-specific.
 
+---------------------------------
+4. Architecture-specific sections
+---------------------------------
+Architectures may override arch_klp_init_object_loaded() to perform
+additional arch-specific tasks when a target module loads, such as applying
+arch-specific sections. On x86 for example, we must apply per-object
+.altinstructions and .parainstructions sections when a target module loads.
+These sections can be prefixed with ".klp.arch.$objname." so that they can
+be easily identified when iterating through a patch module's Elf sections
+(See arch/x86/kernel/livepatch.c for a complete example).
+
 --------------------------------------
-4. Symbol table and Elf section access
+5. Symbol table and Elf section access
 --------------------------------------
 A livepatch module's symbol table is accessible through module->symtab.
 
-- 
2.5.5

Re: [PATCH v3 3/3] Documentation: livepatch: add section about arch-specific code

[Petr Mladek]

On Wed 2016-08-17 20:58:30, Jessica Yu wrote:
> Document usage of arch-specific elf sections in livepatch as well
> as implementation of arch-specific code.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  Documentation/livepatch/module-elf-format.txt | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
> index eedbdcf..02bfafa 100644
> --- a/Documentation/livepatch/module-elf-format.txt
> +++ b/Documentation/livepatch/module-elf-format.txt
> @@ -25,7 +25,8 @@ Table of Contents
>         3.3.2 Required name format
>         3.3.3 Example livepatch symbol names
>         3.3.4 Example `readelf --symbols` output
> -4. Symbol table and Elf section access
> +4. Architecture-specific sections
> +5. Symbol table and Elf section access
>  
>  ----------------------------
>  0. Background and motivation
> @@ -46,7 +47,7 @@ architecture.
>  
>  Since apply_relocate_add() requires access to a module's section header
>  table, symbol table, and relocation section indices, Elf information is
> -preserved for livepatch modules (see section 4). Livepatch manages its own
> +preserved for livepatch modules (see section 5). Livepatch manages its own
>  relocation sections and symbols, which are described in this document. The
>  Elf constants used to mark livepatch symbols and relocation sections were
>  selected from OS-specific ranges according to the definitions from glibc.
> @@ -117,7 +118,7 @@ also possible for a livepatch module to have no livepatch relocation
>  sections, as in the case of the sample livepatch module (see
>  samples/livepatch).
>  
> -Since Elf information is preserved for livepatch modules (see Section 4), a
> +Since Elf information is preserved for livepatch modules (see Section 5), a
>  livepatch relocation section can be applied simply by passing in the
>  appropriate section index to apply_relocate_add(), which then uses it to
>  access the relocation section and apply the relocations.
> @@ -292,8 +293,19 @@ Symbol table '.symtab' contains 127 entries:
>  [*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
>      "OS" means OS-specific.
>  
> +---------------------------------
> +4. Architecture-specific sections
> +---------------------------------
> +Architectures may override arch_klp_init_object_loaded() to perform
> +additional arch-specific tasks when a target module loads, such as applying
> +arch-specific sections. On x86 for example, we must apply per-object
> +.altinstructions and .parainstructions sections when a target module loads.
> +These sections can be prefixed with ".klp.arch.$objname." so that they can
                  ^^^

I would personally use "must" instead of "can". Or replace "can be prefixed"
with "are prefixed".

Otherwise, it looks fine.

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

Best Regards,
Petr

Re: Documentation: livepatch: add section about arch-specific code

[Jessica Yu]

+++ Petr Mladek [18/08/16 11:57 +0200]:
>On Wed 2016-08-17 20:58:30, Jessica Yu wrote:
>> Document usage of arch-specific elf sections in livepatch as well
>> as implementation of arch-specific code.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  Documentation/livepatch/module-elf-format.txt | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/livepatch/module-elf-format.txt b/Documentation/livepatch/module-elf-format.txt
>> index eedbdcf..02bfafa 100644
>> --- a/Documentation/livepatch/module-elf-format.txt
>> +++ b/Documentation/livepatch/module-elf-format.txt
>> @@ -25,7 +25,8 @@ Table of Contents
>>         3.3.2 Required name format
>>         3.3.3 Example livepatch symbol names
>>         3.3.4 Example `readelf --symbols` output
>> -4. Symbol table and Elf section access
>> +4. Architecture-specific sections
>> +5. Symbol table and Elf section access
>>
>>  ----------------------------
>>  0. Background and motivation
>> @@ -46,7 +47,7 @@ architecture.
>>
>>  Since apply_relocate_add() requires access to a module's section header
>>  table, symbol table, and relocation section indices, Elf information is
>> -preserved for livepatch modules (see section 4). Livepatch manages its own
>> +preserved for livepatch modules (see section 5). Livepatch manages its own
>>  relocation sections and symbols, which are described in this document. The
>>  Elf constants used to mark livepatch symbols and relocation sections were
>>  selected from OS-specific ranges according to the definitions from glibc.
>> @@ -117,7 +118,7 @@ also possible for a livepatch module to have no livepatch relocation
>>  sections, as in the case of the sample livepatch module (see
>>  samples/livepatch).
>>
>> -Since Elf information is preserved for livepatch modules (see Section 4), a
>> +Since Elf information is preserved for livepatch modules (see Section 5), a
>>  livepatch relocation section can be applied simply by passing in the
>>  appropriate section index to apply_relocate_add(), which then uses it to
>>  access the relocation section and apply the relocations.
>> @@ -292,8 +293,19 @@ Symbol table '.symtab' contains 127 entries:
>>  [*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
>>      "OS" means OS-specific.
>>
>> +---------------------------------
>> +4. Architecture-specific sections
>> +---------------------------------
>> +Architectures may override arch_klp_init_object_loaded() to perform
>> +additional arch-specific tasks when a target module loads, such as applying
>> +arch-specific sections. On x86 for example, we must apply per-object
>> +.altinstructions and .parainstructions sections when a target module loads.
>> +These sections can be prefixed with ".klp.arch.$objname." so that they can
>                  ^^^
>
>I would personally use "must" instead of "can". Or replace "can be prefixed"
>with "are prefixed".

Agreed, poor word choice there. Let's just swap "can" with "must."

Jessica

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

[Miroslav Benes]

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

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

[Jiri Kosina]

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