Re: Bug with paravirt ops and livepatches

[Miroslav Benes <mbenes@suse.cz>]

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...

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
>