Re: Bug with paravirt ops and livepatches

[Chris J Arges <chris.j.arges@canonical.com>]

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