LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31  8:45 [PATCH] VMI paravirt-ops bugfix for 2.6.21 Zachary Amsden
@ 2007-03-31  8:02 ` Andrew Morton
  2007-03-31  8:11   ` Jeremy Fitzhardinge
  2007-03-31  8:11 ` Jeremy Fitzhardinge
  2007-03-31 14:35 ` Andi Kleen
  2 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2007-03-31  8:02 UTC (permalink / raw)
  To: Zachary Amsden
  Cc: Andi Kleen, Linus Torvalds, Jeremy Fitzhardinge,
	Linux Kernel Mailing List

On Sat, 31 Mar 2007 00:45:59 -0800 Zachary Amsden <zach@vmware.com> wrote:

> +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();

That will cause upset if CONFIG_PREEMPT=y?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31  8:45 [PATCH] VMI paravirt-ops bugfix for 2.6.21 Zachary Amsden
  2007-03-31  8:02 ` Andrew Morton
@ 2007-03-31  8:11 ` Jeremy Fitzhardinge
  2007-03-31  9:19   ` Zachary Amsden
  2007-03-31 14:35 ` Andi Kleen
  2 siblings, 1 reply; 7+ messages in thread
From: Jeremy Fitzhardinge @ 2007-03-31  8:11 UTC (permalink / raw)
  To: Zachary Amsden
  Cc: Andrew Morton, Andi Kleen, Linus Torvalds, Linux Kernel Mailing List

Zachary Amsden wrote:
> 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.  This bug is only visible with >970
> megs of RAM and extreme memory pressure, but nontheless extremely serious.
>   

Why's that?  Don't some things get preferentially allocated out of highmem?

> 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 be8c61492e28 arch/i386/kernel/vmi.c
> --- a/arch/i386/kernel/vmi.c	Fri Mar 30 14:13:45 2007 -0700
> +++ b/arch/i386/kernel/vmi.c	Fri Mar 30 14:18:16 2007 -0700
> @@ -69,6 +69,7 @@ struct {
>  	void (*flush_tlb)(int);
>  	void (*set_initial_ap_state)(int, int);
>  	void (*halt)(void);
> + 	void (*set_lazy_mode)(int mode);
>  } vmi_ops;
>  
>  /* XXX move this to alternative.h */
> @@ -574,6 +575,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));
>   

It's probably better to use __get_cpu_var() rather than per_cpu(X,
smp_processor_id());  It's a real pity there's no ^^ operator...

> +	
> +	if (new_mode)
> +		local_irq_save(per_cpu(flags, cpu));
> +	else
> +		local_irq_restore(per_cpu(flags, cpu));
>   

The comment only talks about disabling interrupts for lazy_mmu, but this
seems to do it for lazy_cpu as well.  Is that OK?  What happens if
someone wants to change interrupt states under lazy_cpu; I can't think
of an inherent reason why that wouldn't be allowed (though I don't think
it happens now).

This kind of logic is a bit clunky anyway; would it be better to simply
have separate enable/disable functions?  Or at least separate functions
per mode?

Anyway,

Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>

    J

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31  8:02 ` Andrew Morton
@ 2007-03-31  8:11   ` Jeremy Fitzhardinge
  0 siblings, 0 replies; 7+ messages in thread
From: Jeremy Fitzhardinge @ 2007-03-31  8:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Zachary Amsden, Andi Kleen, Linus Torvalds, Linux Kernel Mailing List

Andrew Morton wrote:
> On Sat, 31 Mar 2007 00:45:59 -0800 Zachary Amsden <zach@vmware.com> wrote:
>
>   
>> +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();
>>     
>
> That will cause upset if CONFIG_PREEMPT=y?
>   

It must be called under the pagetable lock, so preempt should already be
disabled.

    J

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] VMI paravirt-ops bugfix for 2.6.21
@ 2007-03-31  8:45 Zachary Amsden
  2007-03-31  8:02 ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Zachary Amsden @ 2007-03-31  8:45 UTC (permalink / raw)
  To: Andrew Morton, Andi Kleen, Linus Torvalds, Jeremy Fitzhardinge,
	Linux Kernel Mailing List

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

So lazy MMU mode is vulnerable to interrupts coming in and issuing 
kmap_atomic, which does not work when under lazy MMU mode.  The window 
for this is small, but it means highmem kernels, especially with heavy 
network, USB, or AIO workloads are vulnerable to getting invariably 
fatal pagefaults in interrupt handlers.  For now, the best fix is to 
simply disable and re-enable interrupts when entering and exiting lazy 
mode (which, btw, is already guaranteed to have preempt disabled).  For 
the future, a better fix is to simply exit lazy mode when issuing 
kmap_atomic, but I do not want to touch any generic code now for 2.6.21.

Hopefully there is still time to apply it.  Thanks to Jeremy 
Fitzhardinge for pointing this out.

Zach

