[PATCH] Bugfix for VMI paravirt ops

[PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

I noticed this never got applied.  There was some feedback which I did 
not include in this patch because I think it is inappropriate to touch 
code outside vmi.c at this point for 2.6.21.  Please apply; this patch 
is needed as a bugfix in 2.6.21.  An updated version for 2.6.22 will 
come later which has a nicer interface.

Zach

[-- Attachment #2: vmi-lazy-mmu-fix.patch --]
[-- Type: text/plain, Size: 2356 bytes --]

Critical bugfix; when using software RAID, potentially USB or AIO in
highmem configurations, drivers are allowed to use kmap_atomic from
interrupt context.  This is incompatible with the current implementation
of lazy MMU mode, and means the kmap will silently fail, causing either
memory corruption or kernel panics.

The fix is to disable interrupts on the CPU when entering a lazy MMU
state; this is totally safe, as preemption is already disabled, and
lazy update state can neither be nested nor overlapping.  Thus per-cpu
variables to track the state and flags can be used to disable interrupts
during this critical region.

Signed-off-by: Zachary Amsden <zach@vmware.com>

Index: ubuntu-2.6.20/arch/i386/kernel/vmi.c
===================================================================
--- ubuntu-2.6.20.orig/arch/i386/kernel/vmi.c	2007-03-29 21:17:47.000000000 -0700
+++ ubuntu-2.6.20/arch/i386/kernel/vmi.c	2007-03-30 00:01:20.000000000 -0700
@@ -69,6 +69,7 @@
 	void (fastcall *flush_tlb)(int);
 	void (fastcall *set_initial_ap_state)(int, int);
 	void (fastcall *halt)(void);
+ 	void (fastcall *set_lazy_mode)(int mode);
 } vmi_ops;
  
 /* XXX move this to alternative.h */
@@ -577,6 +578,31 @@
 }
 #endif
 
