On Mon, Oct 21, 2019 at 04:14:02PM +0200, Peter Zijlstra wrote: > On Mon, Oct 21, 2019 at 08:53:12AM -0500, Josh Poimboeuf wrote:
> > Doesn't livepatch code also need to be modified? We have: > > Urgh bah.. I was too focussed on the other klp borkage :/ But yes, > arm64/ftrace and klp are the only two users of that function (outside of > module.c) and Mark was already writing a patch for arm64. > > Means these last two patches need to wait a little until we've fixed > those. So the other KLP issue: <mbenes> peterz: ad klp, apply_relocate_add() and text_poke()... what about apply_alternatives() and apply_paravirt()? They call text_poke_early(), which was ok with module_disable/enable_ro(), but now it is not, I suppose. See arch_klp_init_object_loaded() in arch/x86/kernel/livepatch.c. <peterz> mbenes: hurm, I don't see why we would need to do apply_alternatives() / apply_paravirt() later, why can't we do that the moment we load the module <peterz> mbenes: that is, those things _never_ change after boot <mbenes> peterz: as jpoimboe explained. See commit d4c3e6e1b193497da3a2ce495fb1db0243e41e37 for detailed explanation. Now sadly that commit missed all the useful information, luckily I could find the patch in my LKML folder, more sad, that thread still didn't contain the actual useful information, for that I was directed to github: https://github.com/dynup/kpatch/issues/580 Now, someone is owning me a beer for having to look at github for this. That finally explained that what happens is that the RELA was trying to fix up the paravirt indirect call to 'local_irq_disable', which apply_paravirt() will have overwritten with 'CLI; NOP'. This then obviously goes *bang*. This then raises a number of questions: 1) why is that RELA (that obviously does not depend on any module) applied so late? 2) why can't we unconditionally skip RELA's to paravirt sites? 3) Is there ever a possible module-dependent RELA to a paravirt / alternative site? Now, for 1), I would propose '.klp.rela.${mod}' sections only contain RELAs that depend on symbols in ${mod} (or modules in general). We can fix up RELAs that depend on core kernel early without problems. Let them be in the normal .rela sections and be fixed up on loading the patch-module as per usual. This should also deal with 2, paravirt should always have RELAs into the core kernel. Then for 3) we only have alternatives left, and I _think_ it unlikely to be the case, but I'll have to have a hard look at that. Hmmm ?