LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86: prevent unconditional writes to DebugCtl MSR
@ 2008-03-10 13:11 Jan Beulich
  2008-03-10 15:56 ` Andi Kleen
  2008-03-10 17:14 ` Ingo Molnar
  0 siblings, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2008-03-10 13:11 UTC (permalink / raw)
  To: mingo, tglx, hpa; +Cc: markus.t.metzger, linux-kernel

Otherwise, enabling (or better, subsequent disabling) of single
stepping would cause a kernel oops on CPUs not having this MSR.

The patch could have been added a conditional to the MSR write in
user_disable_single_step(), but centralizing the updates seems safer
and (looking forward) better manageable.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Markus Metzger <markus.t.metzger@intel.com>

---
 arch/x86/kernel/kprobes.c    |    4 ++--
 arch/x86/kernel/process_32.c |    4 ++--
 arch/x86/kernel/process_64.c |    4 ++--
 arch/x86/kernel/step.c       |    2 +-
 include/asm-x86/processor.h  |    9 +++++++++
 5 files changed, 16 insertions(+), 7 deletions(-)

--- linux-2.6.25-rc5/arch/x86/kernel/kprobes.c	2008-03-10 13:24:10.000000000 +0100
+++ 2.6.25-rc5-x86-debugctlmsr/arch/x86/kernel/kprobes.c	2008-03-06 11:02:51.000000000 +0100
@@ -410,13 +410,13 @@ static void __kprobes set_current_kprobe
 static void __kprobes clear_btf(void)
 {
 	if (test_thread_flag(TIF_DEBUGCTLMSR))
-		wrmsrl(MSR_IA32_DEBUGCTLMSR, 0);
+		update_debugctlmsr(0);
 }
 
 static void __kprobes restore_btf(void)
 {
 	if (test_thread_flag(TIF_DEBUGCTLMSR))
-		wrmsrl(MSR_IA32_DEBUGCTLMSR, current->thread.debugctlmsr);
+		update_debugctlmsr(current->thread.debugctlmsr);
 }
 
 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
--- linux-2.6.25-rc5/arch/x86/kernel/process_32.c	2008-03-10 13:24:10.000000000 +0100
+++ 2.6.25-rc5-x86-debugctlmsr/arch/x86/kernel/process_32.c	2008-03-06 11:02:51.000000000 +0100
@@ -575,12 +575,12 @@ __switch_to_xtra(struct task_struct *pre
 		/* we clear debugctl to make sure DS
 		 * is not in use when we change it */
 		debugctl = 0;
-		wrmsrl(MSR_IA32_DEBUGCTLMSR, 0);
+		update_debugctlmsr(0);
 		wrmsr(MSR_IA32_DS_AREA, next->ds_area_msr, 0);
 	}
 
 	if (next->debugctlmsr != debugctl)
-		wrmsr(MSR_IA32_DEBUGCTLMSR, next->debugctlmsr, 0);
+		update_debugctlmsr(next->debugctlmsr);
 
 	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
 		set_debugreg(next->debugreg0, 0);
--- linux-2.6.25-rc5/arch/x86/kernel/process_64.c	2008-03-10 13:24:10.000000000 +0100
+++ 2.6.25-rc5-x86-debugctlmsr/arch/x86/kernel/process_64.c	2008-03-06 11:02:51.000000000 +0100
@@ -573,12 +573,12 @@ static inline void __switch_to_xtra(stru
 		/* we clear debugctl to make sure DS
 		 * is not in use when we change it */
 		debugctl = 0;
-		wrmsrl(MSR_IA32_DEBUGCTLMSR, 0);
+		update_debugctlmsr(0);
 		wrmsrl(MSR_IA32_DS_AREA, next->ds_area_msr);
 	}
 
 	if (next->debugctlmsr != debugctl)
-		wrmsrl(MSR_IA32_DEBUGCTLMSR, next->debugctlmsr);
+		update_debugctlmsr(next->debugctlmsr);
 
 	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
 		loaddebug(next, 0);
