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