Bug with paravirt ops and livepatches

Bug with paravirt ops and livepatches

[Chris J Arges]

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

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

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

Re: Bug with paravirt ops and livepatches

[Jiri Kosina]

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

Re: Bug with paravirt ops and livepatches

[Jiri Kosina]

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

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

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

Re: Bug with paravirt ops and livepatches

[Chris J Arges]

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

Re: Bug with paravirt ops and livepatches

[Chris J Arges]

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

Re: Bug with paravirt ops and livepatches

[Jiri Kosina]

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

Re: Bug with paravirt ops and livepatches

[Josh Poimboeuf]

On Fri, Apr 01, 2016 at 09:35:34PM +0200, Jiri Kosina wrote:
> 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,

So I think this doesn't fix the problem.  Dynamic relocations are
applied to the "patch module", whereas the above code deals with the
initialization order of the "patched module".  This distinction
originally confused me as well, until Jessica set me straight.

Let me try to illustrate the problem with an example.  Imagine you have
a patch module P which applies a patch to module M.  P replaces M's
function F with a new function F', which uses paravirt ops.

1) Patch P is loaded before module M.  P's new function F' has an
   instruction which is patched by apply_paravirt(), even though the
   patch hasn't been applied yet.

2) Module M is loaded.  Before applying the patch, livepatch tries to
   apply a klp_reloc to the instruction in F' which was already patched
   by apply_paravirt() in step 1.  This results in undefined behavior
   because it tries to patch the original instruction but instead
   patches the new paravirt instruction.

So the above patch makes no difference because the paravirt module
loading order doesn't really matter.

Jessica proposed some novel fixes here:
   
   https://github.com/dynup/kpatch/issues/580#issuecomment-183001652

But I get the feeling that any fix would be quite ugly and brittle.

I think the *real* problem here (and one that we've seen before) is that
we have a feature which allows you to load a patch to a module before
loading the module itself.  That really goes against the grain of how
module dependencies work.  It has already given us several headaches and
it makes the livepatch code a lot more complex.

I really think we need to take another hard look about whether it's
really worth it.  My current feeling is that it's not.

If we were able to get rid of that "feature", yes, the livepatch code
would be simpler, but there might be another awesome benefit: I suspect
we'd also be able to get rid of the need for specialized patch creation
tooling like kpatch-build.  Instead I think we could just specify
klp_relocs info in the source code of the patch, and just use kbuild to
build the patch module.  Not only would the livepatch code be simpler
(and much easier to wrap your head around), but the user space tooling
could be *vastly* simpler.

Of course, removing that feature might create some headaches for the
user.  It is nice to be able to load a big cumulative patch without
having to load all the dependencies first.  But maybe there are things
we could do to make the dependency problem more manageable.  e.g.,
splitting up patch modules to be per-object?  requiring the user to load
modules they don't need?  patching or replacing the module on disk?
copying the new module to a new locaiton and telling modprobe where to
find it?

I don't have all the answers but I think we should take a hard look at
some of these other approaches.

-- 
Josh

Re: Bug with paravirt ops and livepatches

[Jessica Yu]

+++ Josh Poimboeuf [04/04/16 11:14 -0500]:
>On Fri, Apr 01, 2016 at 09:35:34PM +0200, Jiri Kosina wrote:
>> 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,
>
>So I think this doesn't fix the problem. Dynamic relocations are
>applied to the "patch module", whereas the above code deals with the
>initialization order of the "patched module".  This distinction
>originally confused me as well, until Jessica set me straight.
>
>Let me try to illustrate the problem with an example.  Imagine you have
>a patch module P which applies a patch to module M.  P replaces M's
>function F with a new function F', which uses paravirt ops.
>
>1) Patch P is loaded before module M.  P's new function F' has an
>   instruction which is patched by apply_paravirt(), even though the
>   patch hasn't been applied yet.
>
>2) Module M is loaded.  Before applying the patch, livepatch tries to
>   apply a klp_reloc to the instruction in F' which was already patched
>   by apply_paravirt() in step 1. This results in undefined behavior
>   because it tries to patch the original instruction but instead
>   patches the new paravirt instruction.
>
>So the above patch makes no difference because the paravirt module
>loading order doesn't really matter.

Yup, exactly, the crux of the issue is the fact that we're hacking the
module load order and delying certain operations (e.g. applying
relocations) in order to allow patching of a not-yet-loaded module.

In this case, the problem can be briefly summarized as follows:
For any patch module, apply_paravirt/alternatives needs to come after
apply_relocate_add (recall that these operations affect the
replacement functions in the patch module). Right now we have it the
other way around: apply_paravirt() is patching our replacement
functions in module_finalize() before the to-be-patched module is
loaded and apply_relocate_add() can be called. 

The hack fix I had involved delaying the apply_paravirt() call
(basically calling is_livepatch_module() in the x86 code, and skipping
the paravirt/alternative calls if true), and when the to-be-patched
module loads, having livepatch finish up those apply_{paravirt,alternative}
calls just before apply_relocate_add(). Unfortunately this involved
adding arch-specific code :-\, but I'm not sure there are really any
elegant solutions out there to this problem.

Jessica

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Mon, 4 Apr 2016, Josh Poimboeuf wrote:

> So I think this doesn't fix the problem.  Dynamic relocations are
> applied to the "patch module", whereas the above code deals with the
> initialization order of the "patched module".  This distinction
> originally confused me as well, until Jessica set me straight.
> 
> Let me try to illustrate the problem with an example.  Imagine you have
> a patch module P which applies a patch to module M.  P replaces M's
> function F with a new function F', which uses paravirt ops.
> 
> 1) Patch P is loaded before module M.  P's new function F' has an
>    instruction which is patched by apply_paravirt(), even though the
>    patch hasn't been applied yet.
> 
> 2) Module M is loaded.  Before applying the patch, livepatch tries to
>    apply a klp_reloc to the instruction in F' which was already patched
>    by apply_paravirt() in step 1.  This results in undefined behavior
>    because it tries to patch the original instruction but instead
>    patches the new paravirt instruction.
> 
> So the above patch makes no difference because the paravirt module
> loading order doesn't really matter.