--- linux-2.6.25-rc5/arch/x86/kernel/step.c	2008-03-10 13:24:10.000000000 +0100
+++ 2.6.25-rc5-x86-debugctlmsr/arch/x86/kernel/step.c	2008-03-06 11:02:51.000000000 +0100
@@ -145,7 +145,7 @@ static void write_debugctlmsr(struct tas
 	if (child != current)
 		return;
 
-	wrmsrl(MSR_IA32_DEBUGCTLMSR, val);
+	update_debugctlmsr(val);
 }
 
 /*
--- linux-2.6.25-rc5/include/asm-x86/processor.h	2008-03-10 13:24:33.000000000 +0100
+++ 2.6.25-rc5-x86-debugctlmsr/include/asm-x86/processor.h	2008-03-06 11:02:51.000000000 +0100
@@ -662,6 +662,15 @@ extern void switch_to_new_gdt(void);
 extern void cpu_init(void);
 extern void init_gdt(int cpu);
 
+static inline void update_debugctlmsr(unsigned long debugctlmsr)
+{
+#ifndef CONFIG_X86_DEBUGCTLMSR
+	if (boot_cpu_data.x86 < 6)
+		return;
+#endif
+	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);
+}
+
 /* from system description table in BIOS.  Mostly for MCA use, but
  * others may find it useful. */
 extern unsigned int machine_id;




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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
  2008-03-10 13:11 [PATCH] x86: prevent unconditional writes to DebugCtl MSR Jan Beulich
@ 2008-03-10 15:56 ` Andi Kleen
  2008-03-10 17:14 ` Ingo Molnar
  1 sibling, 0 replies; 7+ messages in thread
From: Andi Kleen @ 2008-03-10 15:56 UTC (permalink / raw)
  To: Jan Beulich; +Cc: mingo, tglx, hpa, markus.t.metzger, linux-kernel

"Jan Beulich" <jbeulich@novell.com> writes:
>  
> +static inline void update_debugctlmsr(unsigned long debugctlmsr)
> +{
> +#ifndef CONFIG_X86_DEBUGCTLMSR
> +	if (boot_cpu_data.x86 < 6)
> +		return;
> +#endif
> +	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctlmsr);

It would be better to use checking_wrmsrl() instead of the model
check because often VMs etc. are missing such MSRs too, but 
report the higher family number.

Also silently failing is not very nice, but that's a different issue.

-Andi

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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
  2008-03-10 13:11 [PATCH] x86: prevent unconditional writes to DebugCtl MSR Jan Beulich
  2008-03-10 15:56 ` Andi Kleen
@ 2008-03-10 17:14 ` Ingo Molnar
  1 sibling, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2008-03-10 17:14 UTC (permalink / raw)
  To: Jan Beulich; +Cc: tglx, hpa, markus.t.metzger, linux-kernel


* Jan Beulich <jbeulich@novell.com> wrote:

> Otherwise, enabling (or better, subsequent disabling) of single 
> stepping would cause a kernel oops on CPUs not having this MSR.
> 
> The patch could have been added a conditional to the MSR write in 
> user_disable_single_step(), but centralizing the updates seems safer 
> and (looking forward) better manageable.

thanks Jan, applied.

	Ingo

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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
  2008-04-21 16:34   ` Jan Beulich
@ 2008-04-21 21:30     ` Roland McGrath
  0 siblings, 0 replies; 7+ messages in thread
From: Roland McGrath @ 2008-04-21 21:30 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ingo Molnar, Thomas Gleixner, linux-kernel

> >agreed. Jan, wanna send a fix for this?
> 
> Hmm, without knowing the context here I'd say the way it's done in
> .25 is quite reasonable; a BUG_ON() here would seem quite rude to
> me. I thought it really should be the call sites ensuring this doesn't
> get called, and the silent ignoring is just to prevent bringing the
> system down. A WARN_ON() (or perhaps even WARN_ON_ONCE()) would
> be the most I'd be inclined to add there.

Like I said, the call sites already do ensure that it doesn't get called.
If there were any such bug, then the 2.6.25 behavior of silently being
broken is inordinately rude.  Since we think there is no such bug now, the
BUG_ON would not be hit, and if it is then it's deservedly so.  I don't
care whether it's a BUG_ON or a WARN_ON, since the point is that it never
happen.  What I object to is the change you made in 2.6.25, which ensures
that extra cycles at low level will be spent on every call, to guarantee
that any such bug in the future is maximally confusing.  That is not helping.


