Message-ID: <20160712140120.hiwhudmxld5oapcn@treble>
Date:	Tue, 12 Jul 2016 09:01:20 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Petr Mladek <pmladek@...e.com>, Jessica Yu <jeyu@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Chris J Arges <chris.j.arges@...onical.com>,
	Eugene Shatokhin <eugene.shatokhin@...alab.ru>,
	live-patching@...r.kernel.org, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

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.

-- 
Josh

Powered by blists - more mailing lists