On Fri, 3 May 2019, Joe Lawrence wrote:
> On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote:
> >
> > [ ... snip ... ]
> >
> > Quick look, but it seems quite similar to the problem we had with
> > apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which
> > introduced it.
>
> That was an interesting diversion :) I think I grok the idea as:
>
> The kernel supports a few different code-patching methods:
>
> - SMP locks
> - alternatives
> - paravirt
> - jump labels
>
> and we need to ensure that they do not prematurely operate on unresolved
> klp-relocations. The solution that led to arch/x86/kernel/livepatch.c
> introduces "klp.arch" sections that rename such klp-relocations *and*
> their associated special section data structures. Processing is then
> deferred until after a relevant klp_object is loaded.
Correct.
> > I think, we should do the same for jump labels. Add
> > jump_label_apply_nops() from module_finalize() to
> > arch_klp_init_object_loaded() and convert jump_table ELF section so its
> > processing is delayed.
>
> Nod. Tthat sounds about right. There may be some more work yet in the
> static keys API as well, but I'm not 100%.
>
> > Which leads me another TODO... klp-convert does not convert even
> > .altinstructions and .parainstructions sections, so it has that problem as
> > well. If I remember, it was on Josh's TODO list when he first introduced
> > klp-convert. See cover.1477578530.git.jpoimboe@xxxxxxxxxx.
>
> In the RFC, Josh highlights a somewhat difficult problem regarding these
> special sections -- how to associate these special section data
> structures and their relocations to a specific klp_object.
>
> If I understand his suggestion, he proposed annotating livepatch module
> replacement functions as to stuff them into specially named ELF sections
> (which would include the klp_object name) and then bypass the existing
> livepatch registration API. No minor change.
>
> With that in mind, I'm starting to think of a game plan for klp-convert
> like:
>
> - phase 1: detect /abort unsupported sections
>
> - phase 2: manual annotations in livepatch modules (like
> KLP_MODULE_RELOC / SYMPOS, but for special sections) so
> that klp-convert can start building "klp.arch" sections
>
> - phase 3: livepatch API change above to support somewhat more
> automatic generation of phase 2 annotations
Looks good to me. First, I'd focus on something which covers (hopefully) a
vast majority cases and then we can do the rest. So yes, this seems to be
a good plan.
> > And of course we should look at the other supported architectures and
> > their module_finalize() functions. I have it on my TODO list somewhere,
> > but you know how it works with those :/. I am sure there are more hidden
> > surprises there.
>
> Hmm, well powerpc and s390 do appear to have processing for special
> sections as well ... but for the moment, I'm going to focus on x86 as
> that seems like enough work for now :)
Yes, please :).
> > > Detection
> > > ---------
> > >
> > > I can post ("livepatch/klp-convert: abort on static key conversion")
> > > here as a follow commit if it looks reasonable and folks wish to review
> > > it... or we can try and tackle static keys before merging klp-convert.
> >
> > Good idea. I'd rather fix it, but I think it could be a lot of work, so
> > something like this patch seems to be a good idea.
>
> I'm thinking of adding this in a commit so klp-convert can intercept
> these sections:
>
> static bool is_section_supported(char *sname)
> {
> if (strcmp(sname, ".rela.altinstructions") == 0)
> return false;
> if (strcmp(sname, ".rela.parainstructions") == 0)
> return false;
> if (strcmp(sname, ".rela__jump_table") == 0)
> return false;
> return true;
> }
>
> Right now my v4 collection has a bunch of small fixups and nitpick
> corrections. It feels like a good resting place for now before
> embarking on special section support, what do you think?
Yes.
Thanks
Miroslav
