Re: 2.6.26-git: NULL pointer deref in __switch_to

From: Rusty Russell
Date: Wed Jun 18 2008 - 01:35:21 EST


On Wednesday 18 June 2008 09:50:22 Suresh Siddha wrote:
> On Mon, Jun 16, 2008 at 02:21:23PM -0700, Simon Holm Thøgersen wrote:
> > > Can you please upload it some where? I will also try with another guest
> > > image meanwhile.
> >
> > [access provided to Suresh in private email]
>
> Simon, Thanks.
>
> Simon, Patrick, I am able to reproduce the oops in __switch_to()
> with lguest. My debug showed that there is atleast one lguest specific
> issue (which should be present in 2.6.25 and before aswell) and it got
> exposed with a kernel oops with the recent fpu dynamic allocation patches.
>
> In addition to the previous possible scenario (with fpu_counter), in the
> presence of lguest, it is possible that the cpu's TS bit it still set and
> the lguest launcher task's thread_info has TS_USEDFPU still set.
>
> This is because of the way the lguest launcher handling the guest's TS bit.
> (look at lguest_set_ts() in lguest_arch_run_guest()). This can result
> in a DNA fault while doing unlazy_fpu() in __switch_to(). This will
> end up causing a DNA fault in the context of new process thats
> getting context switched in (as opossed to handling DNA fault in the
> context of lguest launcher/helper process).
>
> This is wrong in both pre and post 2.6.25 kernels. In the recent
> 2.6.26-rc series, this is showing up as NULL pointer dereferences or
> sleeping function called from atomic context(__switch_to()), as
> we free and dynamically allocate the FPU context for the newly
> created threads. Older kernels might show some FPU corruption for processes
> running inside of lguest.
>
> With the appended patch, my test system is running for more than 50 mins
> now. So atleast some of your oops (hopefully all!) should get fixed.
> Please give it a try. I will spend more time with this fix tomorrow.
>
> Apart from the last hunk(MSR_IA32_SYSENTER_CS changes), I believe
> the below patch is needed for 2.6.25 aswell.
>
> Thanks.
>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@xxxxxxxxx>
> ---
>
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 6d54833..e2db9ac 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -333,6 +333,7 @@ void flush_thread(void)
> /*
> * Forget coprocessor state..
> */
> + tsk->fpu_counter = 0;
> clear_fpu(tsk);
> clear_used_math();
> }
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index ac54ff5..c6eb5c9 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -294,6 +294,7 @@ void flush_thread(void)
> /*
> * Forget coprocessor state..
> */
> + tsk->fpu_counter = 0;
> clear_fpu(tsk);
> clear_used_math();
> }
> diff --git a/drivers/lguest/x86/core.c b/drivers/lguest/x86/core.c
> index 5126d5d..4a98404 100644
> --- a/drivers/lguest/x86/core.c
> +++ b/drivers/lguest/x86/core.c
> @@ -176,7 +176,7 @@ void lguest_arch_run_guest(struct lg_cpu *cpu)
> * we set it now, so we can trap and pass that trap to the Guest if it
> * uses the FPU. */
> if (cpu->ts)
> - lguest_set_ts();
> + unlazy_fpu(current);
>
> /* SYSENTER is an optimized way of doing system calls. We can't allow
> * it because it always jumps to privilege level 0. A normal Guest
> @@ -196,6 +196,10 @@ void lguest_arch_run_guest(struct lg_cpu *cpu)
> * trap made the switcher code come back, and an error code which some
> * traps set. */
>
> + /* Restore SYSENTER if it's supposed to be on. */
> + if (boot_cpu_has(X86_FEATURE_SEP))
> + wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
> +
> /* If the Guest page faulted, then the cr2 register will tell us the
> * bad virtual address. We have to grab this now, because once we
> * re-enable interrupts an interrupt could fault and thus overwrite
> @@ -203,13 +207,12 @@ void lguest_arch_run_guest(struct lg_cpu *cpu)
> if (cpu->regs->trapnum == 14)
> cpu->arch.last_pagefault = read_cr2();
> /* Similarly, if we took a trap because the Guest used the FPU,
> - * we have to restore the FPU it expects to see. */
> + * we have to restore the FPU it expects to see.
> + * math_state_restore() may sleep and we may even move off to
> + * a different CPU. So all the critical stuff should be done
> + * before this. */
> else if (cpu->regs->trapnum == 7)
> math_state_restore();

Hi Suresh,

Firstly, thanks for figuring this out. But math_state_restore() has nasty
semantics now. Currently lguest will work, because no code path following
this call relies on being on the same CPU.

So, this patch is fine, but I wonder if I should just be forcing fpu
allocation earlier for lguest tasks, so I can avoid this altogether?

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/