On Tue, Nov 07, 2017 at 12:31:05PM +0100, Torsten Duwe wrote: > On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote: > > > 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. Ah, that would work a lot better. > > > 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 ;-) Yeah, maybe that's not so realistic. > > > 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. I agree that doing the same work in tooling would be better than adding complexity to the kernel. > > > 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. That's definitely a possibility, but I'd rather not go there because it increases risk and cognitive load. > > 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. That could work, but it means the patch author has to modify the patch. I'd rather have tooling hide this problem somehow. > This might also be doable with a gcc "plugin", which would not require changes > to the compiler itself. Hm, that's not a bad idea. > OTOH there's no such thing as a weak static function... Yeah. Instead of weak, maybe just make them global (remove the "static")? Anyway, thanks for the ideas. I may try them out and see what sticks. -- Josh -- 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
