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

Groups > linux.kernel > #1453677 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt  patches after relocations Miroslav Benes <mbenes@suse.cz> - 2016-08-02 11:10 +0200
    Re: [PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt  patches after relocations Petr Mladek <pmladek@suse.com> - 2016-08-02 15:50 +0200
    Re: [PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt  patches after relocations Jiri Kosina <jikos@kernel.org> - 2016-08-02 21:40 +0200

#1453677 — Re: [PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt patches after relocations

On Thu, 21 Jul 2016, 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 in
> a patch module are prefixed with ".klp.arch.${objname}" and applied in
> arch_klp_init_object_loaded().

Would it make sense to update our documentation and include this info?

> 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 0503f5b..4f656fe 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -83,6 +83,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..78209f1
> --- /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>

It seems I don't have such header file here and it causes a build error. 
Shouldn't it be <asm/alternative.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);
> +	}
> +}

Otherwise it looks good.

Regards,
Miroslav

[toc] | [next] | [standalone]

#1454430

On Tue 2016-08-02 10:59:07, Miroslav Benes wrote:
> On Thu, 21 Jul 2016, 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.
> > 
> > diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> > new file mode 100644
> > index 0000000..78209f1
> > --- /dev/null
> > +++ b/arch/x86/kernel/livepatch.c
> > +#include <linux/module.h>
> > +#include <linux/kallsyms.h>
> > +#include <linux/livepatch.h>
> > +#include <asm/text-patching.h>
> 
> It seems I don't have such header file here and it causes a build error. 
> Shouldn't it be <asm/alternative.h>?

It is in linux-next, see 35de5b0692aaa ("x86/asm: Stop depending on
ptrace.h in alternative.h").

I agree with the other Mirek's comments. Otherwise, it looks good.

Thanks for working on it,
Petr

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

#1455427

On Tue, 2 Aug 2016, Miroslav Benes wrote:

> > +#include <linux/module.h>
> > +#include <linux/kallsyms.h>
> > +#include <linux/livepatch.h>
> > +#include <asm/text-patching.h>
> 
> It seems I don't have such header file here and it causes a build error. 
> Shouldn't it be <asm/alternative.h>?

I've now refreshed our for-next branch so that it's based on v4.7 to allow 
for it being used as a up-to-date basis for development.

(there have been no KLP patches this merge window, which made for-next 
also quite silent).

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web