Re: Fix issue with alternatives/paravirt patches

[Jessica Yu <jeyu@redhat.com>]

+++ Miroslav Benes [12/07/16 14:06 +0200]:
>On Tue, 5 Jul 2016, Jessica Yu wrote:
>
>> Hi,
>>
>> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
>> patching that was discussed here [1] and here [2]. To briefly summarize the
>> bug, patch modules that contained .altinstructions or .parainstructions
>> sections would break because these alternative/paravirt patches would be
>> applied first by the module loader (see x86 module_finalize()), then
>> livepatch would later clobber these patches when applying per-object
>> relocations. This lead to crashes and unpredictable behavior.
>>
>> One conclusion we reached from our last discussion was that we will
>> need to introduce some arch-specific code to address this problem.
>> This patchset presents a possible fix for the bug by adding a new
>> arch-specific arch_klp_init_object_loaded() function that by default
>> does nothing but can be overridden by different arches.
>>
>> To fix this issue for x86, since we can access a patch module's Elf
>> sections through mod->klp_info, we can simply delay the calls to
>> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
>> which is called after relocations have been written for an object.
>> In addition, for patch modules, .parainstructions and .altinstructions are
>> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
>> and livepatch can apply them manually.
>>
>> Currently for kpatch, we don't support including jump table sections in
>> the patch module, and supporting .smp_locks is currently broken, so we
>> don't consider those sections (for now).
>>
>> I did some light testing with some patches to kvm and verified that the
>> original issue reported in [2] was fixed.
>>
>> Based on linux-next.
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
>> [2] https://github.com/dynup/kpatch/issues/580
>>
>> Jessica Yu (2):
>>   livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
>>   livepatch/x86: apply alternatives and paravirt patches after relocations
>>
>>  arch/x86/kernel/Makefile    |  1 +
>>  arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/livepatch.h   |  3 +++
>>  kernel/livepatch/core.c     | 12 +++++++--
>>  4 files changed, 80 insertions(+), 2 deletions(-)
>>  create mode 100644 arch/x86/kernel/livepatch.c
>
>Hi,
>
>thanks Jessica for implementing it. It does not look as bad as I was
>afraid, which is great. Few remarks...
>
>Is there a problem when you need to generate a dynrela for paravirt code?
>I mean one does not know during the build of a patch module if paravirt
>would or would not be applied. If such code needs to be relocated it could
>be a problem for kpatch-build. Is this correct or am I missing something?

In kpatch-build, "special" sections like .parainstructions and
.altinstructions are into consideration. These sections are included
in the final patch module if they reference any of the new replacement
code. Like with any other section, kpatch-build will convert any relas
from .rela.parainstructions (for example) to dynrelas if it is
necessary.  And with this patch we apply all "dynrelas" (which are now
actual relas in .klp.rela sections) first before applying any
paravirt/alternatives patches in arch_klp_init_object_loaded(), which
is the correct order.

>Now the other architectures we support. s390 should be ok. There is
>nothing much in module_finalize() there. powerpc is different though. It
>is quite similar to x86_64 case. And also arm64 needs to be handled in
>future.

Yeah, I think we are OK for s390, arm64 looks fairly straightforward
(just a call to apply_alternatives()), powerpc looks slightly more
involved but I haven't thoroughly dug into it yet.

So other arches will need to have arch_klp_init_object_loaded()
implemented eventually (if needed), but I think it is fine for now to
fix this issue on x86 first, since that's where the original bug
cropped up.

Jessica