On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote:

Josh Poimboeuf [off-list ref] writes:

quoted

On Tue, Oct 31, 2017 at 07:39:59PM +0100, Torsten Duwe wrote:

quoted

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

quoted

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

That other discussion isn't relevant.  Even if we do eventually decide
to go with a source-based approach, that's still a long ways off.

OK, that was my thinking, but good to have it confirmed.

It depends. We can write and compile live patching modules right away. From
my point of view it's a matter of proceedingly automating this task.

quoted

For the foreseeable future, kpatch-build is the only available safe way
to create live patches.  We need to figure out a way to make it work,
one way or another.

As stated, I disagree here, but let's leave that aside, and stick to
your ( :-) problem.

quoted

If I understand correctly, the main problem here is that a call to a
previously-local-but-now-global function is missing a needed nop
instruction after the call, which is needed for restoring r2 (the TOC
pointer).

Yes, that's the root of the problem.

Yes.

quoted

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

It's fairly horrible. It might be *less* horrible if you generated an
assembly listing using the compiler, modified that, and then fed that
through the assembler and linker.

quoted

c) Create a new GCC flag which treats all calls as global, which can be
   used by kpatch-build to generate the right code (assuming this flag
   doesn't already exist).  This would be a good option, I think.

That's not impossible, but I doubt it will be all that popular with the
toolchain folks who'd have to implement it :)  It will also take a long
time to percolate out to users.

BTDT ;-)

quoted

d) Have kpatch-build do some other kind of transformation?  For example,
   maybe it could generate klp stubs which the callee calls into.  Each
   klp stub could then do a proper global call to the SHN_LIVEPATCH
   symbol.

That could work.

Indeed. There is no reason to load that off onto the kernel module loader.

quoted

Do I understand the problem correctly?  Do the potential solutions make
sense?  Any other possible solutions I missed?

Possibly, I'm not really across how kpatch works in detail and what the
constraints are.

One option would be to detect any local calls made by the patched
function and pull those in as well - ie. make them part of the patch.
The pathological case could obviously end up pulling in every function
in the kernel, but in practice that's probably unlikely. It already
happens to some extent anyway via inlining.

If modifying the source is an option, a sneaky solution is to mark the
local functions as weak, which means the compiler/linker has to assume
they could become global calls.

This might also be doable with a gcc "plugin", which would not require changes
to the compiler itself. OTOH there's no such thing as a weak static function...

	Torsten