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

[Josh Poimboeuf]

On Thu, Oct 19, 2017 at 04:27:31PM +0200, Miroslav Benes wrote:
> On Thu, 19 Oct 2017, Josh Poimboeuf wrote:
> > > 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.

Ah right, I forgot about that improvement to the patches.  But wouldn't
the binary diff tool still have to do the manual KLP_MODULE_RELOC
annotation when it's needed?  If so, then I still don't see the benefit
of the extra conversion step.

> > > 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.

Sounds nice, though I wonder what the obstacles are?

> You could verify the result and its correctness.

Does that mean it's easier to do code review?  Or something else?

> It could also be beneficial if we'd like to pursue automatic
> verification in the future.

What do you mean by automatic verification?

> > > > 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'm very confused by this.  You're the expert, so please set me straight :-)

I think you're talking about functions whose symbols have been renamed
by GCC and have been given an optimized suffix, like ".isra" or
".constprop", right?  Wasn't one of your main points at Plumbers last
year that not all such function-ABI-breaking optimizations result in a
rename of the symbol?

> 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.

My main objection to merging klp-convert in its current state is that
it's not useful by itself.  In fact, it's actively dangerous if people
assume that because it's in-tree, it's the definitive way to safely
create patches.

I have a similar worry about the livepatch-sample module.  It's also
actively dangerous.  Its only decent justification for being in-tree,
IMO, is that we at least need some type of in-tree user of the klp
interfaces.

(But maybe we could solve that by converting livepatch-sample to
selftests.  That would also have another benefit of giving us some
automated test coverage.)

klp-convert is a vast improvement to the livepatch-sample module, but I
view that as a bad thing because it makes it a lot easier to do
something stupid ;-)

If it were part of a complete solution, with some supporting tooling
and/or documentation which prevent the user from making dumb mistakes,
then I think it would make sense to merge it.

Anyway I appreciate all the research efforts you guys are doing.  All
the different options sound promising.

-- 
Josh