Hi,

we are trying really hard to understand the actual culprit here and as it 
is quite confusing I have several questions/comments...

1. can you provide dynrela sections of the patch module from 
https://github.com/dynup/kpatch/issues/580? What is interesting is that 
kvm_arch_vm_ioctl() function contains rela records only for trivial (== 
exported) symbols from the first look. The problem should be there only if 
you want to patch a function which reference some paravirt_ops unexported 
symbol. For that symbol dynrela should be created.

2. I am almost 100 percent sure that the second problem Chris mentions in 
github post is something different. If he patches only kvm_arch_vm_ioctl() 
so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no 
problem. Because it is a trivial livepatching case. There are no dynrelas 
and no alternatives in the patch module. The crash is suspicious and we 
have a problem somewhere. Chris, can you please provide more info about 
that? That is how exactly you used kallsyms_lookup_name() and so on...

3. Now I'll try to explain what really happens here... because of 1. and 
the way the alternatives and relocations are implemented the only 
problematic case is when one wants to patch a module which introduces its 
own alternatives. Which is probably the case of KVM. Why?

When alternative is used, the call to some pv_*_ops.function is placed 
somewhere. The address of this location is stored in a separate elf 
section and when apply_paravirt() is called it takes the address and 
place the new code there (or it does not, it depends :)). When the 
alternative is in some module and pv_*_ops is exported, which is the 
usual case, there is no problem. No dynrela needs to be used when a user 
wants to patch such function with the alternative.

The only problem is when the function uses unexported pv_*_ops (or some 
other alternative symbol) from its own object file. When the user wants to 
patch this one there is a need for dynrela.

So what really happens is that we load a patch module, we do not apply 
our relocations but we do apply alternatives to the patch module, which is 
problematic because there is a reference to something which is not yet 
resolved (that is unexported pv_*_ops). When a patched module arrives we 
happily resolve relocations but since we do not apply alternatives again 
there is a rubbish in the patching code.

Is this all correct?

> Jessica proposed some novel fixes here:
>    
>    https://github.com/dynup/kpatch/issues/580#issuecomment-183001652

Yes, I think that fix should be the same we have for relocations. To 
postpone the alternatives applications. Maybe it is even possible to do it 
in a similar way. And yes on one hand it is gonna be ugly, on the other 
hand it is gonna be consistent with relocations. 

> I think the *real* problem here (and one that we've seen before) is that
> we have a feature which allows you to load a patch to a module before
> loading the module itself.  That really goes against the grain of how
> module dependencies work.  It has already given us several headaches and
> it makes the livepatch code a lot more complex.
> 
> I really think we need to take another hard look about whether it's
> really worth it.  My current feeling is that it's not.

I can only say that maybe about 1/3 of kgraft patches we currently have 
are for modules (1/3 is probably not correct but it is definitely 
non-trivial number). It would be really unfortunate to load all the 
to-be-patched modules when a patch module is applied.

This does not mean that we cannot solve it in another way as you propose 
below. I have to think about it.

Miroslav

> If we were able to get rid of that "feature", yes, the livepatch code
> would be simpler, but there might be another awesome benefit: I suspect
> we'd also be able to get rid of the need for specialized patch creation
> tooling like kpatch-build.  Instead I think we could just specify
> klp_relocs info in the source code of the patch, and just use kbuild to
> build the patch module.  Not only would the livepatch code be simpler
> (and much easier to wrap your head around), but the user space tooling
> could be *vastly* simpler.
> 
> Of course, removing that feature might create some headaches for the
> user.  It is nice to be able to load a big cumulative patch without
> having to load all the dependencies first.  But maybe there are things
> we could do to make the dependency problem more manageable.  e.g.,
> splitting up patch modules to be per-object?  requiring the user to load
> modules they don't need?  patching or replacing the module on disk?
> copying the new module to a new locaiton and telling modprobe where to
> find it?
> 
> I don't have all the answers but I think we should take a hard look at
> some of these other approaches.
> 
> -- 
> Josh
> 

Re: Bug with paravirt ops and livepatches

[Evgenii Shatokhin]

05.04.2016 16:07, Miroslav Benes пишет:
> On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
>
>> So I think this doesn't fix the problem.  Dynamic relocations are
>> applied to the "patch module", whereas the above code deals with the
>> initialization order of the "patched module".  This distinction
>> originally confused me as well, until Jessica set me straight.
>>
>> Let me try to illustrate the problem with an example.  Imagine you have
>> a patch module P which applies a patch to module M.  P replaces M's
>> function F with a new function F', which uses paravirt ops.
>>
>> 1) Patch P is loaded before module M.  P's new function F' has an
>>     instruction which is patched by apply_paravirt(), even though the
>>     patch hasn't been applied yet.
>>
>> 2) Module M is loaded.  Before applying the patch, livepatch tries to
>>     apply a klp_reloc to the instruction in F' which was already patched
>>     by apply_paravirt() in step 1.  This results in undefined behavior
>>     because it tries to patch the original instruction but instead
>>     patches the new paravirt instruction.
>>
>> So the above patch makes no difference because the paravirt module
>> loading order doesn't really matter.
>
> Hi,
>
> we are trying really hard to understand the actual culprit here and as it
> is quite confusing I have several questions/comments...
>
> 1. can you provide dynrela sections of the patch module from
> https://github.com/dynup/kpatch/issues/580? What is interesting is that
> kvm_arch_vm_ioctl() function contains rela records only for trivial (==
> exported) symbols from the first look. The problem should be there only if
> you want to patch a function which reference some paravirt_ops unexported
> symbol. For that symbol dynrela should be created.

As far as I understand it, create-diff-object does not check if a symbol 
is exported or not when a patch for a module rather than for vmlinux is 
being prepared.

In such cases, it only checks if the referenced global symbol is defined 
somewhere in that module and if not, creates a dynrela for it.

The code is in kpatch_create_dynamic_rela_sections() from 
https://github.com/dynup/kpatch/blob/master/kpatch-build/create-diff-object.c.

