From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758405AbcDENHT (ORCPT ); Tue, 5 Apr 2016 09:07:19 -0400 Received: from mx2.suse.de ([195.135.220.15]:42768 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751576AbcDENHR (ORCPT ); Tue, 5 Apr 2016 09:07:17 -0400 Date: Tue, 5 Apr 2016 15:07:13 +0200 (CEST) From: Miroslav Benes To: Josh Poimboeuf cc: Jiri Kosina , Chris J Arges , jeyu@redhat.com, eugene.shatokhin@rosalab.ru, live-patching@vger.kernel.org, Linux Kernel Mailing List , pmladek@suse.cz Subject: Re: Bug with paravirt ops and livepatches In-Reply-To: <20160404161428.3qap2i4vpgda66iw@treble.redhat.com> Message-ID: References: <20160329120518.GA21252@canonical.com> <20160401190704.GB7837@canonical.com> <20160404161428.3qap2i4vpgda66iw@treble.redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >