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

[Miroslav Benes]

On Thu, 19 Oct 2017, Josh Poimboeuf wrote:

> On Thu, Oct 19, 2017 at 03:24:51PM +0200, Miroslav Benes wrote:
> > On Thu, 19 Oct 2017, Josh Poimboeuf wrote:
> > 
> > > On Wed, Oct 11, 2017 at 09:42:09AM -0300, Joao Moreira wrote:
> > > > > 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.
> > > 
> > > Instead of having multiple approaches, I'd strongly prefer that we
> > > converge on a single in-tree approach that works for everybody.
> > > 
> > > (Whether that will be source-based like klp-convert or binary-based like
> > > kpatch-build, I don't know.)
> > 
> > I think that klp-convert can work with both. Even with non-source-based 
> > solution you need something to generate those relocation records. I 
> > consider klp-convert as a part of the building pipeline.
> 
> Hm.  If I understand correctly, the binary diff tool (or some tool in
> the pipeline) would create the .klp.module_relocs.* section, and then
> klp-convert would convert that to the .klp.sym.* and .klp.rela.*
> sections which livepatch needs.
> 
> But if the original tool is creating a relocation section, can't it
> instead just create the livepatch .klp.* sections directly?  What's the
> benefit of the extra conversion step?

I haven't seen this patch set for a while (which is embarassing), but 
klp-convert tries to generate needed sections automatically without 
.klp.module_relocs.* section. Only when there is an ambiguity which cannot 
be solved automatically, manual annotation (KLP_MODULE_RELOC) is needed. 
In that case klp-convert provides hints what needs to be done.

> > > BTW, what is bin-diff?  Have you seen kpatch-build?
> > 
> > I'm speaking for Joao here, but we discussed this personally and I think 
> > he meant approach based on asmtool 
> > (https://github.com/joergroedel/asmtool). We'd like to explore as much as 
> > possible.
> 
> Ok, I'd be interested in seeing that, and also what its benefits are
> compared to kpatch-build.
> 
> > We also considered complete source-based solution. Nicolai Stange works on 
> > that (or at least on something which would make it possible).
> 
> What is a complete source-based solution?  Is it just "klp-convert +
> some GCC optimization strategy" or is it something more?

There's more. You'd give the tool a fix (patch, diff) and kernel sources, 
and it would automatically generate a source code of its livepatch. If 
possible (and there are some obstacles), there would be an advantage 
compared to kpatch-build or different asm/obj-based solution. You could 
verify the result and its correctness. It could also be beneficial if we'd 
like to pursue automatic verification in the future.

> > We can decide what to have in upstream afterwards. But I still think that 
> > klp-convert will be part of it in some form. Am I missing something?
> 
> I guess it really depends on what the solution looks like.  If we
> decided on kpatch-build (or some variation of its tooling) then we might
> not need klp-convert.

That's possible.

> > > > 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?
> > > 
> > > IMO, klp-convert will only be useful if we have a realistic strategy for
> > > dealing with GCC optimizations.  So I'd say we should follow through on
> > > that with the compiler folks before spending too much more time on it.
> > 
> > Yes, I'm all for a solution on GCC side, but that may take a while and 
> > even then it is still a huge step to get it into a distribution (we have 
> > GCC 4.8.5 in SLE12 :)).
> > 
> > However, there is an easy temporary solution. You can add all 
> > referenced optimized functions to a livepatch and let klp-convert process 
> > the rest.
> 
> How do you find all referenced optimized functions?

I guess that since there is no connection between a symbol and its 
optimized counterpart, klp-convert warns about this.

Joao, is this correct?

I understand your position and I agree that klp-convert may become 
superfluous in the future. Maybe not. And maybe the future is far away. 
Anyway, it looks useful in its current form and it would help tremendously 
at least here at SUSE, which is the reason Joao worked on it and send it 
upstream.

Miroslav