>
> 2. I am almost 100 percent sure that the second problem Chris mentions in
> github post is something different. If he patches only kvm_arch_vm_ioctl()
> so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no
> problem. Because it is a trivial livepatching case. There are no dynrelas
> and no alternatives in the patch module. The crash is suspicious and we
> have a problem somewhere. Chris, can you please provide more info about
> that? That is how exactly you used kallsyms_lookup_name() and so on...
>
> 3. Now I'll try to explain what really happens here... because of 1. and
> the way the alternatives and relocations are implemented the only
> problematic case is when one wants to patch a module which introduces its
> own alternatives. Which is probably the case of KVM. Why?
>
> When alternative is used, the call to some pv_*_ops.function is placed
> somewhere. The address of this location is stored in a separate elf
> section and when apply_paravirt() is called it takes the address and
> place the new code there (or it does not, it depends :)). When the
> alternative is in some module and pv_*_ops is exported, which is the
> usual case, there is no problem. No dynrela needs to be used when a user
> wants to patch such function with the alternative.
>
> The only problem is when the function uses unexported pv_*_ops (or some
> other alternative symbol) from its own object file. When the user wants to
> patch this one there is a need for dynrela.
>
> So what really happens is that we load a patch module, we do not apply
> our relocations but we do apply alternatives to the patch module, which is
> problematic because there is a reference to something which is not yet
> resolved (that is unexported pv_*_ops). When a patched module arrives we
> happily resolve relocations but since we do not apply alternatives again
> there is a rubbish in the patching code.
>
> Is this all correct?
>
>> Jessica proposed some novel fixes here:
>>
>>     https://github.com/dynup/kpatch/issues/580#issuecomment-183001652
>
> Yes, I think that fix should be the same we have for relocations. To
> postpone the alternatives applications. Maybe it is even possible to do it
> in a similar way. And yes on one hand it is gonna be ugly, on the other
> hand it is gonna be consistent with relocations.
>
>> I think the *real* problem here (and one that we've seen before) is that
>> we have a feature which allows you to load a patch to a module before
>> loading the module itself.  That really goes against the grain of how
>> module dependencies work.  It has already given us several headaches and
>> it makes the livepatch code a lot more complex.
>>
>> I really think we need to take another hard look about whether it's
>> really worth it.  My current feeling is that it's not.
>
> I can only say that maybe about 1/3 of kgraft patches we currently have
> are for modules (1/3 is probably not correct but it is definitely
> non-trivial number). It would be really unfortunate to load all the
> to-be-patched modules when a patch module is applied.
>
> This does not mean that we cannot solve it in another way as you propose
> below. I have to think about it.
>
> Miroslav
>
>> If we were able to get rid of that "feature", yes, the livepatch code
>> would be simpler, but there might be another awesome benefit: I suspect
>> we'd also be able to get rid of the need for specialized patch creation
>> tooling like kpatch-build.  Instead I think we could just specify
>> klp_relocs info in the source code of the patch, and just use kbuild to
>> build the patch module.  Not only would the livepatch code be simpler
>> (and much easier to wrap your head around), but the user space tooling
>> could be *vastly* simpler.
>>
>> Of course, removing that feature might create some headaches for the
>> user.  It is nice to be able to load a big cumulative patch without
>> having to load all the dependencies first.  But maybe there are things
>> we could do to make the dependency problem more manageable.  e.g.,
>> splitting up patch modules to be per-object?  requiring the user to load
>> modules they don't need?  patching or replacing the module on disk?
>> copying the new module to a new locaiton and telling modprobe where to
>> find it?
>>
>> I don't have all the answers but I think we should take a hard look at
>> some of these other approaches.
>>
>> --
>> Josh
>>

Regards,
Evgenii

Re: Bug with paravirt ops and livepatches

[Josh Poimboeuf]

On Tue, Apr 05, 2016 at 03:07:13PM +0200, Miroslav Benes wrote:
> On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
> 
> > So I think this doesn't fix the problem.  Dynamic relocations are
> > applied to the "patch module", whereas the above code deals with the
> > initialization order of the "patched module".  This distinction
> > originally confused me as well, until Jessica set me straight.
> > 
> > Let me try to illustrate the problem with an example.  Imagine you have
> > a patch module P which applies a patch to module M.  P replaces M's
> > function F with a new function F', which uses paravirt ops.
> > 
> > 1) Patch P is loaded before module M.  P's new function F' has an
> >    instruction which is patched by apply_paravirt(), even though the
> >    patch hasn't been applied yet.
> > 
> > 2) Module M is loaded.  Before applying the patch, livepatch tries to
> >    apply a klp_reloc to the instruction in F' which was already patched
> >    by apply_paravirt() in step 1.  This results in undefined behavior
> >    because it tries to patch the original instruction but instead
> >    patches the new paravirt instruction.
> > 
> > So the above patch makes no difference because the paravirt module
> > loading order doesn't really matter.
> 
> Hi,
> 
> we are trying really hard to understand the actual culprit here and as it 
> is quite confusing I have several questions/comments...
> 
> 1. can you provide dynrela sections of the patch module from 
> https://github.com/dynup/kpatch/issues/580? What is interesting is that 
> kvm_arch_vm_ioctl() function contains rela records only for trivial (== 
> exported) symbols from the first look. The problem should be there only if 
> you want to patch a function which reference some paravirt_ops unexported 
> symbol. For that symbol dynrela should be created.

Agreed, kpatch-build could be smarter here.  However the problem still
exists for non-exported symbols, at least in theory.

> 2. I am almost 100 percent sure that the second problem Chris mentions in 
> github post is something different. If he patches only kvm_arch_vm_ioctl() 
> so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no 
> problem. Because it is a trivial livepatching case. There are no dynrelas 
> and no alternatives in the patch module. The crash is suspicious and we 
> have a problem somewhere. Chris, can you please provide more info about 
> that? That is how exactly you used kallsyms_lookup_name() and so on...
> 
> 3. Now I'll try to explain what really happens here... because of 1. and 
> the way the alternatives and relocations are implemented the only 
> problematic case is when one wants to patch a module which introduces its 
> own alternatives. Which is probably the case of KVM. Why?
> 
> When alternative is used, the call to some pv_*_ops.function is placed 
> somewhere. The address of this location is stored in a separate elf 
> section and when apply_paravirt() is called it takes the address and 
> place the new code there (or it does not, it depends :)). When the 
> alternative is in some module and pv_*_ops is exported, which is the 
> usual case, there is no problem. No dynrela needs to be used when a user 
> wants to patch such function with the alternative.
> 
> The only problem is when the function uses unexported pv_*_ops (or some 
> other alternative symbol) from its own object file. When the user wants to 
> patch this one there is a need for dynrela.
> 
> So what really happens is that we load a patch module, we do not apply 
> our relocations but we do apply alternatives to the patch module, which is 
> problematic because there is a reference to something which is not yet 
> resolved (that is unexported pv_*_ops). When a patched module arrives we 
> happily resolve relocations but since we do not apply alternatives again 
> there is a rubbish in the patching code.
> 
> Is this all correct?

