Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Roland McGrath]

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

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Ingo Molnar]

* 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

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Jan Beulich]

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

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Roland McGrath]

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

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Ingo Molnar]

* 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

Re: [PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Andi Kleen]

"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

[PATCH] x86: prevent unconditional writes to DebugCtl MSR

[Jan Beulich]

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;