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


Groups > linux.kernel > #1366195 > unrolled thread

Bug with paravirt ops and livepatches

Started byChris J Arges <chris.j.arges@canonical.com>
First post2016-03-29 14:10 +0200
Last post2016-03-29 15:10 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel


Contents

  Bug with paravirt ops and livepatches Chris J Arges <chris.j.arges@canonical.com> - 2016-03-29 14:10 +0200
    Re: Bug with paravirt ops and livepatches Jiri Kosina <jikos@kernel.org> - 2016-03-29 15:10 +0200
      Re: Bug with paravirt ops and livepatches Jiri Kosina <jikos@kernel.org> - 2016-04-01 17:10 +0200
        Re: Bug with paravirt ops and livepatches Miroslav Benes <mbenes@suse.cz> - 2016-04-01 17:50 +0200
          Re: Bug with paravirt ops and livepatches Chris J Arges <chris.j.arges@canonical.com> - 2016-04-01 18:10 +0200
          Re: Bug with paravirt ops and livepatches Chris J Arges <chris.j.arges@canonical.com> - 2016-04-01 21:10 +0200
            Re: Bug with paravirt ops and livepatches Jiri Kosina <jikos@kernel.org> - 2016-04-01 21:40 +0200
    Re: Bug with paravirt ops and livepatches Miroslav Benes <mbenes@suse.cz> - 2016-03-29 15:10 +0200

#1366195 — Bug with paravirt ops and livepatches

FromChris J Arges <chris.j.arges@canonical.com>
Date2016-03-29 14:10 +0200
SubjectBug with paravirt ops and livepatches
Message-ID<ri0D7-61I-1@gated-at.bofh.it>
Paravirtualized ops and livepatching currently don't mix very well and can
cause undefined behavor such as oops, invalid opcodes or corrupted stacks.
The original discussion of this issue can be found here [1].

I've written an example livepatch module that reproduces the issue [2].
In order to trigger the issue you must first insert the module then trigger
the paravirt ops by starting a VM.

In the thread here [1] a couple of solutions have been proposed:

1) Jessica proposed using the Arch-independent patchset ensure that livepatch
finishes writing its relas before apply_paravirt() is called. However, this
introduces a bit more arch-dependent code. It would be useful to see if other
arches are affected by this as well.

2) Eugene proposed skipping application of the rela if the instruction to be
relocated has already been changed. This passes the initial example [2];
however its unclear if/how this will break things.

It may be good to weigh in here and get more eyes on this.
Thanks,
--chris

[1]: https://github.com/dynup/kpatch/issues/580
[2]: http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl/livepatch.c

[toc] | [next] | [standalone]


#1366255

FromJiri Kosina <jikos@kernel.org>
Date2016-03-29 15:10 +0200
Message-ID<ri1zd-6Hc-25@gated-at.bofh.it>
In reply to#1366195
On Tue, 29 Mar 2016, Miroslav Benes wrote:

> > 1) Jessica proposed using the Arch-independent patchset ensure that livepatch
> > finishes writing its relas before apply_paravirt() is called. However, this
> > introduces a bit more arch-dependent code. It would be useful to see if other
> > arches are affected by this as well.
> 
> I think this is the way to go. Provided we have Jessica's two patch sets 
> applied (arch-independent and notifiers removal) there are two options. We 
> either move a call to klp_coming_module() somewhere before 
> module_finalize(), or we move the problematic parts of module_finalize() 
> to the end of load_module() (on x86 it is probably module_finalize() as a 
> whole). The former is almost impossible because of the dependencies 
> (ftrace and such), the latter should be doable (with very careful check we 
> won't break anything).

Agreed; I think we should be safe applying all the alternatives (with 
paravirt being really just a special case of those) to the coming module 
at the very last phase; they really are required only during runtime, but 
nothing else should be depending on them. Right? If anyone is able to come 
up with and counter-example, please speak up :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

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


#1369405

FromJiri Kosina <jikos@kernel.org>
Date2016-04-01 17:10 +0200
Message-ID<rj8RY-6cF-15@gated-at.bofh.it>
In reply to#1366255
On Tue, 29 Mar 2016, Jiri Kosina wrote:

> Agreed; I think we should be safe applying all the alternatives (with 
> paravirt being really just a special case of those) to the coming module 
> at the very last phase; they really are required only during runtime, 
> but nothing else should be depending on them. Right? If anyone is able 
> to come up with and counter-example, please speak up :)

So I have quickly gone through all the architectures that actually do 
overload __weak module_finalize() by their own implementation, and except 
for applying self-modifying code changes and registering unwind tables, 
there doesn't seem to be any relevant heavy-lifting, that'd need to be 
done before relocations have been written.