That all sounds mostly right to me, except it sounds like you're
conflating alternatives with paravirt patching, when in fact they're two
separate mechanisms.  Paravirt patches are specific to virtualization,
but alternatives can be used on both virt and HW.

And then there's also jump labels.

In any case I don't think we should get too bogged down into details.
Even it's not currently possible to hit this bug today, it could easily
come back to bite us later.

> > Jessica proposed some novel fixes here:
> >    
> >    https://github.com/dynup/kpatch/issues/580#issuecomment-183001652
> 
> Yes, I think that fix should be the same we have for relocations. To 
> postpone the alternatives applications. Maybe it is even possible to do it 
> in a similar way. And yes on one hand it is gonna be ugly, on the other 
> hand it is gonna be consistent with relocations. 

Yeah.  If we made it consistent with how we do relocations in Jessica's
arch-independent patches, it looks like apply_paravirt() and
apply_alternatives() already take section data as input, so we might be
able to call them as-is from livepatch code.  The jump label code is
different and would probably need some rework for us to be able to reuse
it.

So maybe the kernel code wouldn't be *too* bad.  But it does add
complexity and it would also further complicate our patch creation tools
as well.

> > I think the *real* problem here (and one that we've seen before) is that
> > we have a feature which allows you to load a patch to a module before
> > loading the module itself.  That really goes against the grain of how
> > module dependencies work.  It has already given us several headaches and
> > it makes the livepatch code a lot more complex.
> > 
> > I really think we need to take another hard look about whether it's
> > really worth it.  My current feeling is that it's not.
> 
> I can only say that maybe about 1/3 of kgraft patches we currently have 
> are for modules (1/3 is probably not correct but it is definitely 
> non-trivial number). It would be really unfortunate to load all the 
> to-be-patched modules when a patch module is applied.
> 
> This does not mean that we cannot solve it in another way as you propose 
> below. I have to think about it.

Yeah, it's a complicated issue and I need to think about it some more as
well.

> Miroslav
> 
> > If we were able to get rid of that "feature", yes, the livepatch code
> > would be simpler, but there might be another awesome benefit: I suspect
> > we'd also be able to get rid of the need for specialized patch creation
> > tooling like kpatch-build.  Instead I think we could just specify
> > klp_relocs info in the source code of the patch, and just use kbuild to
> > build the patch module.  Not only would the livepatch code be simpler
> > (and much easier to wrap your head around), but the user space tooling
> > could be *vastly* simpler.
> > 
> > Of course, removing that feature might create some headaches for the
> > user.  It is nice to be able to load a big cumulative patch without
> > having to load all the dependencies first.  But maybe there are things
> > we could do to make the dependency problem more manageable.  e.g.,
> > splitting up patch modules to be per-object?  requiring the user to load
> > modules they don't need?  patching or replacing the module on disk?
> > copying the new module to a new locaiton and telling modprobe where to
> > find it?
> > 
> > I don't have all the answers but I think we should take a hard look at
> > some of these other approaches.

-- 
Josh

Re: Bug with paravirt ops and livepatches

[Jessica Yu]

+++ Miroslav Benes [05/04/16 15:07 +0200]:
>On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
>
>> So I think this doesn't fix the problem.  Dynamic relocations are
>> applied to the "patch module", whereas the above code deals with the
>> initialization order of the "patched module".  This distinction
>> originally confused me as well, until Jessica set me straight.
>>
>> Let me try to illustrate the problem with an example.  Imagine you have
>> a patch module P which applies a patch to module M.  P replaces M's
>> function F with a new function F', which uses paravirt ops.
>>
>> 1) Patch P is loaded before module M.  P's new function F' has an
>>    instruction which is patched by apply_paravirt(), even though the
>>    patch hasn't been applied yet.
>>
>> 2) Module M is loaded.  Before applying the patch, livepatch tries to
>>    apply a klp_reloc to the instruction in F' which was already patched
>>    by apply_paravirt() in step 1.  This results in undefined behavior
>>    because it tries to patch the original instruction but instead
>>    patches the new paravirt instruction.
>>
>> So the above patch makes no difference because the paravirt module
>> loading order doesn't really matter.
>
>Hi,
>
>we are trying really hard to understand the actual culprit here and as it
>is quite confusing I have several questions/comments...

I don't have a 100% clear understanding of the whole picture either,
but I'll try to help clarify up some things..

>1. can you provide dynrela sections of the patch module from
>https://github.com/dynup/kpatch/issues/580? What is interesting is that
>kvm_arch_vm_ioctl() function contains rela records only for trivial (==
>exported) symbols from the first look. The problem should be there only if
>you want to patch a function which reference some paravirt_ops unexported
>symbol. For that symbol dynrela should be created.

Just to dispel some confusion over this, kpatch isn't "smart" enough
yet to differentiate between exported and non-exported symbols, as
Evgenii already mentioned. Just global and local, and whether the
symbol belongs to a module or vmlinux. So that means dynrelas are
indeed being created for the pv_*_ops symbols, despite the fact they
are exported.

So this is part of the problem, since apply_paravirt() runs first (as
part of module_finalize()), and dynrelas are written second,
livepatch is clobbering/overwriting those paravirt patch sites that
apply_paravirt had fixed up already.

