Re: [PATCH 0/8] livepatch: klp-convert tool

[Joao Moreira]

On 10/10/2017 11:46 PM, Josh Poimboeuf wrote:
> On Tue, Oct 10, 2017 at 04:17:10PM +0200, Miroslav Benes wrote:
> > On Wed, 30 Aug 2017, Josh Poimboeuf wrote:
> >
> > > On Tue, Aug 29, 2017 at 04:01:32PM -0300, Joao Moreira wrote:
> > > > Livepatches may use symbols which are not contained in its own scope,
> > > > and, because of that, may end up compiled with relocations that will
> > > > only be resolved during module load. Yet, when the referenced symbols are
> > > > not exported, solving this relocation requires information on the object
> > > > that holds the symbol (either vmlinux or modules) and its position inside
> > > > the object, as an object may contain multiple symbols with the same name.
> > > > Providing such information must be done accordingly to what is specified
> > > > in Documentation/livepatch/module-elf-format.txt.
> > > >
> > > > Currently, there is no trivial way to embed the required information as
> > > > requested in the final livepatch elf object. klp-convert solves this
> > > > problem in two different forms: (i) by relying on a symbol map, which is
> > > > built during kernel compilation, to automatically infers the relocation
> > > > targeted symbol, and, when such inference is not possible (ii) by using
> > > > annotations in the elf object to convert the relocation accordingly to
> > > > the specification, enabling it to be handled by the livepatch loader.
> > > >
> > > > Given the above, add support for symbol mapping in the form of
> > > > Symbols.list file; add klp-convert tool; integrate klp-convert tool into
> > > > kbuild; make livepatch modules discernible during kernel compilation
> > > > pipeline; add data-structure and macros to enable users to annotate
> > > > livepatch source code; make modpost stage compatible with livepatches;
> > > > update livepatch-sample and update documentation.
> > > >
> > > > The patch was tested under three use-cases:
> > > >
> > > > use-case 1: There is a relocation in the lp that can be automatically
> > > > resolved by klp-convert (tested by removing the annotations from
> > > > samples/livepatch/livepatch-annotated-sample.c)
> > > >
> > > > use-case 2: There is a relocation in the lp that cannot be automatically
> > > > resolved, as the name of the respective symbol appears in multiple
> > > > objects. The livepatch contains an annotation to enable a correct
> > > > relocation - reproducible with this livepatch sample:
> > > > www.livewire.com.br/suse/klp/livepatch-sample.1.c
> > > >
> > > > use-case 3: There is a relocation in the lp that cannot be automatically
> > > > resolved similarly as 2, but no annotation was provided in the livepatch,
> > > > triggering an error during compilation - reproducible with this livepatch
> > > > sample: www.livewire.com.br/suse/klp/livepatch-sample.2.c
> > > >
> > > > Joao Moreira (2):
> > > >    kbuild: Support for Symbols.list creation
> > > >    documentation: Update on livepatch elf format
> > > >
> > > > Josh Poimboeuf (5):
> > > >    livepatch: Create and include UAPI headers
> > > >    livepatch: Add klp-convert tool
> > > >    livepatch: Add klp-convert annotation helpers
> > > >    modpost: Integrate klp-convert
> > > >    livepatch: Add sample livepatch module
> > > >
> > > > Miroslav Benes (1):
> > > >    modpost: Add modinfo flag to livepatch modules
> > >
> > > Thanks a lot for picking these patches up and improving them.  I've only
> > > glanced at the code, but so far it's looking good.  It may be a few
> > > weeks before a I get a chance to do a proper review.
> > >
> > > One quick question, possibly for Miroslav.  Do we have a plan yet for
> > > dealing with GCC optimizations?
> > >
> > >    https://lkml.kernel.org/r/20161110161053.heua3abuaekz4yy7@treble
> > >
> > > I still like the '-fpreserve-function-abi' idea, but maybe it's not
> > > realistic.
> >
> > I'm sorry for the late response, I failed to reply immediately and then
> > completely forgot about it :(
>
> No worries...
>
> > I talked to Martin Jambor from gcc community month ago and he told me it
> > could be possible to do. But we need to come up with a good proposal. We
> > need a good description of what it should do and provide reasons why we
> > need it. I'll talk to him again tomorrow and I'll start to work on the
> > proposal.
>
> Sounds good!  For klp-convert to be successful, we really need a
> strategy for dealing with such optimizations.  I'm thinking that a
> '-fpreserve-function-abi' flag would be the cleanest way to handle it.
>
> If we don't have a strategy for dealing with optimizations, then we may
> instead need to go with a binary diff-based tool like kpatch-build.

I'm currently looking into binary diff-based solutions to deal with this 
problem. My plan is to submit a second patch set once I have it 
functional and land both things (klp-convert and bin-diff) in two 
different steps.

Is there any issue with following this schedule? Meaning, do you guys 
still plan on reviewing this patch set or do you prefer me to do 
something differently in terms of approach?

> > Ideas are of course more than welcome.
> >
> > However that might be the easier part. We need to find out what it would
> > mean for the whole kernel and its performance.
>
> TBH, I'd be surprised if it affected kernel performance in any
> measurable way.  The vast majority of non-inlined functions seem to
> conform to function ABI.