Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760195AbcDEX1l (ORCPT ); Tue, 5 Apr 2016 19:27:41 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:48152 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752786AbcDEX1j (ORCPT ); Tue, 5 Apr 2016 19:27:39 -0400 Date: Wed, 6 Apr 2016 00:27:29 +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: <20160405232729.GA18198@canonical.com> References: <20160329120518.GA21252@canonical.com> <20160401190704.GB7837@canonical.com> <20160404161428.3qap2i4vpgda66iw@treble.redhat.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: 3420 Lines: 70 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. --chris