Messages in this thread | | | Subject | Re: Bug with paravirt ops and livepatches | From | Evgenii Shatokhin <> | Date | Tue, 5 Apr 2016 16:53:49 +0300 |
| |
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
| |