Re: Bug with paravirt ops and livepatches

[Evgenii Shatokhin <eshatokhin@virtuozzo.com>]

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