Re: Bug with paravirt ops and livepatches

[Josh Poimboeuf]

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.

Agreed, kpatch-build could be smarter here.  However the problem still
exists for non-exported symbols, at least in theory.

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

That all sounds mostly right to me, except it sounds like you're
conflating alternatives with paravirt patching, when in fact they're two
separate mechanisms.  Paravirt patches are specific to virtualization,
but alternatives can be used on both virt and HW.

And then there's also jump labels.

In any case I don't think we should get too bogged down into details.
Even it's not currently possible to hit this bug today, it could easily
come back to bite us later.

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

Yeah.  If we made it consistent with how we do relocations in Jessica's
arch-independent patches, it looks like apply_paravirt() and
apply_alternatives() already take section data as input, so we might be
able to call them as-is from livepatch code.  The jump label code is
different and would probably need some rework for us to be able to reuse
it.

So maybe the kernel code wouldn't be *too* bad.  But it does add
complexity and it would also further complicate our patch creation tools
as well.

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

Yeah, it's a complicated issue and I need to think about it some more as
well.

> 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