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

[Miroslav Benes]

On Thu, 19 Oct 2017, Josh Poimboeuf wrote:

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

In a way, yes. It depends on the tool. I always pictured it as a pipeline 
(similar to toolchain) and klp-convert as one of its blocks.
 
> > > > 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?

Those GCC optimizations you mentioned below and which I didn't connect to 
klp-convert itself. More on that later.

Nothing serious aside from that, I hope. Nicolai is currently implementing 
C parser for kernel sources.
 
> > You could verify the result and its correctness.
> 
> Does that mean it's easier to do code review?  Or something else?

Yes, the code review.

> > It could also be beneficial if we'd like to pursue automatic
> > verification in the future.
> 
> What do you mean by automatic verification?

Formal verification. Theoretically we could have a formal specification of 
our consistency model and we could prove/disprove whether a livepatch and 
its implementation are correct with respect to it. It is a vague idea 
though and I personally haven't got sufficient knowledge to do anything 
about it.

> > > > > 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 :-)

Oh, am I now? :)

> 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 misunderstood your question then. Yes, I was talking about changed 
symbol names, while you asked about something I had in a different block 
of "pipeline".

So yes, it is still a problem, which can be easily solved by asm/obj-based 
approach and not so easily by source-based approach.

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

Well, you could use this reasoning even for kernel livepatching codebase 
itself. It is hard to use it right, but it is there and thus dangerous.

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

Yes, that would be great.

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

Right, so this is where our views differ a bit. I'd like to get to the 
finish line (whatever that means) slowly but steady and not to wait for 
the ultimate solution if it can be implemented step by step. 

I think it is time for others to express their opinions. We should talk 
about it next week at OSS.

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

Yeah, I think it is important to try as much as possible.

Thanks,
Miroslav