From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751908AbcGMBSv (ORCPT ); Tue, 12 Jul 2016 21:18:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58693 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751084AbcGMBSm (ORCPT ); Tue, 12 Jul 2016 21:18:42 -0400 Date: Tue, 12 Jul 2016 21:18:14 -0400 From: Jessica Yu To: Josh Poimboeuf Cc: Miroslav Benes , Petr Mladek , Jiri Kosina , Chris J Arges , Eugene Shatokhin , live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: Fix issue with alternatives/paravirt patches Message-ID: <20160713011744.GB30925@packer-debian-8-amd64.digitalocean.com> References: <1467772500-26092-1-git-send-email-jeyu@redhat.com> <20160707155633.GC3556@pathway.suse.cz> <20160707225323.dj5zwu4bh7mlxew3@treble> <20160712140120.hiwhudmxld5oapcn@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160712140120.hiwhudmxld5oapcn@treble> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 13 Jul 2016 01:18:21 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Josh Poimboeuf [12/07/16 09:01 -0500]: >On Tue, Jul 12, 2016 at 01:55:54PM +0200, Miroslav Benes wrote: >> On Thu, 7 Jul 2016, Josh Poimboeuf wrote: >> >> > On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote: >> > > On Tue 2016-07-05 22:34:58, Jessica Yu wrote: >> > > > Hi, >> > > > >> > > > A few months ago, Chris Arges reported a bug involving alternatives/paravirt >> > > > patching that was discussed here [1] and here [2]. To briefly summarize the >> > > > bug, patch modules that contained .altinstructions or .parainstructions >> > > > sections would break because these alternative/paravirt patches would be >> > > > applied first by the module loader (see x86 module_finalize()), then >> > > > livepatch would later clobber these patches when applying per-object >> > > > relocations. This lead to crashes and unpredictable behavior. >> > > > >> > > > One conclusion we reached from our last discussion was that we will >> > > > need to introduce some arch-specific code to address this problem. >> > > > This patchset presents a possible fix for the bug by adding a new >> > > > arch-specific arch_klp_init_object_loaded() function that by default >> > > > does nothing but can be overridden by different arches. >> > > > >> > > > To fix this issue for x86, since we can access a patch module's Elf >> > > > sections through mod->klp_info, we can simply delay the calls to >> > > > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(), >> > > > which is called after relocations have been written for an object. >> > > > In addition, for patch modules, .parainstructions and .altinstructions are >> > > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them >> > > > and livepatch can apply them manually. >> > > >> > > The solution looks correct to me. The fun will be how to generate >> > > the sections. If I get this correctly, it is not enough to rename >> > > the existing ones. Instead, we need to split .parainstructions >> > > and .altinstructions sections into per-object ones. >> > > >> > > I wonder if there is a plan for this. Especially I am interested >> > > into the patches created from sources ;-) I wonder if we could add >> > > a tag somewhere and improve the build infrastructure. >> > >> > Yeah. I'd like to reiterate[1] that this would all be a lot easier if >> > we weren't circumventing module dependencies. >> > >> > [1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda66iw@treble.redhat.com >> >> Oh, we haven't come to any conclusion. I think it would be a great topic >> for Plumbers conf. It is always better to discuss such things personally. >> What do you think? Any volunteer to propose it? :) > >Well, it's somewhat related to my "Livepatch module creation tooling" >proposed talk, because I suspect the tooling could be *much* simpler if >we didn't circumvent module dependencies. So I'll probably talk about >that aspect of it. > >But it would be great if somebody wanted to submit a separate talk to >explore the pros and cons of our current "load patches to modules before >the modules themselves have been loaded" approach and if there are any >viable alternatives. In addition to Josh's linked discussion, we also once talked about the idea of forcing module dependencies (exactly!) one year ago today: http://www.spinics.net/lists/live-patching/msg00946.html I've forgotten a lot of what I was blabbering about back then (and this was way before we talked about the .klp.rela stuff), but I do remember we talked a bit about forcing module dependencies and potentially forcing to-be-patched modules to be loaded first before loading the patch module. I still don't think we should do that but instead we could potentially implement something more palatable like enforcing maybe one patch module per object (so maybe depmod could record dependencies for us) or something like that. It would be interesting to revisit the problem again, esp. in the context of recent changes. If I end up collecting enough talking points, I can submit a proposal. Jessica