lkml.org 
[lkml]   [2016]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Bug with paravirt ops and livepatches
    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

    \
     
     \ /
      Last update: 2016-04-06 01:41    [W:5.300 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site