If you skip generating dynrelas that affect those paravirt patch
sites, I can verify that the crash disappears, since in that case
we're not stepping over those paravirt patch sites with our dynrelas
(just to test and verify the problem, I kind of cheated and forced
kpatch to not generate dynrelas for pv_*_ops symbols, and the panics
disappear). Another temporary solution was to not apply the dynrela
if the target memory is not all zero's. These approaches aren't
reliable enough to serve as a permanent solution but they do give us
a better idea of what's happening. Here are some more relevant
comments -
https://github.com/dynup/kpatch/issues/580#issuecomment-199314636
https://github.com/dynup/kpatch/issues/580#issuecomment-202395452

Jessica

Re: Bug with paravirt ops and livepatches

[Chris J Arges]

On Tue, Apr 05, 2016 at 03:07:13PM +0200, Miroslav Benes wrote:
> On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
> 
> > So I think this doesn't fix the problem.  Dynamic relocations are
> > applied to the "patch module", whereas the above code deals with the
> > initialization order of the "patched module".  This distinction
> > originally confused me as well, until Jessica set me straight.
> > 
> > Let me try to illustrate the problem with an example.  Imagine you have
> > a patch module P which applies a patch to module M.  P replaces M's
> > function F with a new function F', which uses paravirt ops.
> > 
> > 1) Patch P is loaded before module M.  P's new function F' has an
> >    instruction which is patched by apply_paravirt(), even though the
> >    patch hasn't been applied yet.
> > 
> > 2) Module M is loaded.  Before applying the patch, livepatch tries to
> >    apply a klp_reloc to the instruction in F' which was already patched
> >    by apply_paravirt() in step 1.  This results in undefined behavior
> >    because it tries to patch the original instruction but instead
> >    patches the new paravirt instruction.
> > 
> > So the above patch makes no difference because the paravirt module
> > loading order doesn't really matter.
> 
> Hi,
> 
> we are trying really hard to understand the actual culprit here and as it 
> is quite confusing I have several questions/comments...
> 
> 1. can you provide dynrela sections of the patch module from 
> https://github.com/dynup/kpatch/issues/580? What is interesting is that 
> kvm_arch_vm_ioctl() function contains rela records only for trivial (== 
> exported) symbols from the first look. The problem should be there only if 
> you want to patch a function which reference some paravirt_ops unexported 
> symbol. For that symbol dynrela should be created.
> 
> 2. I am almost 100 percent sure that the second problem Chris mentions in 
> github post is something different. If he patches only kvm_arch_vm_ioctl() 
> so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no 
> problem. Because it is a trivial livepatching case. There are no dynrelas 
> and no alternatives in the patch module. The crash is suspicious and we 
> have a problem somewhere. Chris, can you please provide more info about 
> that? That is how exactly you used kallsyms_lookup_name() and so on...
> 

Miroslav,

In my original comment I was trying to see if this was a kpatch-build specific
issue and that's why I tried to reproduce using just a simple out of tree built
livepatch module. For this case I copied kvm_arch_vm_ioctl into
__kvm_arch_vm_ioctl (to simplify patching). I then built and installed this
base kernel and kvm.ko module.  However, for the crashing and non-crashing
cases I used two slightly different base kernels and livepatch code.

I just re-tested this using the latest livepatch.git/for-next code and have the
following:

This crashes if I use kallsyms_lookup_name to find the __kvm_arch_vm_ioctl
symbol and then call it from my livepatch:
http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.crashes/

This works if I export __kvm_arch_vm_ioctl in the base kernel, and in the
livepatch just call it directly:
http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works/

Where livepatch.c is the livepatch and kernel.patch is the base kernel patch for
both directories.

--chris

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Tue, 5 Apr 2016, Jessica Yu wrote:

> +++ Miroslav Benes [05/04/16 15:07 +0200]:
> > On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
> > 
> > > So I think this doesn't fix the problem.  Dynamic relocations are
> > > applied to the "patch module", whereas the above code deals with the
> > > initialization order of the "patched module".  This distinction
> > > originally confused me as well, until Jessica set me straight.
> > > 
> > > Let me try to illustrate the problem with an example.  Imagine you have
> > > a patch module P which applies a patch to module M.  P replaces M's
> > > function F with a new function F', which uses paravirt ops.
> > > 
> > > 1) Patch P is loaded before module M.  P's new function F' has an
> > >    instruction which is patched by apply_paravirt(), even though the
> > >    patch hasn't been applied yet.
> > > 
> > > 2) Module M is loaded.  Before applying the patch, livepatch tries to
> > >    apply a klp_reloc to the instruction in F' which was already patched
> > >    by apply_paravirt() in step 1.  This results in undefined behavior
> > >    because it tries to patch the original instruction but instead
> > >    patches the new paravirt instruction.
> > > 
> > > So the above patch makes no difference because the paravirt module
> > > loading order doesn't really matter.
> > 
> > Hi,
> > 
> > we are trying really hard to understand the actual culprit here and as it
> > is quite confusing I have several questions/comments...
> 
> I don't have a 100% clear understanding of the whole picture either,
> but I'll try to help clarify up some things..
> 
> > 1. can you provide dynrela sections of the patch module from
> > https://github.com/dynup/kpatch/issues/580? What is interesting is that
> > kvm_arch_vm_ioctl() function contains rela records only for trivial (==
> > exported) symbols from the first look. The problem should be there only if
> > you want to patch a function which reference some paravirt_ops unexported
> > symbol. For that symbol dynrela should be created.
> 
> Just to dispel some confusion over this, kpatch isn't "smart" enough
> yet to differentiate between exported and non-exported symbols, as
> Evgenii already mentioned. Just global and local, and whether the
> symbol belongs to a module or vmlinux. So that means dynrelas are
> indeed being created for the pv_*_ops symbols, despite the fact they
> are exported.

Ah, ok. So for vmlinux you do not generate dynrelas for exported symbols 
while for modules there is no such logic. This makes the problem only more 
pronounced. We were wondering how it was possible there were crashes in 
this case as there were only exported symbols there. This is an 
explanation.

> So this is part of the problem, since apply_paravirt() runs first (as
> part of module_finalize()), and dynrelas are written second,
> livepatch is clobbering/overwriting those paravirt patch sites that
> apply_paravirt had fixed up already.

Yes.