Thanks,
Roland

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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
  2008-04-21 15:55 ` Ingo Molnar
@ 2008-04-21 16:34   ` Jan Beulich
  2008-04-21 21:30     ` Roland McGrath
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2008-04-21 16:34 UTC (permalink / raw)
  To: Ingo Molnar, Roland McGrath; +Cc: Thomas Gleixner, linux-kernel

>>> Ingo Molnar <mingo@elte.hu> 21.04.08 17:55 >>>
>
>* Roland McGrath <roland@redhat.com> wrote:
>
>> If TIF_DEBUGCTLMSR is ever set on a machine without the support, that 
>> is a bug we should diagnose earlier.  If you want some paranoia, then 
>> keep update_debugctlmsr but make it do:
>> 
>> 	BUG_ON(boot_cpu_data.x86 < 6);
>> 
>> instead.
>
>agreed. Jan, wanna send a fix for this?

Hmm, without knowing the context here I'd say the way it's done in
.25 is quite reasonable; a BUG_ON() here would seem quite rude to
me. I thought it really should be the call sites ensuring this doesn't
get called, and the silent ignoring is just to prevent bringing the
system down. A WARN_ON() (or perhaps even WARN_ON_ONCE()) would
be the most I'd be inclined to add there.

Jan


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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
  2008-04-18 19:43 Roland McGrath
@ 2008-04-21 15:55 ` Ingo Molnar
  2008-04-21 16:34   ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2008-04-21 15:55 UTC (permalink / raw)
  To: Roland McGrath; +Cc: Thomas Gleixner, Jan Beulich, linux-kernel


* Roland McGrath <roland@redhat.com> wrote:

> If TIF_DEBUGCTLMSR is ever set on a machine without the support, that 
> is a bug we should diagnose earlier.  If you want some paranoia, then 
> keep update_debugctlmsr but make it do:
> 
> 	BUG_ON(boot_cpu_data.x86 < 6);
> 
> instead.

agreed. Jan, wanna send a fix for this?

	Ingo

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

* Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR
@ 2008-04-18 19:43 Roland McGrath
  2008-04-21 15:55 ` Ingo Molnar
  0 siblings, 1 reply; 7+ messages in thread
From: Roland McGrath @ 2008-04-18 19:43 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner; +Cc: Jan Beulich, linux-kernel

I don't think this was a good idea:

	commit 5b0e508415a83989fe704b4718a1a214bc333ca7
	Author: Jan Beulich <jbeulich@novell.com>
	Date:   Mon Mar 10 13:11:17 2008 +0000

	    x86: prevent unconditional writes to DebugCtl MSR

It's already a bug if there is any unconditional use of the MSR.
Silenting ignoring it is just wrong.

There was such a bug before this fix:

	commit 4ba51fd75cc3789be83f0d6f878dabbb0cb19bca
	Author: Roland McGrath <roland@redhat.com>
	Date:   Thu Apr 3 14:18:55 2008 -0700

	    x86 ptrace: avoid unnecessary wrmsr

But there should not be any more.  The use of the MSR for block-step is
controlled by arch_has_block_step(), which uses the same condition.  Any
use of the MSR for other purposes (DS et al) should be controlled by more
specific CPU model checks.  

If TIF_DEBUGCTLMSR is ever set on a machine without the support, that is a
bug we should diagnose earlier.  If you want some paranoia, then keep
update_debugctlmsr but make it do:

	BUG_ON(boot_cpu_data.x86 < 6);

instead.


Thanks,
Roland

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

end of thread, other threads:[~2008-04-21 21:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-10 13:11 [PATCH] x86: prevent unconditional writes to DebugCtl MSR Jan Beulich
2008-03-10 15:56 ` Andi Kleen
2008-03-10 17:14 ` Ingo Molnar
2008-04-18 19:43 Roland McGrath
2008-04-21 15:55 ` Ingo Molnar
2008-04-21 16:34   ` Jan Beulich
2008-04-21 21:30     ` Roland McGrath

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).