Is the (completely untested) sort-of-a-patch below a complete rubbish 
(on top of current livepatching.git's for-next)?




diff --git a/kernel/module.c b/kernel/module.c
index 5f71aa6..c003648 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
 	return 0;
 }
 
-static int post_relocation(struct module *mod, const struct load_info *info)
+static void post_relocation(struct module *mod, const struct load_info *info)
 {
 	/* Sort exception table now relocations are done. */
 	sort_extable(mod->extable, mod->extable + mod->num_exentries);
@@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 
 	/* Setup kallsyms-specific fields. */
 	add_kallsyms(mod, info);
-
-	/* Arch-specific module finalizing. */
-	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
 /* Is this module of this name done loading?  No locks held. */
@@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto free_modinfo;
 
-	err = post_relocation(mod, info);
-	if (err < 0)
-		goto free_modinfo;
+	post_relocation(mod, info);
 
 	flush_module_icache(mod);
 
@@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto bug_cleanup;
 
+	/* Arch-specific module finalizing. */
+	err = module_finalize(info->hdr, info->sechdrs, mod);
+	if (err)
+		goto bug_cleanup;
+
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
 				  -32768, 32767, mod,

-- 
Jiri Kosina
SUSE Labs

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


#1369436

FromMiroslav Benes <mbenes@suse.cz>
Date2016-04-01 17:50 +0200
Message-ID<rj9uG-6rC-21@gated-at.bofh.it>
In reply to#1369405
On Fri, 1 Apr 2016, Jiri Kosina wrote:

> On Tue, 29 Mar 2016, Jiri Kosina wrote:
> 
> > Agreed; I think we should be safe applying all the alternatives (with 
> > paravirt being really just a special case of those) to the coming module 
> > at the very last phase; they really are required only during runtime, 
> > but nothing else should be depending on them. Right? If anyone is able 
> > to come up with and counter-example, please speak up :)
> 
> So I have quickly gone through all the architectures that actually do 
> overload __weak module_finalize() by their own implementation, and except 
> for applying self-modifying code changes and registering unwind tables, 
> there doesn't seem to be any relevant heavy-lifting, that'd need to be 
> done before relocations have been written.
> 
> Is the (completely untested) sort-of-a-patch below a complete rubbish 
> (on top of current livepatching.git's for-next)?
> 
> 
> 
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 5f71aa6..c003648 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
>  	return 0;
>  }
>  
> -static int post_relocation(struct module *mod, const struct load_info *info)
> +static void post_relocation(struct module *mod, const struct load_info *info)
>  {
>  	/* Sort exception table now relocations are done. */
>  	sort_extable(mod->extable, mod->extable + mod->num_exentries);
> @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
>  
>  	/* Setup kallsyms-specific fields. */
>  	add_kallsyms(mod, info);
> -
> -	/* Arch-specific module finalizing. */
> -	return module_finalize(info->hdr, info->sechdrs, mod);
>  }
>  
>  /* Is this module of this name done loading?  No locks held. */
> @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto free_modinfo;
>  
> -	err = post_relocation(mod, info);
> -	if (err < 0)
> -		goto free_modinfo;
> +	post_relocation(mod, info);
>  
>  	flush_module_icache(mod);
>  
> @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err)
>  		goto bug_cleanup;
>  
> +	/* Arch-specific module finalizing. */
> +	err = module_finalize(info->hdr, info->sechdrs, mod);
> +	if (err)
> +		goto bug_cleanup;

goto coming_cleanup;

Otherwise it looks ok. I'll give it a proper look on Monday though.

Miroslav

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


#1369459

