On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote: > Josh Poimboeuf <jpoimboe@xxxxxxxxxx> writes: > > > On Tue, Oct 31, 2017 at 07:39:59PM +0100, Torsten Duwe wrote: > >> On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote: > >> > On 2017/10/31 03:30PM, Torsten Duwe wrote: > >> > > > >> > > Maybe I failed to express my views properly; I find the whole approach > >> [...] > >> > > NAK'd-by: Torsten Duwe <duwe@xxxxxxx> > >> > > >> > 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@xxxxxxxxxxxxxxxx/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. > > 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. > > 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. > > 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. > > > 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 ;-) > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
