Re: Bug with paravirt ops and livepatches

[Evgenii Shatokhin]

05.04.2016 16:07, Miroslav Benes ÐÐÑÐÑ:
> 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.

As far as I understand it, create-diff-object does not check if a symbol 
is exported or not when a patch for a module rather than for vmlinux is 
being prepared.

In such cases, it only checks if the referenced global symbol is defined 
somewhere in that module and if not, creates a dynrela for it.

The code is in kpatch_create_dynamic_rela_sections() from 
https://github.com/dynup/kpatch/blob/master/kpatch-build/create-diff-object.c.

> 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...
>
> 3. Now I'll try to explain what really happens here... because of 1. and
> the way the alternatives and relocations are implemented the only
> problematic case is when one wants to patch a module which introduces its
> own alternatives. Which is probably the case of KVM. Why?
>
> When alternative is used, the call to some pv_*_ops.function is placed
> somewhere. The address of this location is stored in a separate elf
> section and when apply_paravirt() is called it takes the address and
> place the new code there (or it does not, it depends :)). When the
> alternative is in some module and pv_*_ops is exported, which is the
> usual case, there is no problem. No dynrela needs to be used when a user
> wants to patch such function with the alternative.
>
> The only problem is when the function uses unexported pv_*_ops (or some
> other alternative symbol) from its own object file. When the user wants to
> patch this one there is a need for dynrela.
>
> So what really happens is that we load a patch module, we do not apply
> our relocations but we do apply alternatives to the patch module, which is
> problematic because there is a reference to something which is not yet
> resolved (that is unexported pv_*_ops). When a patched module arrives we
> happily resolve relocations but since we do not apply alternatives again
> there is a rubbish in the patching code.
>
> Is this all correct?
>
> > Jessica proposed some novel fixes here:
> >
> >     https://github.com/dynup/kpatch/issues/580#issuecomment-183001652
>
> Yes, I think that fix should be the same we have for relocations. To
> postpone the alternatives applications. Maybe it is even possible to do it
> in a similar way. And yes on one hand it is gonna be ugly, on the other
> hand it is gonna be consistent with relocations.
>
> > I think the *real* problem here (and one that we've seen before) is that
> > we have a feature which allows you to load a patch to a module before
> > loading the module itself.  That really goes against the grain of how
> > module dependencies work.  It has already given us several headaches and
> > it makes the livepatch code a lot more complex.
> >
> > I really think we need to take another hard look about whether it's
> > really worth it.  My current feeling is that it's not.
>
> I can only say that maybe about 1/3 of kgraft patches we currently have
> are for modules (1/3 is probably not correct but it is definitely
> non-trivial number). It would be really unfortunate to load all the
> to-be-patched modules when a patch module is applied.
>
> This does not mean that we cannot solve it in another way as you propose
> below. I have to think about it.
>
> Miroslav
>
> > If we were able to get rid of that "feature", yes, the livepatch code
> > would be simpler, but there might be another awesome benefit: I suspect
> > we'd also be able to get rid of the need for specialized patch creation
> > tooling like kpatch-build.  Instead I think we could just specify
> > klp_relocs info in the source code of the patch, and just use kbuild to
> > build the patch module.  Not only would the livepatch code be simpler
> > (and much easier to wrap your head around), but the user space tooling
> > could be *vastly* simpler.
> >
> > Of course, removing that feature might create some headaches for the
> > user.  It is nice to be able to load a big cumulative patch without
> > having to load all the dependencies first.  But maybe there are things
> > we could do to make the dependency problem more manageable.  e.g.,
> > splitting up patch modules to be per-object?  requiring the user to load
> > modules they don't need?  patching or replacing the module on disk?
> > copying the new module to a new locaiton and telling modprobe where to
> > find it?
> >
> > I don't have all the answers but I think we should take a hard look at
> > some of these other approaches.
> >
> > --
> > Josh

Regards,
Evgenii