FromChris J Arges <chris.j.arges@canonical.com>
Date2016-04-01 18:10 +0200
Message-ID<rj9O2-6S2-15@gated-at.bofh.it>
In reply to#1369436
On Fri, Apr 01, 2016 at 05:46:52PM +0200, Miroslav Benes wrote:
> On Fri, 1 Apr 2016, Jiri Kosina wrote:
> 
> > On Tue, 29 Mar 2016, Jiri Kosina wrote:
> > 
> > > Agreed; I think we should be safe applying all the alternatives (with 
> > > paravirt being really just a special case of those) to the coming module 
> > > at the very last phase; they really are required only during runtime, 
> > > but nothing else should be depending on them. Right? If anyone is able 
> > > to come up with and counter-example, please speak up :)
> > 
> > So I have quickly gone through all the architectures that actually do 
> > overload __weak module_finalize() by their own implementation, and except 
> > for applying self-modifying code changes and registering unwind tables, 
> > there doesn't seem to be any relevant heavy-lifting, that'd need to be 
> > done before relocations have been written.
> > 
> > Is the (completely untested) sort-of-a-patch below a complete rubbish 
> > (on top of current livepatching.git's for-next)?
> > 
> > 
> > 
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 5f71aa6..c003648 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
> >  	return 0;
> >  }
> >  
> > -static int post_relocation(struct module *mod, const struct load_info *info)
> > +static void post_relocation(struct module *mod, const struct load_info *info)
> >  {
> >  	/* Sort exception table now relocations are done. */
> >  	sort_extable(mod->extable, mod->extable + mod->num_exentries);
> > @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
> >  
> >  	/* Setup kallsyms-specific fields. */
> >  	add_kallsyms(mod, info);
> > -
> > -	/* Arch-specific module finalizing. */
> > -	return module_finalize(info->hdr, info->sechdrs, mod);
> >  }
> >  
> >  /* Is this module of this name done loading?  No locks held. */
> > @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err < 0)
> >  		goto free_modinfo;
> >  
> > -	err = post_relocation(mod, info);
> > -	if (err < 0)
> > -		goto free_modinfo;
> > +	post_relocation(mod, info);
> >  
> >  	flush_module_icache(mod);
> >  
> > @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err)
> >  		goto bug_cleanup;
> >  
> > +	/* Arch-specific module finalizing. */
> > +	err = module_finalize(info->hdr, info->sechdrs, mod);
> > +	if (err)
> > +		goto bug_cleanup;
> 
> goto coming_cleanup;
> 
> Otherwise it looks ok. I'll give it a proper look on Monday though.
> 
> Miroslav

I'll test this out and see if it fixes the original issue.
--chris

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


#1369540

FromChris J Arges <chris.j.arges@canonical.com>
Date2016-04-01 21:10 +0200
Message-ID<rjcCd-pj-13@gated-at.bofh.it>
In reply to#1369436
On Fri, Apr 01, 2016 at 05:46:52PM +0200, Miroslav Benes wrote:
> On Fri, 1 Apr 2016, Jiri Kosina wrote:
> 
> > On Tue, 29 Mar 2016, Jiri Kosina wrote:
> > 
> > > Agreed; I think we should be safe applying all the alternatives (with 
> > > paravirt being really just a special case of those) to the coming module 
> > > at the very last phase; they really are required only during runtime, 
> > > but nothing else should be depending on them. Right? If anyone is able 
> > > to come up with and counter-example, please speak up :)
> > 
> > So I have quickly gone through all the architectures that actually do 
> > overload __weak module_finalize() by their own implementation, and except 
> > for applying self-modifying code changes and registering unwind tables, 
> > there doesn't seem to be any relevant heavy-lifting, that'd need to be 
> > done before relocations have been written.
> > 
> > Is the (completely untested) sort-of-a-patch below a complete rubbish 
> > (on top of current livepatching.git's for-next)?
> > 
> > 
> > 
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 5f71aa6..c003648 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
> >  	return 0;
> >  }
> >  
> > -static int post_relocation(struct module *mod, const struct load_info *info)
> > +static void post_relocation(struct module *mod, const struct load_info *info)
> >  {
> >  	/* Sort exception table now relocations are done. */
> >  	sort_extable(mod->extable, mod->extable + mod->num_exentries);
> > @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
> >  
> >  	/* Setup kallsyms-specific fields. */
> >  	add_kallsyms(mod, info);
> > -
> > -	/* Arch-specific module finalizing. */
> > -	return module_finalize(info->hdr, info->sechdrs, mod);
> >  }
> >  
> >  /* Is this module of this name done loading?  No locks held. */
> > @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err < 0)
> >  		goto free_modinfo;
> >  
> > -	err = post_relocation(mod, info);
> > -	if (err < 0)
> > -		goto free_modinfo;
> > +	post_relocation(mod, info);
> >  
> >  	flush_module_icache(mod);
> >  
> > @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err)
> >  		goto bug_cleanup;
> >  
> > +	/* Arch-specific module finalizing. */
> > +	err = module_finalize(info->hdr, info->sechdrs, mod);
> > +	if (err)
> > +		goto bug_cleanup;
> 
> goto coming_cleanup;
> 
> Otherwise it looks ok. I'll give it a proper look on Monday though.
> 
> Miroslav
>

