Re: [PATCH v4 15/16] module: Move where we mark modules RO,X

[Peter Zijlstra]

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 ?