lkml.org 
[lkml]   [2010]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] x86, vm86: fix preemption bug for int1 debug and int3 breakpoint handlers.
    Below is a more minimal and more correct version of the patch I sent 
    earlier today. Changing return_to_32bit() to not JMP is desirable but
    needs more careful checking of return statements in vm86_32.c.

    Impact: fix kernel bug such as:
    BUG: scheduling while atomic: dosemu.bin/19680/0x00000004
    See also Ubuntu bug 455067 at
    https://bugs.launchpad.net/ubuntu/+source/linux/+bug/455067

    Commits 4915a35e35a037254550a2ba9f367a812bc37d40
    ("Use preempt_conditional_sti/cli in do_int3, like on x86_64.")
    and 3d2a71a596bd9c761c8487a2178e95f8a61da083
    ("x86, traps: converge do_debug handlers")
    started disabling preemption in int1 and int3 handlers on i386.
    The problem with vm86 is that the call to handle_vm86_trap() may jump
    straight to entry_32.S and never returns so preempt is never enabled
    again, and there is an imbalance in the preempt count.

    Commit be716615fe596ee117292dc615e95f707fb67fd1 ("x86, vm86:
    fix preemption bug"), which was later (accidentally?) reverted by commit
    08d68323d1f0c34452e614263b212ca556dae47f ("hw-breakpoints: modifying
    generic debug exception to use thread-specific debug registers")
    fixed the problem for debug exceptions but not for breakpoints.

    There are three solutions to this problem.

    1. Reenable preemption before calling handle_vm86_trap(). This
    was the approach that was later reverted.

    2. Do not disable preemption for i386 in breakpoint and debug handlers.
    This was the situation before October 2008. As far as I understand
    preemption only needs to be disabled on x86_64 because a seperate stack is
    used, but it's nice to have things work the same way on
    i386 and x86_64.

    3. Let handle_vm86_trap() return instead of jumping to assembly code.
    By setting a flag in _TIF_WORK_MASK, either TIF_IRET or TIF_NOTIFY_RESUME,
    the code in entry_32.S is instructed to return to 32 bit mode from
    V86 mode. The logic in entry_32.S was already present to handle signals.
    (I chose TIF_IRET because it's slightly more efficient in
    do_notify_resume() in signal.c, but in fact TIF_IRET can probably be
    replaced by TIF_NOTIFY_RESUME everywhere.)

    I'm submitting approach 3, because I believe it is the most elegant
    and prevents future confusion. Still, an obvious
    preempt_conditional_cli(regs);
    is necessary in traps.c to correct the bug.

    Signed-off-by: Bart Oldeman <bartoldeman@users.sourceforge.net>

    arch/x86/kernel/traps.c | 1 +
    arch/x86/kernel/vm86_32.c | 10 ++++++++--
    2 files changed, 9 insertions(+), 2 deletions(-)

    diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
    index 725ef4d..4d0f3ed 100644
    --- a/arch/x86/kernel/traps.c
    +++ b/arch/x86/kernel/traps.c
    @@ -568,6 +568,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
    if (regs->flags & X86_VM_MASK) {
    handle_vm86_trap((struct kernel_vm86_regs *) regs,
    error_code, 1);
    + preempt_conditional_cli(regs);
    return;
    }

    diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
    index 5ffb562..61fb985 100644
    --- a/arch/x86/kernel/vm86_32.c
    +++ b/arch/x86/kernel/vm86_32.c
    @@ -551,8 +551,14 @@ cannot_handle:
    int handle_vm86_trap(struct kernel_vm86_regs *regs, long error_code, int trapno)
    {
    if (VMPI.is_vm86pus) {
    - if ((trapno == 3) || (trapno == 1))
    - return_to_32bit(regs, VM86_TRAP + (trapno << 8));
    + if ((trapno == 3) || (trapno == 1)) {
    + KVM86->regs32->ax = VM86_TRAP + (trapno << 8);
    + /* setting this flag forces the code in entry_32.S to
    + call save_v86_state() and change the stack pointer
    + to KVM86->regs32 */
    + set_thread_flag(TIF_IRET);
    + return 0;
    + }
    do_int(regs, trapno, (unsigned char __user *) (regs->pt.ss << 4), SP(regs));
    return 0;
    }

    \
     
     \ /
      Last update: 2010-09-27 20:09    [W:4.023 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site