Messages in this thread | | | Date | Thu, 25 Apr 2019 13:40:04 +0200 | From | Peter Zijlstra <> | Subject | Re: x86/paravirt: Detect over-sized patching bugs in paravirt_patch_call() |
| |
On Thu, Apr 25, 2019 at 12:57:45PM +0200, Ingo Molnar wrote: > * Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Apr 25, 2019 at 11:50:39AM +0200, Ingo Molnar wrote: > > > * Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Thu, Apr 25, 2019 at 11:17:17AM +0200, Ingo Molnar wrote: > > > > > It basically means that we silently won't do any patching and the kernel > > > > > will crash later on in mysterious ways, because paravirt patching is > > > > > usually relied on. > > > > > > > > That's OK. The compiler emits an indirect CALL/JMP to the pv_ops > > > > structure contents. That _should_ stay valid and function correctly at > > > > all times. > > > > > > It might result in a correctly executing kernel in terms of code > > > generation, but it doesn't result in a viable kernel: some of the places > > > rely on the patching going through and don't know what to do when it > > > doesn't and misbehave or crash in interesting ways. > > > > > > Guess how I know this. ;-) > > > > What sites would that be? It really should work AFAIK. > > So for example I tried to increasing the size of one of the struct > patch_xxl members: > > --- a/arch/x86/kernel/paravirt_patch.c > +++ b/arch/x86/kernel/paravirt_patch.c > @@ -28,7 +28,7 @@ struct patch_xxl { > const unsigned char irq_restore_fl[2]; > # ifdef CONFIG_X86_64 > const unsigned char cpu_wbinvd[2]; > - const unsigned char cpu_usergs_sysret64[6]; > + const unsigned char cpu_usergs_sysret64[60]; > const unsigned char cpu_swapgs[3]; > const unsigned char mov64[3]; > # else
So this then fails to patch the immediate; but the compiler emitted:
175: ff 25 00 00 00 00 jmpq *0x0(%rip) # 17b <syscall_return_via_sysret+0x75> 177: R_X86_64_PC32 pv_ops+0xfc
and pv_ops+0xfc is (+4 because of reloc magic):
void (*usergs_sysret64)(void); /* 0x100 0x8 */
which defaults to:
arch/x86/kernel/paravirt.c: .cpu.usergs_sysret64 = native_usergs_sysret64,
which in turn reads like:
0000000000000000 <native_usergs_sysret64>: 0: 0f 01 f8 swapgs 3: 48 0f 07 sysretq
So I _really_ don't understand how:
> Which with the vanilla kernel crashes on boot much, much later: > > [ 2.478026] PANIC: double fault, error_code: 0x0
happens.
> But in any case, even if many of the others will work if the patching > fails silently, is there any case where we'd treat patching failure as an > acceptable case?
It really should just work. And we need to figure out why it comes unstuck. Can you print the code when it fails patching?
> BUG_ON() in paravirt kernels is an easily debuggable condition and beats > the above kinds of symptoms. But I can turn it into a WARN_ON_ONCE() if > you think that's better?
Not patching should be a performance issue; not a correctness issue, as per the above. So WARN is the right thing.
| |