Re: Bug with paravirt ops and livepatches

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

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