First, thanks for taking the time to look into this issue! I can confirm that both patches fixed the panic and I'm able to run a VM without any issues.
I still have some concerns. I need to preface this by noting that I am not entirely familiar with the paravirtualization code in the kernel, so someone who is more familiar should weigh in and let me know if my concerns are valid or not.
Since dynrelas are written after module load as opposed to the expected order, which is to 1) apply all relocations to a function first before 2) applying paravirt and alternative patches (i.e. the order is
kernel/module.c), I am not sure if we are breaking any expectations here. My main concern is leaving
apply_paravirt() to patch a function without all its relas written in first (which becomes the case when we skip dynrelas).
A paravirt patch site does in fact refer to the old instruction that is expected to be there, which may be "incomplete" if there are still relas (including dynrelas) to be written (i.e., I'm referring to the address of where the old instruction is expected to be in the
.text, in this case, the text of the function
See the struct definition of
paravirt_patch_site and the field
instr: http://lxr.free-electrons.com/source/arch/x86/include/asm/paravirt_types.h#L674. Actually, I'm not even sure if the old instruction even ever gets used, because it looks like the instruction buffer in
apply_paravirt() just gets overwritten in the end anyway.
So, even though in this case these patches work, I'm just worried we'll be shooting ourselves in the foot later on. IMHO, I think the least risky solution is to avoid hard coding edge cases and to apply both relocations and paravirt patches in the original expected order.
Yep, so my hack fix was to simply have livepatch finish writing its relas before calling
apply_paravirt(), which is easy to do with the arch independent patchset since it allows a module to access all its Elf section information. I guess the ugly part of that solution is the irony of introducing more arch code (x86 specific in this case) which we were trying to avoid in the first place :P. I think we should bring this up on the livepatch mailing list, since we'll get more opinions there.
tl;dr, My main concern is the possible consequences (if at all any...) of letting
apply_paravirt patch a function that may possibly not have all its relas and dynrelas written in beforehand. I haven't been able to think of concrete problems yet, so perhaps I'm being overly paranoid.