> If you skip generating dynrelas that affect those paravirt patch
> sites, I can verify that the crash disappears, since in that case
> we're not stepping over those paravirt patch sites with our dynrelas
> (just to test and verify the problem, I kind of cheated and forced
> kpatch to not generate dynrelas for pv_*_ops symbols, and the panics
> disappear). Another temporary solution was to not apply the dynrela
> if the target memory is not all zero's. These approaches aren't
> reliable enough to serve as a permanent solution but they do give us
> a better idea of what's happening. Here are some more relevant
> comments -

Agreed.

> https://github.com/dynup/kpatch/issues/580#issuecomment-199314636

As Josh correctly pointed out there could be paravirt instructions that
need relocations. Maybe spinlocks. And as I browsed through the code I got 
the feeling that the relocations were needed just for the very application 
of the paravirt instruction.

See PVOP_VCALL* macros and especially PARAVIRT_CALL. There is a relocation 
record for this call in an object file. Must be.

Anyway I see there are some new comments on github. I'll look at those. 
But I'd prefer to discuss all the relevant things (that is kpatch 
unspecific) here. It would make it easier.

Miroslav

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Wed, 6 Apr 2016, Miroslav Benes wrote:

> Anyway I see there are some new comments on github. I'll look at those. 
> But I'd prefer to discuss all the relevant things (that is kpatch 
> unspecific) here. It would make it easier.

And you do (after seeing dates of the posts there), sorry for the noise.

Jessica, I think I am perfectly fine with introducing some arch-specific 
code because of this problem.

We used generic apply_relocate_add() because it was a single 
arch-independent entry point. There is no such things for paravirt_ops, 
alternatives, jump labels and such things. In fact only module_finalize() 
is there and that is not enough. So some arch-specific code in livepatch 
seems to be unnecessary.

Cheers,
Miroslav

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Wed, 6 Apr 2016, Chris J Arges wrote:

> On Tue, Apr 05, 2016 at 03:07:13PM +0200, Miroslav Benes wrote:
> > On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
> > 
> > > So I think this doesn't fix the problem.  Dynamic relocations are
> > > applied to the "patch module", whereas the above code deals with the
> > > initialization order of the "patched module".  This distinction
> > > originally confused me as well, until Jessica set me straight.
> > > 
> > > Let me try to illustrate the problem with an example.  Imagine you have
> > > a patch module P which applies a patch to module M.  P replaces M's
> > > function F with a new function F', which uses paravirt ops.
> > > 
> > > 1) Patch P is loaded before module M.  P's new function F' has an
> > >    instruction which is patched by apply_paravirt(), even though the
> > >    patch hasn't been applied yet.
> > > 
> > > 2) Module M is loaded.  Before applying the patch, livepatch tries to
> > >    apply a klp_reloc to the instruction in F' which was already patched
> > >    by apply_paravirt() in step 1.  This results in undefined behavior
> > >    because it tries to patch the original instruction but instead
> > >    patches the new paravirt instruction.
> > > 
> > > So the above patch makes no difference because the paravirt module
> > > loading order doesn't really matter.
> > 
> > Hi,
> > 
> > we are trying really hard to understand the actual culprit here and as it 
> > is quite confusing I have several questions/comments...
> > 
> > 1. can you provide dynrela sections of the patch module from 
> > https://github.com/dynup/kpatch/issues/580? What is interesting is that 
> > kvm_arch_vm_ioctl() function contains rela records only for trivial (== 
> > exported) symbols from the first look. The problem should be there only if 
> > you want to patch a function which reference some paravirt_ops unexported 
> > symbol. For that symbol dynrela should be created.
> > 
> > 2. I am almost 100 percent sure that the second problem Chris mentions in 
> > github post is something different. If he patches only kvm_arch_vm_ioctl() 
> > so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no 
> > problem. Because it is a trivial livepatching case. There are no dynrelas 
> > and no alternatives in the patch module. The crash is suspicious and we 
> > have a problem somewhere. Chris, can you please provide more info about 
> > that? That is how exactly you used kallsyms_lookup_name() and so on...
> > 
> 
> Miroslav,
> 
> In my original comment I was trying to see if this was a kpatch-build specific
> issue and that's why I tried to reproduce using just a simple out of tree built
> livepatch module. For this case I copied kvm_arch_vm_ioctl into
> __kvm_arch_vm_ioctl (to simplify patching). I then built and installed this
> base kernel and kvm.ko module.  However, for the crashing and non-crashing
> cases I used two slightly different base kernels and livepatch code.
> 
> I just re-tested this using the latest livepatch.git/for-next code and have the
> following:
> 
> This crashes if I use kallsyms_lookup_name to find the __kvm_arch_vm_ioctl
> symbol and then call it from my livepatch:
> http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.crashes/
> 
> This works if I export __kvm_arch_vm_ioctl in the base kernel, and in the
> livepatch just call it directly:
> http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works/
> 
> Where livepatch.c is the livepatch and kernel.patch is the base kernel patch for
> both directories.

Thank you, Chris.

First (before to reproduce it) I have to ask one thing. What is the order 
of module loading? Do you first load kvm.ko and then livepatch or the 
opposite.

It does not matter in the first case where the function is exported. 
Because of it there would be a dependency of the modules so once you load 
livepatch module kvm.ko is loaded before it. This is the only way to do 
it. Undefined symbols of livepatch module are thus easily resolved.

The situation differs in the second case though. You use 
kallsyms_lookup_name to circumvent dynrela for __kvm_arch_vm_ioctl (it is 
not exported now). So when you load livepatch module kallsyms_lookup_name 
would in fact fail and the kernel would then crash. Although the bug would 
be different (NULL pointer dereference) and I guess you are aware of that 
because you have a printk there. But just to make sure...

Thanks
Miroslav

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Wed, 6 Apr 2016, Miroslav Benes wrote:

> On Wed, 6 Apr 2016, Miroslav Benes wrote:
> 
> > Anyway I see there are some new comments on github. I'll look at those. 
> > But I'd prefer to discuss all the relevant things (that is kpatch 
> > unspecific) here. It would make it easier.
> 
> And you do (after seeing dates of the posts there), sorry for the noise.
> 
> Jessica, I think I am perfectly fine with introducing some arch-specific 
> code because of this problem.
> 
> We used generic apply_relocate_add() because it was a single 
> arch-independent entry point. There is no such things for paravirt_ops, 
> alternatives, jump labels and such things. In fact only module_finalize() 
> is there and that is not enough. So some arch-specific code in livepatch 
> seems to be unnecessary.

s/unnecessary/necessary/

Re: Bug with paravirt ops and livepatches

[Chris J Arges]

On Wed, Apr 06, 2016 at 11:09:04AM +0200, Miroslav Benes wrote:
> On Wed, 6 Apr 2016, Chris J Arges wrote:
> 
> > On Tue, Apr 05, 2016 at 03:07:13PM +0200, Miroslav Benes wrote:
> > > On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
> > > 
> > > > So I think this doesn't fix the problem.  Dynamic relocations are
> > > > applied to the "patch module", whereas the above code deals with the
> > > > initialization order of the "patched module".  This distinction
> > > > originally confused me as well, until Jessica set me straight.
> > > > 
> > > > Let me try to illustrate the problem with an example.  Imagine you have
> > > > a patch module P which applies a patch to module M.  P replaces M's
> > > > function F with a new function F', which uses paravirt ops.
> > > > 
> > > > 1) Patch P is loaded before module M.  P's new function F' has an
> > > >    instruction which is patched by apply_paravirt(), even though the
> > > >    patch hasn't been applied yet.
> > > > 
> > > > 2) Module M is loaded.  Before applying the patch, livepatch tries to
> > > >    apply a klp_reloc to the instruction in F' which was already patched
> > > >    by apply_paravirt() in step 1.  This results in undefined behavior
> > > >    because it tries to patch the original instruction but instead
> > > >    patches the new paravirt instruction.
> > > > 
> > > > So the above patch makes no difference because the paravirt module
> > > > loading order doesn't really matter.
> > > 
> > > Hi,
> > > 
> > > we are trying really hard to understand the actual culprit here and as it 
> > > is quite confusing I have several questions/comments...
> > > 
> > > 1. can you provide dynrela sections of the patch module from 
> > > https://github.com/dynup/kpatch/issues/580? What is interesting is that 
> > > kvm_arch_vm_ioctl() function contains rela records only for trivial (== 
> > > exported) symbols from the first look. The problem should be there only if 
> > > you want to patch a function which reference some paravirt_ops unexported 
> > > symbol. For that symbol dynrela should be created.
> > > 
> > > 2. I am almost 100 percent sure that the second problem Chris mentions in 
> > > github post is something different. If he patches only kvm_arch_vm_ioctl() 
> > > so that it calls his exported __kvm_arch_vm_ioctl() duplicate there is no 
> > > problem. Because it is a trivial livepatching case. There are no dynrelas 
> > > and no alternatives in the patch module. The crash is suspicious and we 
> > > have a problem somewhere. Chris, can you please provide more info about 
> > > that? That is how exactly you used kallsyms_lookup_name() and so on...
> > > 
> > 
> > Miroslav,
> > 
> > In my original comment I was trying to see if this was a kpatch-build specific
> > issue and that's why I tried to reproduce using just a simple out of tree built
> > livepatch module. For this case I copied kvm_arch_vm_ioctl into
> > __kvm_arch_vm_ioctl (to simplify patching). I then built and installed this
> > base kernel and kvm.ko module.  However, for the crashing and non-crashing
> > cases I used two slightly different base kernels and livepatch code.
> > 
> > I just re-tested this using the latest livepatch.git/for-next code and have the
> > following:
> > 
> > This crashes if I use kallsyms_lookup_name to find the __kvm_arch_vm_ioctl
> > symbol and then call it from my livepatch:
> > http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.crashes/
> > 
> > This works if I export __kvm_arch_vm_ioctl in the base kernel, and in the
> > livepatch just call it directly:
> > http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works/
> > 
> > Where livepatch.c is the livepatch and kernel.patch is the base kernel patch for
> > both directories.
> 
> Thank you, Chris.
> 
> First (before to reproduce it) I have to ask one thing. What is the order 
> of module loading? Do you first load kvm.ko and then livepatch or the 
> opposite.
> 

I just tested this experimentally, in both cases kvm and kvm_intel was loaded
before the livepatch module was inserted. The bug is triggered only when I
actually start the VM and only when using my kallsyms_lookup_name hack.

> It does not matter in the first case where the function is exported. 
> Because of it there would be a dependency of the modules so once you load 
> livepatch module kvm.ko is loaded before it. This is the only way to do 
> it. Undefined symbols of livepatch module are thus easily resolved.
> 
> The situation differs in the second case though. You use 
> kallsyms_lookup_name to circumvent dynrela for __kvm_arch_vm_ioctl (it is 
> not exported now). So when you load livepatch module kallsyms_lookup_name 
> would in fact fail and the kernel would then crash. Although the bug would 
> be different (NULL pointer dereference) and I guess you are aware of that 
> because you have a printk there. But just to make sure...
> 
> Thanks
> Miroslav

I think this approach needs more thought and my code has bug(s).

--chris

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Wed, 6 Apr 2016, Chris J Arges wrote:

> I think this approach needs more thought and my code has bug(s).

And indeed there is...

long (*__kvm_arch_vm_ioctl)(struct file *filp, unsigned long ioctl, unsigned long arg) = NULL;

Use a different name than __kvm_arch_vm_ioctl and (ideally) make it 
static.

kallsyms_lookup_name("__kvm_arch_vm_ioctl") returns the address of this 
variable from the patch module.

Miroslav

Re: Bug with paravirt ops and livepatches

[Chris J Arges]

On Wed, Apr 06, 2016 at 02:09:01PM +0200, Miroslav Benes wrote:
> On Wed, 6 Apr 2016, Chris J Arges wrote:
> 
> > I think this approach needs more thought and my code has bug(s).
> 
> And indeed there is...
> 
> long (*__kvm_arch_vm_ioctl)(struct file *filp, unsigned long ioctl, unsigned long arg) = NULL;
> 
> Use a different name than __kvm_arch_vm_ioctl and (ideally) make it 
> static.
> 
> kallsyms_lookup_name("__kvm_arch_vm_ioctl") returns the address of this 
> variable from the patch module.
> 
> Miroslav
>