+static void vmi_set_lazy_mode(int new_mode)
+{
+	static DEFINE_PER_CPU(int, mode);
+	static DEFINE_PER_CPU(unsigned long, flags);
+	int cpu = smp_processor_id();
+
+	if (!vmi_ops.set_lazy_mode)
+		return;
+
+	/*
+	 * Modes do not nest or overlap, so we can simply disable
+	 * irqs when entering a mode and re-enable when leaving.
+	 */
+	BUG_ON(per_cpu(mode, cpu) && new_mode);
+	BUG_ON(!new_mode && !per_cpu(mode, cpu));
+	
+	if (new_mode)
+		local_irq_save(per_cpu(flags, cpu));
+	else
+		local_irq_restore(per_cpu(flags, cpu));
+
+	vmi_ops.set_lazy_mode(new_mode);
+	per_cpu(mode, cpu) = new_mode;
+}
+
 static inline int __init check_vmi_rom(struct vrom_header *rom)
 {
 	struct pci_header *pci;
@@ -806,7 +832,7 @@
 	para_wrap(load_esp0, vmi_load_esp0, set_kernel_stack, UpdateKernelStack);
 	para_fill(set_iopl_mask, SetIOPLMask);
 	para_fill(io_delay, IODelay);
-	para_fill(set_lazy_mode, SetLazyMode);
+	para_wrap(set_lazy_mode, vmi_set_lazy_mode, set_lazy_mode, SetLazyMode);
 
 	/* user and kernel flush are just handled with different flags to FlushTLB */
 	para_wrap(flush_tlb_user, vmi_flush_tlb_user, flush_tlb, FlushTLB);

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

Zachary Amsden wrote:
> I noticed this never got applied.  There was some feedback which I did 
> not include in this patch because I think it is inappropriate to touch 
> code outside vmi.c at this point for 2.6.21.  Please apply; this patch 
> is needed as a bugfix in 2.6.21.  An updated version for 2.6.22 will 
> come later which has a nicer interface.
>

Erm, stale patch, sorry.  This one instead.



[-- Attachment #2: vmi-lazy-mmu-fix.patch --]
[-- Type: text/plain, Size: 2277 bytes --]

Critical bugfix; when using software RAID, potentially USB or AIO in
highmem configurations, drivers are allowed to use kmap_atomic from
interrupt context.  This is incompatible with the current implementation
of lazy MMU mode, and means the kmap will silently fail, causing either
memory corruption or kernel panics.

The fix is to disable interrupts on the CPU when entering a lazy MMU
state; this is totally safe, as preemption is already disabled, and
lazy update state can neither be nested nor overlapping.  Thus per-cpu
variables to track the state and flags can be used to disable interrupts
during this critical region.

Signed-off-by: Zachary Amsden <zach@vmware.com>

diff -r eee69881b2f9 arch/i386/kernel/vmi.c
--- a/arch/i386/kernel/vmi.c	Thu Apr 05 16:20:18 2007 -0700
+++ b/arch/i386/kernel/vmi.c	Thu Apr 05 16:31:12 2007 -0700
@@ -69,6 +69,7 @@ static struct {
 	void (*flush_tlb)(int);
 	void (*set_initial_ap_state)(int, int);
 	void (*halt)(void);
+  	void (*set_lazy_mode)(int mode);
 } vmi_ops;
 
 /*
@@ -545,6 +546,31 @@ vmi_startup_ipi_hook(int phys_apicid, un
 }
 #endif
 
+static void vmi_set_lazy_mode(int new_mode)
+{
+	static DEFINE_PER_CPU(int, mode);
+	static DEFINE_PER_CPU(unsigned long, flags);
+	int cpu = smp_processor_id();
+
+	if (!vmi_ops.set_lazy_mode)
+		return;
+
+	/*
+	 * Modes do not nest or overlap, so we can simply disable
+	 * irqs when entering a mode and re-enable when leaving.
+	 */
+	BUG_ON(per_cpu(mode, cpu) && new_mode);
+	BUG_ON(!new_mode && !per_cpu(mode, cpu));
+	
+	if (new_mode)
+		local_irq_save(per_cpu(flags, cpu));
+	else
+		local_irq_restore(per_cpu(flags, cpu));
+
+	vmi_ops.set_lazy_mode(new_mode);
+	per_cpu(mode, cpu) = new_mode;
+}
+
 static inline int __init check_vmi_rom(struct vrom_header *rom)
 {
 	struct pci_header *pci;
@@ -769,7 +795,7 @@ static inline int __init activate_vmi(vo
 	para_wrap(load_esp0, vmi_load_esp0, set_kernel_stack, UpdateKernelStack);
 	para_fill(set_iopl_mask, SetIOPLMask);
 	para_fill(io_delay, IODelay);
-	para_fill(set_lazy_mode, SetLazyMode);
+	para_wrap(set_lazy_mode, vmi_set_lazy_mode, set_lazy_mode, SetLazyMode);
 
 	/* user and kernel flush are just handled with different flags to FlushTLB */
 	para_wrap(flush_tlb_user, vmi_flush_tlb_user, flush_tlb, FlushTLB);

Re: [PATCH] Bugfix for VMI paravirt ops

[Andi Kleen]

On Friday 06 April 2007 01:29:56 Zachary Amsden wrote:
> I noticed this never got applied.  There was some feedback which I did 
> not include in this patch because I think it is inappropriate to touch 
> code outside vmi.c at this point for 2.6.21. 

I think it is. That is why i didn't apply it.

-Andi

Re: [PATCH] Bugfix for VMI paravirt ops

[Andrew Morton]

On Thu, 05 Apr 2007 16:34:43 -0700
Zachary Amsden <zach@vmware.com> wrote:

> > I noticed this never got applied.  There was some feedback which I did 
> > not include in this patch because I think it is inappropriate to touch 
> > code outside vmi.c at this point for 2.6.21.  Please apply; this patch 
> > is needed as a bugfix in 2.6.21.  An updated version for 2.6.22 will 
> > come later which has a nicer interface.
> >

There was a big foodfight last time you sent this out and I'd assumed that
there were still unresolved issues.  Or at least a general auru of
unhappiness.

I guess we merge it now, then (forget to) fix up those issues later on.

> Erm, stale patch, sorry.  This one instead.

yeah, that's the patch which has been in -mm for a week.

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Andi Kleen wrote:
> On Friday 06 April 2007 01:29:56 Zachary Amsden wrote:
>   
>> I noticed this never got applied.  There was some feedback which I did 
>> not include in this patch because I think it is inappropriate to touch 
>> code outside vmi.c at this point for 2.6.21. 
>>     
>
> I think it is. That is why i didn't apply it.
>   

Well at this point, the "proper" fix is dependent on Jeremy's 
kmap_atomic_pte changes, which are definitely too late to pull into 
2.6.21.  Can we just apply this patch please?

Thanks,

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Well at this point, the "proper" fix is dependent on Jeremy's
> kmap_atomic_pte changes, which are definitely too late to pull into
> 2.6.21.  Can we just apply this patch please? 

Hm, I think they're independent aren't they?  Your fix is about making
lazy_mmu disable interrupts; that's independent of how highpte pages get
mapped.

    J

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Well at this point, the "proper" fix is dependent on Jeremy's
>> kmap_atomic_pte changes, which are definitely too late to pull into
>> 2.6.21.  Can we just apply this patch please? 
>>     
>
> Hm, I think they're independent aren't they?  Your fix is about making
> lazy_mmu disable interrupts; that's independent of how highpte pages get
> mapped.

No, they are totally dependent.  The reason interrupts are disabled is 
to stop kmap_atomic in interrupt handlers.  With the kmap_atomic_pte 
changes, the whole interrupt disable jibberish goes away.

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> No, they are totally dependent.  The reason interrupts are disabled is
> to stop kmap_atomic in interrupt handlers.  With the kmap_atomic_pte
> changes, the whole interrupt disable jibberish goes away. 

But kmap_atomic_pte is a special case of kmap_atomic for ptes. 
Interrupt routines can still use plain kmap_atomic for bouncebuffers and
so on.

A more general patch would be to make kmap/unmap_atomic pv_ops, and then
they can all be rolled together.  I.e: check the type to see if special
pte handling needs to happen, etc.

    J

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> No, they are totally dependent.  The reason interrupts are disabled is
>> to stop kmap_atomic in interrupt handlers.  With the kmap_atomic_pte
>> changes, the whole interrupt disable jibberish goes away. 
>>     
>
> But kmap_atomic_pte is a special case of kmap_atomic for ptes. 
> Interrupt routines can still use plain kmap_atomic for bouncebuffers and
> so on.
>   

Ah, yes.

> A more general patch would be to make kmap/unmap_atomic pv_ops, and then
> they can all be rolled together.  I.e: check the type to see if special
> pte handling needs to happen, etc.
>   

So the clean fix for this is still even further out.  I don't think I 
want to hook kmap/unmap as paravirt-ops.

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> So the clean fix for this is still even further out.  I don't think I
> want to hook kmap/unmap as paravirt-ops.

Yes, it seems like overkill.

How about something like adding PARAVIRT_LAZY_FLUSH as an argument to
set_lazy_mode?  It would be valid to use at any time, and it would flush
any pending work while still remaining in whatever lazy mode its
currently in.  That way kmap_atomic can flush anything pending without
having to muck around with the current lazy state.

    J

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> So the clean fix for this is still even further out.  I don't think I
>> want to hook kmap/unmap as paravirt-ops.
>>     
>
> Yes, it seems like overkill.
>
> How about something like adding PARAVIRT_LAZY_FLUSH as an argument to
> set_lazy_mode?  It would be valid to use at any time, and it would flush
> any pending work while still remaining in whatever lazy mode its
> currently in.  That way kmap_atomic can flush anything pending without
> having to muck around with the current lazy state.
>   

Yes, thought about several solutions, and this seems the best.  But it 
requires a new paravirt-op.

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Yes, thought about several solutions, and this seems the best.  But it
> requires a new paravirt-op.

Not with the power of multiplexing.  Something like this, perhaps?

    J

diff -r 5be4a5ff8e6b arch/i386/mm/highmem.c
--- a/arch/i386/mm/highmem.c	Thu Apr 05 17:04:04 2007 -0700
+++ b/arch/i386/mm/highmem.c	Thu Apr 05 17:50:46 2007 -0700
@@ -42,6 +42,8 @@ void *kmap_atomic_prot(struct page *page
 
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 	set_pte(kmap_pte-idx, mk_pte(page, prot));
+
+	arch_flush_lazy_mmu_mode();
 
 	return (void*) vaddr;
 }
diff -r 5be4a5ff8e6b include/asm-generic/pgtable.h
--- a/include/asm-generic/pgtable.h	Thu Apr 05 17:04:04 2007 -0700
+++ b/include/asm-generic/pgtable.h	Thu Apr 05 17:50:46 2007 -0700
@@ -180,6 +180,7 @@ static inline void ptep_set_wrprotect(st
 #ifndef __HAVE_ARCH_ENTER_LAZY_MMU_MODE
 #define arch_enter_lazy_mmu_mode()	do {} while (0)
 #define arch_leave_lazy_mmu_mode()	do {} while (0)
+#define arch_flush_lazy_mmu_mode()	do {} while (0)
 #endif
 
 /*
@@ -193,6 +194,7 @@ static inline void ptep_set_wrprotect(st
 #ifndef __HAVE_ARCH_ENTER_LAZY_CPU_MODE
 #define arch_enter_lazy_cpu_mode()	do {} while (0)
 #define arch_leave_lazy_cpu_mode()	do {} while (0)
+#define arch_flush_lazy_cpu_mode()	do {} while (0)
 #endif
 
 /*
diff -r 5be4a5ff8e6b include/asm-i386/paravirt.h
--- a/include/asm-i386/paravirt.h	Thu Apr 05 17:04:04 2007 -0700
+++ b/include/asm-i386/paravirt.h	Thu Apr 05 17:50:46 2007 -0700
@@ -27,9 +27,10 @@ struct desc_struct;
 
 /* Lazy mode for batching updates / context switch */
 enum paravirt_lazy_mode {
-	PARAVIRT_LAZY_NONE = 0,
-	PARAVIRT_LAZY_MMU = 1,
-	PARAVIRT_LAZY_CPU = 2,
+	PARAVIRT_LAZY_NONE = 0,		/* exit lazy mode */
+	PARAVIRT_LAZY_MMU = 1,		/* lazy mmu updates */
+	PARAVIRT_LAZY_CPU = 2,		/* lazy cpu state updates */
+	PARAVIRT_LAZY_FLUSH = 3,	/* flush pending changes, if any */
 };
 
 struct paravirt_ops
@@ -1044,6 +1045,10 @@ static inline void arch_leave_lazy_cpu_m
 {
 	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_NONE);
 }
+static inline void arch_flush_lazy_cpu_mode(void)
+{
+	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_FLUSH);
+}
 
 #define  __HAVE_ARCH_ENTER_LAZY_MMU_MODE
 static inline void arch_enter_lazy_mmu_mode(void)
@@ -1053,6 +1058,10 @@ static inline void arch_leave_lazy_mmu_m
 static inline void arch_leave_lazy_mmu_mode(void)
 {
 	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_NONE);
+}
+static inline void arch_flush_lazy_mmu_mode(void)
+{
+	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_FLUSH);
 }
 
 void _paravirt_nop(void);

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Yes, thought about several solutions, and this seems the best.  But it
>> requires a new paravirt-op.
>>     
>
> Not with the power of multiplexing.  Something like this, perhaps?
>   

