Re: x86/paravirt: Detect over-sized patching bugs in paravirt_patch_call()

[Ingo Molnar <mingo@kernel.org>]

* 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

Which with the vanilla kernel crashes on boot much, much later:

[    2.478026] PANIC: double fault, error_code: 0x0

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?

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?

Thanks,

	Ingo