Well that was the bug, I was really stumped why it was giving me a wierd
address for a function. Once I changed my pointer name to something else it
worked, so there was no difference to these approaches. I also had to modify
the symbol lookup to happen in the livepatch so we ensure that the module is
loaded in this case and not get a NULL deref.

The fixed code is here:
http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works.2/

This out of tree patch doesn't have the same failure as building a patch with
kpatch-build which is what we expect since it doesn't have livepatch relocs. In
addition I tested with the kvm module loaded _after_ the livepatch module and
no failure was observed.

--chris

Re: Bug with paravirt ops and livepatches

[Miroslav Benes]

On Wed, 6 Apr 2016, Chris J Arges wrote:

> On Wed, Apr 06, 2016 at 02:09:01PM +0200, Miroslav Benes wrote:
> > On Wed, 6 Apr 2016, Chris J Arges wrote:
> > 
> > > I think this approach needs more thought and my code has bug(s).
> > 
> > And indeed there is...
> > 
> > long (*__kvm_arch_vm_ioctl)(struct file *filp, unsigned long ioctl, unsigned long arg) = NULL;
> > 
> > Use a different name than __kvm_arch_vm_ioctl and (ideally) make it 
> > static.
> > 
> > kallsyms_lookup_name("__kvm_arch_vm_ioctl") returns the address of this 
> > variable from the patch module.
> > 
> > Miroslav
> >
> 
> Well that was the bug, I was really stumped why it was giving me a wierd
> address for a function. Once I changed my pointer name to something else it
> worked, so there was no difference to these approaches. I also had to modify
> the symbol lookup to happen in the livepatch so we ensure that the module is
> loaded in this case and not get a NULL deref.

Just a remark. With this change there is a call to kallsyms_lookup_name 
for each call to patched function. This is not optimal. What we do in 
kgraft is that we register a module notifier which calls 
kallsyms_lookup_name when to-be-patched module arrives. It is not nice but 
it works.

Miroslav

> 
> The fixed code is here:
> http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works.2/
> 
> This out of tree patch doesn't have the same failure as building a patch with
> kpatch-build which is what we expect since it doesn't have livepatch relocs. In
> addition I tested with the kvm module loaded _after_ the livepatch module and
> no failure was observed.
> 
> --chris
> 

Re: Bug with paravirt ops and livepatches

[Jessica Yu]

+++ Jessica Yu [05/04/16 15:19 -0400]:
>+++ Miroslav Benes [05/04/16 15:07 +0200]:
>>On Mon, 4 Apr 2016, Josh Poimboeuf wrote:
>>
>>>So I think this doesn't fix the problem.  Dynamic relocations are
>>>applied to the "patch module", whereas the above code deals with the
>>>initialization order of the "patched module".  This distinction
>>>originally confused me as well, until Jessica set me straight.
>>>
>>>Let me try to illustrate the problem with an example.  Imagine you have
>>>a patch module P which applies a patch to module M.  P replaces M's
>>>function F with a new function F', which uses paravirt ops.
>>>
>>>1) Patch P is loaded before module M.  P's new function F' has an
>>>   instruction which is patched by apply_paravirt(), even though the
>>>   patch hasn't been applied yet.
>>>
>>>2) Module M is loaded.  Before applying the patch, livepatch tries to
>>>   apply a klp_reloc to the instruction in F' which was already patched
>>>   by apply_paravirt() in step 1.  This results in undefined behavior
>>>   because it tries to patch the original instruction but instead
>>>   patches the new paravirt instruction.
>>>
>>>So the above patch makes no difference because the paravirt module
>>>loading order doesn't really matter.
>>
>>Hi,
>>
>>we are trying really hard to understand the actual culprit here and as it
>>is quite confusing I have several questions/comments...
>
>I don't have a 100% clear understanding of the whole picture either,
>but I'll try to help clarify up some things..
>
>>1. can you provide dynrela sections of the patch module from
>>https://github.com/dynup/kpatch/issues/580? What is interesting is that
>>kvm_arch_vm_ioctl() function contains rela records only for trivial (==
>>exported) symbols from the first look. The problem should be there only if
>>you want to patch a function which reference some paravirt_ops unexported
>>symbol. For that symbol dynrela should be created.
>
>Just to dispel some confusion over this, kpatch isn't "smart" enough
>yet to differentiate between exported and non-exported symbols, as
>Evgenii already mentioned. Just global and local, and whether the
>symbol belongs to a module or vmlinux. So that means dynrelas are
>indeed being created for the pv_*_ops symbols, despite the fact they
>are exported.

Gah, slight mistake, I should have mentioned that the above case only
applies to patches to modules (i.e. there is no mechanism to detect
exported symbols for patches to modules), but for vmlinux patches
there is (and those stay normal relas). I'm sorry if I confused
anybody. The relevant dynrela generation code is here if anyone's
interested:
https://github.com/dynup/kpatch/blob/master/kpatch-build/create-diff-object.c#L2661

Jessica

Re: Bug with paravirt ops and livepatches

[Jessica Yu]

+++ Miroslav Benes [06/04/16 10:43 +0200]:
>On Wed, 6 Apr 2016, Miroslav Benes wrote:
>
>> Anyway I see there are some new comments on github. I'll look at those.
>> But I'd prefer to discuss all the relevant things (that is kpatch
>> unspecific) here. It would make it easier.
>
>And you do (after seeing dates of the posts there), sorry for the noise.
>
>Jessica, I think I am perfectly fine with introducing some arch-specific
>code because of this problem.
>
>We used generic apply_relocate_add() because it was a single
>arch-independent entry point. There is no such things for paravirt_ops,
>alternatives, jump labels and such things. In fact only module_finalize()
>is there and that is not enough. So some arch-specific code in livepatch
>seems to be unnecessary.
>

Yeah, unfortunately that appears to be the case..Luckily I don't think
we need to add much code; the calls should be similar to the way we
call apply_relocate_add(), just pass in the right sections.

Jessica