Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754327AbcDFKid (ORCPT ); Wed, 6 Apr 2016 06:38:33 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:54069 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbcDFKib (ORCPT ); Wed, 6 Apr 2016 06:38:31 -0400 Date: Wed, 6 Apr 2016 11:38:21 +0100 From: Chris J Arges To: Miroslav Benes Cc: Josh Poimboeuf , Jiri Kosina , 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 Message-ID: <20160406103821.GA4968@canonical.com> References: <20160401190704.GB7837@canonical.com> <20160404161428.3qap2i4vpgda66iw@treble.redhat.com> <20160405232729.GA18198@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5035 Lines: 101 On Wed, Apr 06, 2016 at 11:09:04AM +0200, Miroslav Benes wrote: > On Wed, 6 Apr 2016, Chris J Arges wrote: > > > 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. > > > > > > 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... > > > > > > > Miroslav, > > > > In my original comment I was trying to see if this was a kpatch-build specific > > issue and that's why I tried to reproduce using just a simple out of tree built > > livepatch module. For this case I copied kvm_arch_vm_ioctl into > > __kvm_arch_vm_ioctl (to simplify patching). I then built and installed this > > base kernel and kvm.ko module. However, for the crashing and non-crashing > > cases I used two slightly different base kernels and livepatch code. > > > > I just re-tested this using the latest livepatch.git/for-next code and have the > > following: > > > > This crashes if I use kallsyms_lookup_name to find the __kvm_arch_vm_ioctl > > symbol and then call it from my livepatch: > > http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.crashes/ > > > > This works if I export __kvm_arch_vm_ioctl in the base kernel, and in the > > livepatch just call it directly: > > http://people.canonical.com/~arges/livepatch_issue/livepatch_kvm_arch_vm_ioctl.works/ > > > > Where livepatch.c is the livepatch and kernel.patch is the base kernel patch for > > both directories. > > Thank you, Chris. > > First (before to reproduce it) I have to ask one thing. What is the order > of module loading? Do you first load kvm.ko and then livepatch or the > opposite. > I just tested this experimentally, in both cases kvm and kvm_intel was loaded before the livepatch module was inserted. The bug is triggered only when I actually start the VM and only when using my kallsyms_lookup_name hack. > It does not matter in the first case where the function is exported. > Because of it there would be a dependency of the modules so once you load > livepatch module kvm.ko is loaded before it. This is the only way to do > it. Undefined symbols of livepatch module are thus easily resolved. > > The situation differs in the second case though. You use > kallsyms_lookup_name to circumvent dynrela for __kvm_arch_vm_ioctl (it is > not exported now). So when you load livepatch module kallsyms_lookup_name > would in fact fail and the kernel would then crash. Although the bug would > be different (NULL pointer dereference) and I guess you are aware of that > because you have a printk there. But just to make sure... > > Thanks > Miroslav I think this approach needs more thought and my code has bug(s). --chris