Loading, please wait...
starting version 229
[    1.182869] random: udevadm urandom read with 2 bits of entropy available
[    1.241404] BUG: unable to handle kernel paging request at ffffffffc000f35f
[    1.242760] IP: [<ffffffff813f3107>] __memcpy+0x17/0x20
[    1.243870] PGD 1e09067 PUD 1e0b067 PMD 1edb0067 PTE 1ee61161
[    1.245172] Oops: 0003 [#1] SMP 
[    1.245975] Modules linked in: floppy(+) pata_acpi
[    1.247086] CPU: 0 PID: 135 Comm: systemd-udevd Not tainted 4.5.0+ #3
[    1.248176] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[    1.249765] task: ffff88001f360000 ti: ffff88001ed40000 task.ti: ffff88001ed40000
[    1.251097] RIP: 0010:[<ffffffff813f3107>]  [<ffffffff813f3107>] __memcpy+0x17/0x20
[    1.252534] RSP: 0018:ffff88001ed43b78  EFLAGS: 00010002
[    1.253441] RAX: ffffffffc000f35f RBX: ffffffffc0011fa8 RCX: 0000000000000007
[    1.254584] RDX: 0000000000000007 RSI: ffff88001ed43ba2 RDI: ffffffffc000f35f
[    1.255736] RBP: ffff88001ed43b90 R08: ffffffff81a046c6 R09: ffffffff81063f77
[    1.256899] R10: ffffffff81f33f00 R11: ffffffff81f33ee0 R12: 0000000000000246
[    1.258042] R13: ffffffffc0011fb4 R14: ffffffff81c70005 R15: ffffffff81c6ffff
[    1.259195] FS:  00007fc8b518e8c0(0000) GS:ffff88001fc00000(0000) knlGS:0000000000000000
[    1.260564] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.261488] CR2: ffffffffc000f35f CR3: 000000001ed2d000 CR4: 00000000000006f0
[    1.262587] Stack:
[    1.263039]  ffffffff810350f2 ffffffffc0011fa8 ffff88001ed43ba2 ffff88001ed43cc0
[    1.264565]  ffffffff8103584d 401f0f0007c63ec0 ffffffffff57a000 0000000000000000
[    1.266084]  0000000000000001 ffff88001ed43c9f 0000160000000000 ffff88001ed43bf0
[    1.267630] Call Trace:
[    1.268139]  [<ffffffff810350f2>] ? text_poke_early+0x22/0x40
[    1.269065]  [<ffffffff8103584d>] apply_paravirt.part.2+0xad/0x140
[    1.270046]  [<ffffffff8106862c>] ? set_pte_vaddr_pud+0x3c/0x50
[    1.270995]  [<ffffffff810686a3>] ? set_pte_vaddr+0x63/0xa0
[    1.271909]  [<ffffffffc000fa1f>] ? redo_fd_request+0x122f/0x13ea [floppy]
[    1.272933]  [<ffffffff810701c8>] ? __native_set_fixmap+0x28/0x40
[    1.273860]  [<ffffffff8107021a>] ? native_set_fixmap+0x3a/0x40
[    1.274765]  [<ffffffffc0008000>] ? 0xffffffffc0008000
[    1.285259]  [<ffffffffc000fbda>] ? redo_fd_request+0x13ea/0x13ea [floppy]
[    1.286251]  [<ffffffff81035b79>] ? alternatives_smp_module_add+0x59/0x190
[    1.287244]  [<ffffffff810358f9>] apply_paravirt+0x19/0x20
[    1.288072]  [<ffffffff8105fbf4>] module_finalize+0xf4/0x150
[    1.288923]  [<ffffffff8110b797>] load_module+0x1f87/0x2b00
[    1.289760]  [<ffffffff812168c8>] ? __vfs_read+0xc8/0x110
[    1.290577]  [<ffffffff81390f9d>] ? ima_post_read_file+0x7d/0xa0
[    1.291464]  [<ffffffff8110c586>] SYSC_finit_module+0xe6/0x120
[    1.292326]  [<ffffffff8110c5de>] SyS_finit_module+0xe/0x10
[    1.293162]  [<ffffffff818206b6>] entry_SYSCALL_64_fastpath+0x1e/0xa8
[    1.294095] Code: ff ff 48 8b 43 60 48 2b 43 50 88 43 4e 5b 5d f3 c3 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 <f3> a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4 c3 0f 1f 80

Tested the patch and noticed a crash on boot once init starts.
Does module_finalize in load_module need to be moved before any of the other
functions perhaps?

--chris

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


#1369558

FromJiri Kosina <jikos@kernel.org>
Date2016-04-01 21:40 +0200
Message-ID<rjd5g-zB-11@gated-at.bofh.it>
In reply to#1369540
On Fri, 1 Apr 2016, Chris J Arges wrote:

> Loading, please wait...
> starting version 229
> [    1.182869] random: udevadm urandom read with 2 bits of entropy available
> [    1.241404] BUG: unable to handle kernel paging request at ffffffffc000f35f

Gah, we surely can't change pages with RO PTE. Thanks for such a prompt 
testing. You do have CONFIG_DEBUG_SET_MODULE_RONX set, don't you?

The patch below should fix that by marking the module RO (and relevant 
parts NX) only when it's guaranteed that .text is not going to be modified 
any more (and includes the error handling fix Miroslav spotted as well).

Thanks.



diff --git a/kernel/module.c b/kernel/module.c
index 5f71aa6..430606d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
 	return 0;
 }
 
-static int post_relocation(struct module *mod, const struct load_info *info)
+static void post_relocation(struct module *mod, const struct load_info *info)
 {
 	/* Sort exception table now relocations are done. */
 	sort_extable(mod->extable, mod->extable + mod->num_exentries);
@@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 
 	/* Setup kallsyms-specific fields. */
 	add_kallsyms(mod, info);
-
-	/* Arch-specific module finalizing. */
-	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
 /* Is this module of this name done loading?  No locks held. */
@@ -3441,10 +3438,6 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	/* This relies on module_mutex for list integrity. */
 	module_bug_finalize(info->hdr, info->sechdrs, mod);
 
-	/* Set RO and NX regions */
-	module_enable_ro(mod);
-	module_enable_nx(mod);
-
 	/* Mark state as coming so strong_try_module_get() ignores us,
 	 * but kallsyms etc. can see us. */
 	mod->state = MODULE_STATE_COMING;
@@ -3562,9 +3555,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err < 0)
 		goto free_modinfo;
 
-	err = post_relocation(mod, info);
-	if (err < 0)
-		goto free_modinfo;
+	post_relocation(mod, info);
 
 	flush_module_icache(mod);
 
@@ -3589,6 +3580,15 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto bug_cleanup;
 
+	/* Arch-specific module finalizing. */
+	err = module_finalize(info->hdr, info->sechdrs, mod);
+	if (err)
+		goto coming_cleanup;
+
+	/* Set RO and NX regions */
+	module_enable_ro(mod);
+	module_enable_nx(mod);
+
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
 				  -32768, 32767, mod,

-- 
Jiri Kosina
SUSE Labs

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


#1366256

FromMiroslav Benes <mbenes@suse.cz>
Date2016-03-29 15:10 +0200
Message-ID<ri1zd-6Hc-27@gated-at.bofh.it>
In reply to#1366195
[ adding CCs ]

On Tue, 29 Mar 2016, Chris J Arges wrote:

> Paravirtualized ops and livepatching currently don't mix very well and can
> cause undefined behavor such as oops, invalid opcodes or corrupted stacks.
> The original discussion of this issue can be found here [1].
> 
> I've written an example livepatch module that reproduces the issue [2].
> In order to trigger the issue you must first insert the module then trigger
> the paravirt ops by starting a VM.
> 
> In the thread here [1] a couple of solutions have been proposed:

Hi,

oh no... so this is not only about paravirt ops but also about 
alternatives, jump labels and so on, isn't it?

> 1) Jessica proposed using the Arch-independent patchset ensure that livepatch
> finishes writing its relas before apply_paravirt() is called. However, this
> introduces a bit more arch-dependent code. It would be useful to see if other
> arches are affected by this as well.

I think this is the way to go. Provided we have Jessica's two patch sets 
applied (arch-independent and notifiers removal) there are two options. We 
either move a call to klp_coming_module() somewhere before 
module_finalize(), or we move the problematic parts of module_finalize() 
to the end of load_module() (on x86 it is probably module_finalize() as a 
whole). The former is almost impossible because of the dependencies 
(ftrace and such), the latter should be doable (with very careful check we 
won't break anything).

> 2) Eugene proposed skipping application of the rela if the instruction to be
> relocated has already been changed. This passes the initial example [2];
> however its unclear if/how this will break things.

Hm, I don't like this one. It really depends on that the paravirt 
instructions which are supposed to be patched do not contain the 
code which needs to be relocated. This can be true for now, but we have to 
think long-term... which leads me to... If the new instructions need to be 
relocated... this is indeed a problem, right? You'd need to fix 
kpatch-build somehow to generate appropriate dynrelas for the paravirt 
patched code. But, during the livepatch module generation one does not 
know if the code would be patched by alternatives. Crap :/

Miroslav

> It may be good to weigh in here and get more eyes on this.
> Thanks,
> --chris
> 
> [1]: https://github.com/dynup/kpatch/issues/580
> [2]: http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl/livepatch.c
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web