mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Bart Oldeman <bartoldeman@users.sourceforge.net>
To: linux-kernel@vger.kernel.org
Cc: heukelum@fastmail.fm, Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
	x86@kernel.org
Subject: [PATCH] x86, vm86: fix preemption bug for int1 debug and int3 breakpoint handlers.
Date: Thu, 23 Sep 2010 09:51:21 -0400 (EDT)	[thread overview]
Message-ID: <alpine.DEB.2.00.1009230949480.8587@localhost.localdomain> (raw)

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 |   14 +++++---------
  2 files changed, 6 insertions(+), 9 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..ad0830d 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -350,16 +350,12 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
  	/* we never return here */
  }

-static inline void return_to_32bit(struct kernel_vm86_regs *regs16, int retval)
+static inline void return_to_32bit(struct kernel_vm86_regs *regs, int retval)
  {
-	struct pt_regs *regs32;
-
-	regs32 = save_v86_state(regs16);
-	regs32->ax = retval;
-	__asm__ __volatile__("movl %0,%%esp\n\t"
-		"movl %1,%%ebp\n\t"
-		"jmp resume_userspace"
-		: : "r" (regs32), "r" (current_thread_info()));
+	KVM86->regs32->ax = retval;
+	/* 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);
  }

  static inline void set_IF(struct kernel_vm86_regs *regs)

             reply	other threads:[~2010-09-23 13:51 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-09-23 13:51 Bart Oldeman [this message]
2010-09-23 17:16 ` Bart Oldeman
2010-09-24  0:28   ` [tip:x86/vm86] x86, vm86: Fix " tip-bot for Bart Oldeman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.00.1009230949480.8587@localhost.localdomain \
    --to=bartoldeman@users.sourceforge.net \
    --cc=heukelum@fastmail.fm \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®