[-- Attachment #2: vmi-lazy-mmu-fix.patch --]
[-- Type: text/plain, Size: 2414 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.  This bug is only visible with >970
megs of RAM and extreme memory pressure, but nontheless extremely serious.

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 be8c61492e28 arch/i386/kernel/vmi.c
--- a/arch/i386/kernel/vmi.c	Fri Mar 30 14:13:45 2007 -0700
+++ b/arch/i386/kernel/vmi.c	Fri Mar 30 14:18:16 2007 -0700
@@ -69,6 +69,7 @@ struct {
 	void (*flush_tlb)(int);
 	void (*set_initial_ap_state)(int, int);
 	void (*halt)(void);
+ 	void (*set_lazy_mode)(int mode);
 } vmi_ops;
 
 /* XXX move this to alternative.h */
@@ -574,6 +575,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;
@@ -804,7 +830,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);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31  8:11 ` Jeremy Fitzhardinge
@ 2007-03-31  9:19   ` Zachary Amsden
  0 siblings, 0 replies; 7+ messages in thread
From: Zachary Amsden @ 2007-03-31  9:19 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Andrew Morton, Andi Kleen, Linus Torvalds, Linux Kernel Mailing List

Jeremy Fitzhardinge wrote:
>
> The comment only talks about disabling interrupts for lazy_mmu, but this
> seems to do it for lazy_cpu as well.  Is that OK?  What happens if
> someone wants to change interrupt states under lazy_cpu; I can't think
> of an inherent reason why that wouldn't be allowed (though I don't think
> it happens now).
>   

Well, lazy cpu is used only for context switch.  Changing interrupt 
states won't happen there.

> This kind of logic is a bit clunky anyway; would it be better to simply
> have separate enable/disable functions?  Or at least separate functions
> per mode?
>   

I want to do a cleaner fix for 2.6.22; this is pretty clunky, agree.  
But it is still better to have fewer paravirt-ops.  Perhaps lazy_enter / 
flush would be more semantically useful.

Zach

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31  8:45 [PATCH] VMI paravirt-ops bugfix for 2.6.21 Zachary Amsden
  2007-03-31  8:02 ` Andrew Morton
  2007-03-31  8:11 ` Jeremy Fitzhardinge
@ 2007-03-31 14:35 ` Andi Kleen
  2007-04-01  7:01   ` Zachary Amsden
  2 siblings, 1 reply; 7+ messages in thread
From: Andi Kleen @ 2007-03-31 14:35 UTC (permalink / raw)
  To: Zachary Amsden
  Cc: Andrew Morton, Linus Torvalds, Jeremy Fitzhardinge,
	Linux Kernel Mailing List

On Saturday 31 March 2007 10:45, Zachary Amsden wrote:
> So lazy MMU mode is vulnerable to interrupts coming in and issuing 
> kmap_atomic, which does not work when under lazy MMU mode.  The window 
> for this is small, but it means highmem kernels, especially with heavy 
> network, USB, or AIO workloads are vulnerable to getting invariably 
> fatal pagefaults in interrupt handlers.  For now, the best fix is to 
> simply disable and re-enable interrupts when entering and exiting lazy 
> mode (which, btw, is already guaranteed to have preempt disabled).  For 
> the future, a better fix is to simply exit lazy mode when issuing 
> kmap_atomic, but I do not want to touch any generic code now for 2.6.21.

I think I would prefer you touch the generic code. This new hook is ugly.

And the lazy mode is currently only used by VMI anyways, isn't it? So you shouldn't 
impact anybody else

-Andi


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21
  2007-03-31 14:35 ` Andi Kleen
@ 2007-04-01  7:01   ` Zachary Amsden
  0 siblings, 0 replies; 7+ messages in thread
From: Zachary Amsden @ 2007-04-01  7:01 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Andrew Morton, Linus Torvalds, Jeremy Fitzhardinge,
	Linux Kernel Mailing List

Andi Kleen wrote:
> I think I would prefer you touch the generic code. This new hook is ugly.
>   

What new hook?  The hook has been there for sometime, and now we are 
using it to fix a bug.  I do not want to touch generic or arch code at 
this point in the 2.6.21 release.

> And the lazy mode is currently only used by VMI anyways, isn't it? So you shouldn't 
> impact anybody else
>   

Yes, but if I don't touch arch code, I won't even affect native.  I do 
not want to risk any instability, despite how easy it would be to do 
what you and others have asked, I don't think it is appropriate at -rc5 
stage.

Thanks,
Zach

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-04-01  6:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-03-31  8:45 [PATCH] VMI paravirt-ops bugfix for 2.6.21 Zachary Amsden
2007-03-31  8:02 ` Andrew Morton
2007-03-31  8:11   ` Jeremy Fitzhardinge
2007-03-31  8:11 ` Jeremy Fitzhardinge
2007-03-31  9:19   ` Zachary Amsden
2007-03-31 14:35 ` Andi Kleen
2007-04-01  7:01   ` Zachary Amsden

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).