Throw it in the queue; I'll slide in after it.

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Yes, thought about several solutions, and this seems the best.  But it
>> requires a new paravirt-op.
>>     
>
> Not with the power of multiplexing.  Something like this, perhaps?
>   

Ok, I tried that and I got a nice clean fix.  For 2.6.22.  Backporting 
this to 2.6.21 creates havoc, as a number of cleanup patches as well as 
changes to highmem code get in the way.

Andi, do you really want to deal with the conflicts this will create for 
the paravirt queue for 2.6.22, or would you rather apply the dumb yet 
simple and non-confrontational workaround I have been trying to get 
applied to 2.6.21?

Zach

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Throw it in the queue; I'll slide in after it.

I've pushed it up.  I added a few missing cases to the patch
(kmap_atomic_pte, kunmap_atomic).

    J

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Throw it in the queue; I'll slide in after it.
>>     
>
> I've pushed it up.  I added a few missing cases to the patch
> (kmap_atomic_pte, kunmap_atomic).
>   

Do you mean kmap_atomic_pfn?  kunmap_atomic can stay lazy (at least for 
VMI), actually, but it doesn't help since it happens outside the spin lock.

Re: [PATCH] Bugfix for VMI paravirt ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Do you mean kmap_atomic_pfn?

Yes.

>   kunmap_atomic can stay lazy (at least for VMI), actually, but it
> doesn't help since it happens outside the spin lock.

May as well be consistent.  Or do you mean you can't flush outside the
spinlock, even if there's nothing pending?

    J

Re: [PATCH] Bugfix for VMI paravirt ops

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Do you mean kmap_atomic_pfn?
>>     
>
> Yes.
>
>   
>>   kunmap_atomic can stay lazy (at least for VMI), actually, but it
>> doesn't help since it happens outside the spin lock.
>>     
>
> May as well be consistent.  Or do you mean you can't flush outside the
> spinlock, even if there's nothing pending?
>   

Consistency is good.  Flush is always fine, just an extra function call.

Zach