Message-ID: <alpine.LNX.2.00.1604061103390.27016@pobox.suse.cz>
Date:	Wed, 6 Apr 2016 11:09:04 +0200 (CEST)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Chris J Arges <chris.j.arges@...onical.com>
cc:	Josh Poimboeuf <jpoimboe@...hat.com>,
	Jiri Kosina <jikos@...nel.org>, jeyu@...hat.com,
	eugene.shatokhin@...alab.ru, live-patching@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	pmladek@...e.cz
Subject: Re: Bug with paravirt ops and livepatches

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

Powered by blists - more mailing lists